fix: use null comparators in cases where zero or false are meaningful#2513
fix: use null comparators in cases where zero or false are meaningful#2513ItsMattL wants to merge 6 commits intoterraform-google-modules:mainfrom
Conversation
…support null values
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @ItsMattL, 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 resolves a critical GKE cluster import problem by refining how Terraform handles optional fields, ensuring that 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where false or 0 values for certain GKE node pool attributes were incorrectly treated as unset, leading to problems with Terraform import and state management. The changes modify the logic for queued_provisioning and advanced_machine_features dynamic blocks to correctly differentiate between unset (null) values and explicit false or 0 values. This is achieved by using lookup(..., null) != null checks to determine if an attribute is present in the configuration. The fix is applied consistently across all relevant cluster and node pool resources in the various modules. The changes are correct and effectively resolve the described issue.
|
/gcbrun |
apeabody
left a comment
There was a problem hiding this comment.
Thanks for the contribution @ItsMattL!
From the tests:
Step #63 - "apply node-pool-local": TestNodePool 2026-01-06T18:12:31Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1;
Step #63 - "apply node-pool-local": Error: Missing required argument
Step #63 - "apply node-pool-local":
Step #63 - "apply node-pool-local": with module.example.module.gke.google_container_node_pool.pools["pool-05"],
Step #63 - "apply node-pool-local": on ../../../modules/beta-public-cluster/cluster.tf line 850, in resource "google_container_node_pool" "pools":
Step #63 - "apply node-pool-local": 850: node_config {
Step #63 - "apply node-pool-local":
Step #63 - "apply node-pool-local": The argument "node_config.0.advanced_machine_features.0.threads_per_core" is
Step #63 - "apply node-pool-local": required, but no definition was found.}
Step #63 - "apply node-pool-local": apply.go:34:
Step #63 - "apply node-pool-local": Error Trace: /builder/home/go/pkg/mod/github.com/gruntwork-io/terratest@v0.54.0/modules/terraform/apply.go:34
Step #63 - "apply node-pool-local": /builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.17.9/pkg/tft/terraform.go:571
Step #63 - "apply node-pool-local": /builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.17.9/pkg/tft/terraform.go:630
Step #63 - "apply node-pool-local": /builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.17.9/pkg/tft/terraform.go:669
Step #63 - "apply node-pool-local": /builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.17.9/pkg/utils/stages.go:31
Step #63 - "apply node-pool-local": /builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/blueprint-test@v0.17.9/pkg/tft/terraform.go:669
Step #63 - "apply node-pool-local": /workspace/test/integration/node_pool/node_pool_test.go:113
Step #63 - "apply node-pool-local": Error: Received unexpected error:
Step #63 - "apply node-pool-local": FatalError{Underlying: error while running command: exit status 1;
Step #63 - "apply node-pool-local": Error: Missing required argument
Step #63 - "apply node-pool-local":
Step #63 - "apply node-pool-local": with module.example.module.gke.google_container_node_pool.pools["pool-05"],
Step #63 - "apply node-pool-local": on ../../../modules/beta-public-cluster/cluster.tf line 850, in resource "google_container_node_pool" "pools":
Step #63 - "apply node-pool-local": 850: node_config {
Step #63 - "apply node-pool-local":
Step #63 - "apply node-pool-local": The argument "node_config.0.advanced_machine_features.0.threads_per_core" is
Step #63 - "apply node-pool-local": required, but no definition was found.}
Step #63 - "apply node-pool-local": Test: TestNodePool
|
Pardon my ignorance, do we think this fix will go into the next release? |
|
Restored the original default of 0 for threads_per_core. |
|
/gcbrun |
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where 0 or false values are incorrectly treated as null (unset), causing issues with Terraform state and resource imports. The changes to the for_each logic in the dynamic blocks are a good step in the right direction. However, the fix is incomplete. In the advanced_machine_features dynamic block, the threads_per_core attribute is still assigned a default value of 0 inside the content block across multiple files. This undermines the change in the for_each and will continue to cause issues when the attribute is not specified. I've left comments on all occurrences of this issue with a suggestion to use null as the default instead, which will fully resolve the problem described.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
While the for_each condition for the advanced_machine_features dynamic block has been correctly updated to distinguish between an unset value (null) and 0 for threads_per_core, the value assignment within the block still defaults to 0. This can lead to unintended behavior where threads_per_core is set to 0 even when it's not specified in the input, which is the issue this PR aims to fix. To be consistent with the changes and the PR's intent, you should use null as the default value here as well.
threads_per_core = lookup(each.value, "threads_per_core", null)
There was a problem hiding this comment.
This is likely still relevant as the content will be used if any of the conditions are not null, so a null default for threads_per_core would be preferable.
There was a problem hiding this comment.
This interaction is so confusing even Gemini can't follow it, but I think it's right the way it currently is. The first iteration of this PR set all fields to null. I believe we then discovered that threads_per_core is in fact required to be set if advanced_machine_features is configured, so 48d3d10 followed up and changed it back to zero. In other words:
- If no fields are configured (all null), do not set advanced_machine_features.
- If any fields are set, set advanced_machine_features plus
- If threads_per_core is configured set it to configured
- If threads_per_core is unconfigured, set to zero.
The comment in #2513 (review) contains the output stating that
"threads_per_core" is required
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
This line still defaults threads_per_core to 0. To align with the goal of this PR, which is to differentiate between 0 and an unset value, the default should be null. This prevents Terraform from sending a 0 value when the attribute is not specified.
threads_per_core = lookup(each.value, "threads_per_core", null)
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
I suspect the error isn't related to the change: |
|
/gcbrun |
|
Ugh still failing. From those errors you shared it appears the /builder/home/.kube/config must be getting generated dynamically and incorrectly somehow. |
I'll re-run |
| for_each = lookup(each.value, "threads_per_core", 0) > 0 || lookup(each.value, "enable_nested_virtualization", false) || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| for_each = lookup(each.value, "threads_per_core", null) != null || lookup(each.value, "enable_nested_virtualization", null) != null || lookup(each.value, "performance_monitoring_unit", null) != null ? [1] : [] | ||
| content { | ||
| threads_per_core = lookup(each.value, "threads_per_core", 0) |
There was a problem hiding this comment.
This is likely still relevant as the content will be used if any of the conditions are not null, so a null default for threads_per_core would be preferable.
As a documented in issues/2304, there are multiple resources where the module incorrectly conflates an undefined field with a defined false or zero value, including advanced_machine_features/enable_nested_virtualization (false vs null), advanced_machine_features/threads_per_core (0 vs null), and queued_provisioning/enabled (false vs null).
In cases where a cluster was first created via the GKE UI, it becomes impossible to later import it into terraform, as the module misinterprets these defaults and tries to force replacement.
Fixes #2304.