Repository navigation
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses deployment failures occurring when firewall modules receive VPC outputs that are unknown during the planning phase. By introducing an explicit control variable, the module now allows users to bypass automatic subnetwork lookups when necessary, ensuring robust planning even in fresh infrastructure deployments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new use_subnetwork_data input variable to the firewall-rules module, allowing users to bypass subnetwork lookups and Private Google Access checks when project and network details are explicitly provided. It also includes comprehensive integration tests and updated documentation. The feedback suggests removing nullable = false from the new variable definition to allow explicit null values without triggering default overrides, which aligns with Terraform best practices.
88dba26 to
4f0d983
Compare
|
/gcbrun |
Use an explicit subnet lookup switch to keep resource counts known during planning. Avoid interpolating a null subnet name in validation errors. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
af4325a to
1afb5f4
Compare
|
@aslam-quad can you |
|
/gcbrun |
|
The PR-validation (hpc-toolkit-dev) failure appears to be unrelated to the changes in this PR: |
saara-tyagi27
added
the
release-bugfix
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
|
@saara-tyagi27 can you rerun |
|
/gcbrun |
| terraform { | ||
| required_version = ">= 1.12.2" | ||
| } |
There was a problem hiding this comment.
[Major] Because this test fixture does not list google under required_providers, terraform test does not bind mock_provider "google" from tests/subnetwork_lookup.tftest.hcl to the child firewall modules. Instead, it initializes the real hashicorp/google provider and tries to load Application Default Credentials (you can reproduce this on an authenticated machine with GOOGLE_APPLICATION_CREDENTIALS=/nonexistent terraform test).
Please add google to required_providers here (and update tests/fixtures/new-network/README.md) so the test uses the mock provider:
| terraform { | |
| required_version = ">= 1.12.2" | |
| } | |
| terraform { | |
| required_providers { | |
| google = { | |
| source = "hashicorp/google" | |
| version = ">= 4.64" | |
| } | |
| } | |
| required_version = ">= 1.12.2" | |
| } |
There was a problem hiding this comment.
Thanks for reproducing this. I added google to the fixture requirements and regenerated its README.
| } | ||
| } | ||
| expect_failures = [terraform_data.pga_check] | ||
| } | ||
|
|
||
| run "new_network_values_unknown_until_apply" { |
There was a problem hiding this comment.
[Minor] During command = plan, the outputs from terraform_data.network are still unknown, so the variable checks, subnet read, and Private Google Access check are all postponed until apply.
Once google is added to required_providers in tests/fixtures/new-network/main.tf, you can switch this test to command = apply with an assertion so those postponed checks are verified too:
| } | |
| } | |
| expect_failures = [terraform_data.pga_check] | |
| } | |
| run "new_network_values_unknown_until_apply" { | |
| run "new_network_values_unknown_until_apply" { | |
| command = apply | |
| module { | |
| source = "./tests/fixtures/new-network" | |
| } | |
| assert { | |
| condition = terraform_data.network.output.project == "test-project" && terraform_data.network.output.name == "test-network" | |
| error_message = "New network outputs must resolve and apply cleanly." | |
| } | |
| } |
There was a problem hiding this comment.
Switched the new-network case to apply and added the assertion for the resolved project and network.
| run "missing_project_without_lookup" { | ||
| command = plan | ||
| variables { | ||
| project_id = null | ||
| } | ||
| expect_failures = [var.use_subnetwork_data] | ||
| } | ||
|
|
||
| run "empty_network_without_lookup" { | ||
| command = plan | ||
| variables { | ||
| network_name = "" | ||
| } | ||
| expect_failures = [var.use_subnetwork_data] | ||
| } |
There was a problem hiding this comment.
[Minor] These two tests cover project_id = null and network_name = "" when use_subnetwork_data = false, but they do not test project_id = "" or network_name = null. Consider adding those two cases so all four parts of the input check in variables.tf are covered:
| run "missing_project_without_lookup" { | |
| command = plan | |
| variables { | |
| project_id = null | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } | |
| run "empty_network_without_lookup" { | |
| command = plan | |
| variables { | |
| network_name = "" | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } | |
| run "missing_project_without_lookup" { | |
| command = plan | |
| variables { | |
| project_id = null | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } | |
| run "empty_project_without_lookup" { | |
| command = plan | |
| variables { | |
| project_id = "" | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } | |
| run "missing_network_without_lookup" { | |
| command = plan | |
| variables { | |
| network_name = null | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } | |
| run "empty_network_without_lookup" { | |
| command = plan | |
| variables { | |
| network_name = "" | |
| } | |
| expect_failures = [var.use_subnetwork_data] | |
| } |
There was a problem hiding this comment.
Added both cases. Null and empty values are now covered for each input.
| # the google_compute_network data source does not allow identification by | ||
| # self_link, which uniquely identifies subnet, project, and network | ||
| data "google_compute_subnetwork" "subnetwork" { | ||
| # Only instantiate this data source if needed |
There was a problem hiding this comment.
[Nit] With this change, the subnet is read whenever var.use_subnetwork_data is true (even when project_id and network_name are already provided, such as when a blueprint uses use: [network]). Let's update this comment so it matches the new behavior:
| # Only instantiate this data source if needed | |
| # Look up the subnetwork when enabled by use_subnetwork_data |
There was a problem hiding this comment.
Updated the comment to match the lookup switch.
| By default, this module looks up `subnetwork_self_link` to derive missing project | ||
| and network values. When either value is omitted, it also verifies that Private | ||
| Google Access is enabled. The subnet can be created in the same deployment group; | ||
| its lookup waits until it exists. |
There was a problem hiding this comment.
[Nit] When use_subnetwork_data is true (the default), the module still looks up subnetwork_self_link even if both project_id and network_name are passed (for example, via use: [network]), and only uses the looked-up values when either input is missing. Making that explicit here helps clarify when the subnet read happens:
| By default, this module looks up `subnetwork_self_link` to derive missing project | |
| and network values. When either value is omitted, it also verifies that Private | |
| Google Access is enabled. The subnet can be created in the same deployment group; | |
| its lookup waits until it exists. | |
| By default (`use_subnetwork_data: true`), this module looks up `subnetwork_self_link` | |
| and uses it to fill in any missing `project_id` or `network_name` value. When either | |
| value is omitted, it also verifies that Private Google Access is enabled. The subnet | |
| can be created in the same deployment group; its lookup waits until it exists. |
There was a problem hiding this comment.
Clarified the default lookup behavior in the README.
There was a problem hiding this comment.
The firewall module changes look good.
Running terraform test locally showed one Major issue in the new test fixture (tests/fixtures/new-network/main.tf is missing google in required_providers, so the test fails without live GCP credentials), plus a few small test and comment suggestions below.
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
shubpal07
left a comment
shubpal07
left a comment
There was a problem hiding this comment.
Thanks for addressing the suggestions!!
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
|
The https://github.com/GoogleCloudPlatform/cluster-toolkit/actions/runs/41898092853/job/127659083551?pr=6536 and https://github.com/GoogleCloudPlatform/cluster-toolkit/actions/runs/41898092853/job/127659083935?pr=6536 pre-commit jobs failed because TFLint flagged the Google provider declaration needed by the mock test. I added the same lint ignore used elsewhere in the repo in f19de27. |
Fresh deployments can fail with
Invalid count argumentwhen the firewall module receives VPC outputs that are unknown until apply. Select subnet lookup with a plan-timeuse_subnetwork_databoolean instead of testing those outputs for null. Also avoid interpolating a null subnet name in the Private Google Access error message.Subnet lookup now defaults to enabled. As before, Private Google Access is required only when deriving a missing project or network value. Callers supplying project and network directly can set
use_subnetwork_data: falseto retain the previous skip behavior. Existing indexed resource addresses are preserved.Related: #6334. This PR contains only the shared firewall fix, documentation, and regression tests.
Validation: a fresh agentgateway deployment passed through
gclusterwith the fix, including live Gemini and synthetic-provider synchronous/streaming/metrics tests and Cloud Logging. Reapplication with the final patch reported all four deployment groups already applied. HCL formatting, TFLint, documentation generation, and applicable non-Terraform pre-commit checks passed. Added targeted Terraform regression cases; they have not been executed because local repository instructions prohibit direct Terraform commands. Fullmake testsand coverage verification remain pending.Live test cleanup completed successfully: the cluster, VPC, nodes and reserved addresses were removed; administrator-provisioned service accounts were preserved.
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.