Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add computed to node_config.kubelet_config & node_pool_auto_config.node_kubelet_config #11971

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

rileykarson
Copy link
Member

@rileykarson rileykarson commented Oct 10, 2024

Fixes hashicorp/terraform-provider-google#19792

Support for the node_config.kubelet_config.insecure_kubelet_readonly_port_enabled field was added in #11573 and was marked Computed. node_config itself is also Computed, however node_config.kubelet_config is not.

This means that:

  • A configuration that does not specify node_config is not affected
  • A configuration that specifies node_config but not node_config.kubelet_config is affected
  • A configuration that specifies node_config.kubelet_config is not affected.

This caused numerous failing tests for us, however, not all the tests we would have expected to fail based on those criteria.I haven't determined another trigger condition so I suspect it's a partial rollout of a serialisation change. This was confirmed as a rollout in hashicorp/terraform-provider-google#19792 (comment).

kubelet_config appears in multiple other locations, node_pool_defaults.node_config_defaults.insecure_kubelet_readonly_port_enabled and node_pool_auto_config.node_kubelet_config.insecure_kubelet_readonly_port_enabled

  • node_pool_defaults.node_config_defaults has the same problem, as it is not Computed while node_pool_defaults and insecure_kubelet_readonly_port_enabled are. However, sending just node_pool_defaults {} already triggers the same issue with another field (logging_variant). Ideally we'd make node_config_defaults Required, but I'm not sure about the impact on autopilot. I'll file a bug.
  • I don't expect a default value to be set for node_pool_auto_config.node_kubelet_config.insecure_kubelet_readonly_port_enabled but I've set node_pool_auto_config.node_kubelet_config to Computed anyways since it could exhibit the same issue and I would not want to have to deal with the same problem again. I may cut the change if I can confirm it will never get a default (but I'm not guaranteed / unlikely to find that answer before shipping the change)

The update part of the original issue (Error: googleapi: Error 400: At least one of ['node_version', <snip>, 'max_run_duration'] must be specified.) is because we used ForceSendFields with a nil KubeletConfig value, which does nothing. We had to set KubeletConfig to &container.KubeletConfig{} in that block. NullFields would be correct for a patch in theory, but this is a PUT / customish update and the JSON null still triggers the API error.

After this change we can no longer trigger that specific update message. I'll still clean up that callsite / any similarly structured ones to ensure we send an empty struct properly if we do hit them, but in a separate change.

Release Note Template for Downstream PRs (will be copied)

container: fixed a diff triggered by a new API-side default value for `node_config.0.kubelet_config.0.insecure_kubelet_readonly_port_enabled`. Terraform will now accept server-specified values for `node_config.0.kubelet_config` when it is not defined in configuration and will not detect drift. Note that this means that removing the value from configuration will now preserve old settings instead of reverting the old settings.
container: `google_container_cluster` will now accept server-specified values for `node_pool_auto_config.0.node_kubelet_config` when it is not defined in configuration and will not detect drift. Note that this means that removing the value from configuration will now preserve old settings instead of reverting the old settings.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 2 insertions(+))
google-beta provider: Diff ( 1 file changed, 2 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 212
Passed tests: 199
Skipped tests: 13
Affected tests: 0

Click here to see the affected service packages
  • container

🟢 All tests passed!

View the build log

@rileykarson rileykarson requested a review from melinath October 10, 2024 21:17
@rileykarson
Copy link
Member Author

VCR will not detect the bug, as the cassettes are old and either version of the code passes on old messages. I've simulated this locally by turning off the insecure port, as my invocations have not been lucky enough to hit the rollout of the changed serialisation. I also kicked off a TC run which is currently incomplete, but shows success so far.

@rileykarson rileykarson requested review from slevenick and removed request for melinath October 10, 2024 21:40
@rileykarson rileykarson merged commit 6fa720f into GoogleCloudPlatform:main Oct 10, 2024
13 checks passed
@rileykarson rileykarson deleted the kubeletconfig-fix branch October 10, 2024 22:34
gontech pushed a commit to gontech/magic-modules that referenced this pull request Oct 16, 2024
jingyih added a commit to jingyih/k8s-config-connector that referenced this pull request Oct 22, 2024
…les/pull/11971

This should fix the reconciliation error when a subfield under
kubeletConfig is set outside of KCC/TF.
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Oct 23, 2024
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Oct 24, 2024
xiaoweim pushed a commit to xiaoweim/k8s-config-connector that referenced this pull request Oct 28, 2024
…les/pull/11971

This should fix the reconciliation error when a subfield under
kubeletConfig is set outside of KCC/TF.
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Nov 5, 2024
akshat-jindal-nit pushed a commit to akshat-jindal-nit/magic-modules that referenced this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provider setting false values in google_container_node_pool kubelet_config, breaking tf apply
3 participants