Do not force send insecure_kubelet_readonly_port_enabled during creation

[shuyama1]

potentially fixes hashicorp/terraform-provider-google#19428

Release Note Template for Downstream PRs (will be copied)

container: fixed a bug where specifying `node_pool_defaults.node_config_defaults` with `enable_autopilot = true` will cause `google_container_cluster` resource creation failure

[modular-magician]

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 ( 2 files changed, 2 deletions(-))
google-beta provider: Diff ( 2 files changed, 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 208
Passed tests: 192
Skipped tests: 13
Affected tests: 3

Click here to see the affected service packages

• container

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledDefaultsUpdates
• TestAccContainerCluster_withLoggingVariantUpdates
• TestAccContainerCluster_withNodePoolDefaults

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withLoggingVariantUpdates[Debug log]
TestAccContainerCluster_withNodePoolDefaults[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledDefaultsUpdates[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[modular-magician]

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, 1 deletion(-))
google-beta provider: Diff ( 1 file changed, 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 208
Passed tests: 195
Skipped tests: 13
Affected tests: 0

Click here to see the affected service packages

• container

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

[modular-magician]

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 ( 2 files changed, 865 insertions(+), 791 deletions(-))
google-beta provider: Diff ( 2 files changed, 939 insertions(+), 879 deletions(-))

[modular-magician]

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• container

#### Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccContainerCluster_withAutopilot_withNodePoolDefaults
  $\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$

View the build log

[modular-magician]

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 ( 2 files changed, 45 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 45 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• container

#### Non-exercised tests

Tests were added that are skipped in VCR:

• TestAccContainerCluster_withAutopilot_withNodePoolDefaults
  $\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$

View the build log

[modular-magician]

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 ( 2 files changed, 45 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 45 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 209
Passed tests: 195
Skipped tests: 13
Affected tests: 1

Click here to see the affected service packages

• container

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccContainerCluster_withAutopilot_withNodePoolDefaults

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAutopilot_withNodePoolDefaults[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[shuyama1]

/gcbrun

[modular-magician]

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 ( 2 files changed, 45 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 45 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 196
Skipped tests: 13
Affected tests: 1

Click here to see the affected service packages

• container

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccContainerCluster_withAutopilot_withNodePoolDefaults

Get to know how VCR tests work

[wyardley]

@shuyama1 sorry if I introduced the issue described in hashicorp/terraform-provider-google#19428 in #11272 or #11573... from what I remember, forcesendfields may have been needed in some situation, (which might explain the failing test?), so maybe some additional code changes will be needed to make this work vs. just removing it outright? Could be wrong, though.

Maybe the setting can just be disallowed or ignored when the cluster is an autopilot cluster?

Side note: I think the release note is not fully filled out above, unless that's something that will get updated later?

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAutopilot_withNodePoolDefaults[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[shuyama1]

@shuyama1 sorry if I introduced the issue described in hashicorp/terraform-provider-google#19428 in #11272 or #11573... from what I remember, forcesendfields may have been needed in some situation, (which might explain the failing test?), so maybe some additional code changes will be needed to make this work vs. just removing it outright? Could be wrong, though.

Maybe the setting can just be disallowed or ignored when the cluster is an autopilot cluster?

@wyardley Hey, thanks for working on these feature implementations! GKE cluster resource may have some edge cases that that're a bit tricky to spot in advance - this seems like one. I'm testing locally o determine if forcesendfields is necessary here - removing it from the creation seems to resolve the issue described in the ticket. The test failure in this PR is due to a permadiff caused by another field. I'm investigating the cause of it and working on a fix as well.

  ~ node_pool_defaults {
      ~ node_config_defaults {
            # (2 unchanged attributes hidden)

          - gcfs_config {
              - enabled = true -> null
            }
        }
    }

Side note: I think the release note is not fully filled out above, unless that's something that will get updated later?

Thanks for catching it! Yes, I was planning to update it before merging the PR

[wyardley]

I'm investigating the cause of it and working on a fix as well.

@shuyama1: #11717 (which you're also assigned to review) should fix it, I believe

[modular-magician]

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 ( 2 files changed, 46 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 46 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 196
Skipped tests: 13
Affected tests: 1

Click here to see the affected service packages

• container

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccContainerCluster_withAutopilot_withNodePoolDefaults

Get to know how VCR tests work

[wyardley]

@shuyama1 it's possible it should also be removed in

magic-modules/mmv1/third_party/terraform/services/container/node_config.go.erb

Line 818 in 1cee8f6

ForceSendFields: []string{"InsecureKubeletReadonlyPortEnabled"},

as well, if the tests pass.

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withAutopilot_withNodePoolDefaults[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log or the debug log for each test

[shuyama1]

@shuyama1 it's possible it should also be removed in

magic-modules/mmv1/third_party/terraform/services/container/node_config.go.erb

Line 818 in 1cee8f6

ForceSendFields: []string{"InsecureKubeletReadonlyPortEnabled"},

as well, if the tests pass.

Hey @wyardley Yes, I believe that removing the forced send of InsecureKubeletReadonlyPortEnabled should resolve the issue listed in the description of this PR. Additionally, your PR #11717 should address the premadiff discovered in the test failure for this PR. Thank you for your work on the fix!

[wyardley]

Additionally, your PR #11717 should address the premadiff discovered in the test failure for this PR. Thank you for your work on the fix!

Thanks! Please also be aware of #11697

Hopefully, these 3 can all go out together in 6.4.0 or 6.5.0; hopefully they can get squeezed in

[modular-magician]

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 ( 2 files changed, 45 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 2 files changed, 45 insertions(+), 1 deletion(-))

[modular-magician]

Tests analytics

Total tests: 211
Passed tests: 198
Skipped tests: 13
Affected tests: 0

Click here to see the affected service packages

• container

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

[wyardley]

Yes, I believe that removing the forced send of InsecureKubeletReadonlyPortEnabled should resolve the issue listed in the description of this PR

Oh, I get it now, this is for node_config_defaults.node_kubelet_config only. node_config.kubelet_config is the one I was thinking was originally getting updated here 🤦

magic-modules/mmv1/third_party/terraform/services/container/node_config.go.erb

Line 1218 in 20ac9f9

kConfig.ForceSendFields = append(kConfig.ForceSendFields, "InsecureKubeletReadonlyPortEnabled")

[ScottSuarez]

@shuyama1, wouldn't this not allow the field to be unset? I notice that this field is marked as Computed in the schema?

https://github.com/shuyama1/magic-modules/blob/e48cdbdb59cf71edfd96fe0e4ada0dc413dc2157/mmv1/third_party/terraform/services/container/node_config.go.erb#L87

[shuyama1]

@shuyama1, wouldn't this not allow the field to be unset? I notice that this field is marked as Computed in the schema?

shuyama1/magic-modules@e48cdbd/mmv1/third_party/terraform/services/container/node_config.go.erb#L87

I've only removed it from the ForceSendFields in create but not update, so I think it should be fine, but I can test it locally to confirm