container: add support for kubelet read only port

[wyardley]

• Add no_enable_insecure_kubelet_readonly_port to google_container_cluster
• Allow setting insecure_kubelet_readonly_port_enabled for container_node_pool and friends

https://cloud.google.com/kubernetes-engine/docs/how-to/disable-kubelet-readonly-port

Fixes hashicorp/terraform-provider-google#15208

Note: @trodge: may be good to get some feedback internally from Google folks about what the right default behavior should be and if this will change over time. I'm trying to do this in the least breaking (for the provider) way, but from my very quick reading of the announcements, it's possible that default behavior may change in the future (and that this may depend on new vs. existing clusters and / or based on cluster version).

Release Note Template for Downstream PRs (will be copied)

container: added `insecure_kubelet_readonly_port_enabled` to `node_pool.node_config.kubelet_config` and `node_config.kubelet_config` in `google_container_node_pool` resource.

container: added `insecure_kubelet_readonly_port_enabled` to `node_pool_defaults.node_config_defaults`, `node_pool.node_config.kubelet_config`, and `node_config.kubelet_config` in `google_container_cluster` resource.

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@trodge, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[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 ( 4 files changed, 174 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 4 files changed, 174 insertions(+), 7 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (375 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_cluster" "primary" {
  node_config {
    kubelet_config {
      insecure_kubelet_readonly_port_enabled = # value needed
    }
  }
}

Resource: google_container_node_pool (71 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_node_pool" "primary" {
  node_config {
    kubelet_config {
      insecure_kubelet_readonly_port_enabled = # value needed
    }
  }
}

[michellepolte-optimizely]

Would be great to get this fixed asap depends on notification from GCP a lot of user will need this.

[wyardley]

Just a note that it will be at least a week before I can dig into it much more, and it may be a tricky one to finish / get working. So if anyone has time to take it on, or if anyone from the team that works on this project wants to take over, I will not be at all unhappy.

[michellepolte-optimizely]

Do we have any updates here? We have already next week without any progress...

[wyardley]

@trodge I know this is in draft still, but any chance you could take a look at what I've got so far? I think there is still some code missing, but I want to make sure I'm going in the right direction (it at least seems to build and work to do a basic plan off of)

Also curious if there are any docs on how, e.g., mmv1/third_party/terraform/services/container/go/node_config.go.tmpl and mmv1/third_party/terraform/services/container/node_config.go.erb interact (assume this is part of the tpgtools transition), and if one generates the other... I asked in the GCP Slack too, but so far didn't hear anything.

Also, I think the google_container_node_pool acceptance test should more or less cover the bit that @modular-magician is complaining about, but not sure if it needs to be a standalone test case, or if I'm just missing one of the variants.

[trodge]

@trodge I know this is in draft still, but any chance you could take a look at what I've got so far? I think there is still some code missing, but I want to make sure I'm going in the right direction (it at least seems to build and work to do a basic plan off of)

Also curious if there are any docs on how, e.g., mmv1/third_party/terraform/services/container/go/node_config.go.tmpl and mmv1/third_party/terraform/services/container/node_config.go.erb interact (assume this is part of the tpgtools transition), and if one generates the other... I asked in the GCP Slack too, but so far didn't hear anything.

They do not interact at the moment. The .go.tmpl file will in the future replace the .go.erb file when the mmv1 generator is fully migrated from ruby to go. This is also completely separate from the tpgtools transition.

Also, I think the google_container_node_pool acceptance test should more or less cover the bit that @modular-magician is complaining about, but not sure if it needs to be a standalone test case, or if I'm just missing one of the variants.

I believe the missing test detection is failing because the provider isn't building, and the provider isn't building because of this error:

Error: google-beta/services/container/resource_container_cluster_test.go:1519:13: undefined: testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfig
Error: google-beta/services/container/resource_container_cluster_test.go:1544:117: cannot use false (untyped bool constant) as string value in argument to testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodePoolBool
Error: google-beta/services/container/resource_container_cluster_test.go:1568:108: cannot use true (untyped bool constant) as string value in argument to testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledNodePoolDefaultBool

The files and line numbers here refer to the generated code in the downstream.

[wyardley]

Hi, thanks for responding.

They do not interact at the moment. The .go.tmpl file will in the future replace the .go.erb file when the mmv1 generator is fully migrated from ruby to go. This is also completely separate from the tpgtools transition.

Thanks -- this is all super helpful information. Are there any (non-Google / Hashicorp internal) docs that explain this at high level? Do the tests run against both sets, and is there anything I can / should do to help make sure they stay in sync while I'm working on stuff?

and the provider isn't building because of this error:

Huh, it was building for me locally, though maybe the test code doesn't get generated / built when you run PRODUCT=container? I just tried building ga and beta for all products off my branch, and I can successfully run make and a provider is built (go vet and go install run without error). I will take a look at the errors.

[edit: I can reproduce the failure with go test ./...; so I think as long as I stick to the unit tests, should be able to figure that out]

Side note: with some recent change, I don't seem to be able to run the magic modules build on OS X without doing ulimit -n unlimited first, which has always worked in the past.

[wyardley]

The updated version should fix the basic problem (at least those tests build now locally), though there may still be some issues with the provider code and / or tests.

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfigUpdates[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

[melinath]

LGTM, just a couple minor changes to docs.

[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 ( 5 files changed, 371 insertions(+), 15 deletions(-))
google-beta provider: Diff ( 5 files changed, 372 insertions(+), 16 deletions(-))

[modular-magician]

Tests analytics

Total tests: 203
Passed tests: 190
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]

Thanks for all the feedback / help @melinath and @trodge! Really appreciate it. And thanks for pushing on getting the update thing to work as well, in the end, much happier that it's shipped with that.

Maybe double-check that the release notes as I had them are also good.

[wyardley]

Also, @melinath: you or someone will cherry-pick this back into 5.x, correct?

[wyardley]

We may see some issues related to hashicorp/terraform-provider-google#15767 once people start using this setting -- I believe cpu_cfs_quota should mostly be a noop for existing clusters when not set, but could cause some inconsistent behavior if a cluster is newly created with this setting, and without setting cpu_cfs_quota to the API default of true explicitly -- i.e., the newly created cluster would come up with cpu_cfs_quota set to false vs. the default behavior of true.

[melinath]

@wyardley Thanks for sticking with it! Yes, we're planning to backport this into 5.X.

[wyardley]

There's a soft tab instead of a hard tab I missed in one spot, not sure if it's worth fixing, especially if they'll get fixed with the move to go templates at some point? I can make a tiny PR to update it if you'd like though.

[melinath]

don't worry about it at this point. Thanks!