feat(google_container_cluster): support enable k8s beta apis #8355

toVersus · 2023-07-14T09:43:45Z

Fixes: hashicorp/terraform-provider-google#15174

This PR added support for enabling beta APIs for the google_container_cluster resource. This allows users to select which beta resources they wish to use. Once activated, they cannot be deactivated. See Configure beta APIs and corresponding REST API doc.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

container: added `enable_k8s_beta_apis.enabled_apis` field to `google_container_cluster`

modular-magician · 2023-07-14T09:44:24Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @melinath, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

mmv1/third_party/terraform/tests/resource_container_cluster_test.go.erb

melinath

marking reviewed pending the switch to using a datasource

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

modular-magician · 2023-07-17T16:11:36Z

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.

Terraform GA: Diff ( 3 files changed, 163 insertions(+))
Terraform Beta: Diff ( 3 files changed, 163 insertions(+))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-07-17T17:31:55Z

Tests analytics

Total tests: 2862
Passed tests 2560
Skipped tests: 299
Affected tests: 3

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

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccContainerCluster_withEnableKubernetesBetaAPIs|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-17T17:41:31Z

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

Rerun these tests in REPLAYING mode to catch issues

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

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

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

coolkp · 2023-07-18T15:35:57Z

this feature is available from 1.27 and currently we can enable API, authentication.k8s.io/v1beta1/selfsubjectreviews. it must be passed in this exact form. Also we cannot disable

melinath · 2023-07-18T15:40:23Z

@coolkp hm, not being able to disable them is a little weird. Is there any beta API that we could rely on existing for testing, or should we just expect this test to break occasionally when APIs are removed?

modular-magician · 2023-07-18T15:43:25Z

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.

Terraform GA: Diff ( 4 files changed, 153 insertions(+))
Terraform Beta: Diff ( 3 files changed, 152 insertions(+))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

coolkp · 2023-07-18T15:48:41Z

@coolkp hm, not being able to disable them is a little weird. Is there any beta API that we could rely on existing for testing, or should we just expect this test to break occasionally when APIs are removed?

@melinath reason why we don't allow disabling is because of lack of controller capability to cleanup any k8s objects created by the api . We ultimately must sync this with k8s upstream open api spec. but beta apis cannot be removed for minimum of 3 versions so we have around a year to do that. in this case we can rely on this api for next 3 versions. I did consider having a allowlisted mock api for test purposes, but this is a front facing feature.

melinath · 2023-07-18T17:00:53Z

@melinath reason why we don't allow disabling is because of lack of controller capability to cleanup any k8s objects created by the api

@coolkp would this be the case for all k8s beta APIs, or just this one? Basically the reason I'm asking is because we need to figure out what the Terraform UX should be for users who remove an API from this field. There are basically two options:

Treat the field as updatable. If there are any API errors (for example from trying to disable a beta k8s API) display those to the user so they can decide what to do.
- Pros: Does not recreate the cluster.
- Cons: Does not allow users to freely change configuration.
Recreate the cluster when a previously-enabled beta k8s API is removed from the list.
- Pros: Allows users to set up a configuration and have confidence that the resource will match the configuration after apply.
- Cons: Recreates the cluster, which takes a while and may lose some internal state.

modular-magician · 2023-07-18T17:02:44Z

Tests analytics

Total tests: 2864
Passed tests 2562
Skipped tests: 299
Affected tests: 3

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_withEnableKubernetesBetaAPIs|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccComputeFirewallPolicyRule_multipleRules

Get to know how VCR tests work

modular-magician · 2023-07-18T17:13:43Z

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

Rerun these tests in REPLAYING mode to catch issues

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

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

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

coolkp · 2023-07-18T18:57:26Z

@melinath reason why we don't allow disabling is because of lack of controller capability to cleanup any k8s objects created by the api

@coolkp would this be the case for all k8s beta APIs, or just this one? Basically the reason I'm asking is because we need to figure out what the Terraform UX should be for users who remove an API from this field. There are basically two options:

Treat the field as updatable. If there are any API errors (for example from trying to disable a beta k8s API) display those to the user so they can decide what to do.

Pros: Does not recreate the cluster.

Cons: Does not allow users to freely change configuration.

Recreate the cluster when a previously-enabled beta k8s API is removed from the list.

Pros: Allows users to set up a configuration and have confidence that the resource will match the configuration after apply.

Cons: Recreates the cluster, which takes a while and may lose some internal state.

ideally i prefer validation to prevent field from being removed but option 1 is ok too show the api call validation error

melinath · 2023-07-18T20:16:29Z

Generally we prefer not to handle this kind of validation in case the API behavior changes - let's leave it as an error for now.

melinath · 2023-07-20T18:18:22Z

mmv1/third_party/terraform/tests/resource_container_cluster_test.go.erb

+  location = "us-central1-a"
+
+  min_master_version = data.google_container_engine_versions.uscentral1a.release_channel_latest_version["STABLE"]
+  # TODO: Migrate to supported Beta APIs when available


I'm not sure I understand the action to be taken from this - could you add more information to the comment? And/or we could discuss in this thread.

Added more context.

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

melinath

This is looking really good! A couple small requests related to tests.

melinath · 2023-07-25T17:04:43Z

mmv1/third_party/terraform/tests/resource_container_cluster_test.go.erb

+		PreCheck:     func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		CheckDestroy: testAccCheckContainerClusterDestroyProducer(t),
+		Steps: []resource.TestStep{


If possible, we should add a second step here that adds a second enabled API to exercise the Update code. If that isn't possible because there is only one beta API, we should add a new test that creates a cluster without any enabled APIs and then enables this beta API in a separate test step.

Unfortunately, there is only one beta API available at this time. Added a new test case to create a GKE cluster and enable the beta API.

Thanks @toVersus yeah we don't have any other APIs for enabling

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

Signed-off-by: toVersus <[email protected]> * use TypeSet to avoid the re-ordering being treated as a diff * add custom diff to recreate cluster when removing enabled apis * add unit and integration tests

modular-magician · 2023-07-26T16:17:18Z

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.

Terraform GA: Diff ( 4 files changed, 338 insertions(+))
Terraform Beta: Diff ( 4 files changed, 333 insertions(+))
TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-07-26T17:35:19Z

Tests analytics

Total tests: 2884
Passed tests 2579
Skipped tests: 301
Affected tests: 4

Action taken

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

TestAccProjectIamPolicy_invalidMembers|TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccContainerCluster_withEnableKubernetesBetaAPIsOnExistingCluster|TestAccContainerCluster_withEnableKubernetesBetaAPIs

Get to know how VCR tests work

modular-magician · 2023-07-26T17:53:48Z

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

Rerun these tests in REPLAYING mode to catch issues

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

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

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

melinath

LGTM modulo this one docs change. Thanks for your work on this!

mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown

…ml.markdown

melinath

Merging based on passing tests on the previous commit - the new commit is only a docs change.

coolkp · 2023-07-27T20:33:11Z

Thanks @melinath and @toVersus for shepherding this and making this feature available on terraform

modular-magician · 2023-07-27T20:42:52Z

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.

TF Conversion: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2023-07-27T22:17:31Z

Tests analytics

Total tests: 2890
Passed tests 2586
Skipped tests: 302
Affected tests: 2

Action taken

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

TestAccContainerAwsNodePool_BetaBasicHandWritten|TestAccVPCAccessConnector_vpcAccessConnectorExample

Get to know how VCR tests work

modular-magician · 2023-07-27T22:23:49Z

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

Rerun these tests in REPLAYING mode to catch issues

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

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

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

toVersus · 2023-07-28T00:49:40Z

Thanks to both @melinath and @coolkp for continuing to provide thoughtful reviews over the long term!

…loudPlatform#8355) * feat(google_container_cluster): support enable k8s beta apis Signed-off-by: toVersus <[email protected]> * use TypeSet to avoid the re-ordering being treated as a diff * add custom diff to recreate cluster when removing enabled apis * add unit and integration tests * fix: use contains instead of sets intersection * Update mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown --------- Co-authored-by: Stephen Lewis (Burrows) <[email protected]>

modular-magician requested a review from melinath July 14, 2023 09:44

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 14, 2023

toVersus commented Jul 14, 2023

View reviewed changes

mmv1/third_party/terraform/tests/resource_container_cluster_test.go.erb Outdated Show resolved Hide resolved

toVersus changed the title ~~feat(google_container_node_pool): support enable k8s beta apis~~ feat(google_container_cluster): support enable k8s beta apis Jul 14, 2023

toVersus force-pushed the feat/k8s-beta-api branch from dd0f508 to d0d6802 Compare July 14, 2023 09:59

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 17, 2023

melinath requested changes Jul 17, 2023

View reviewed changes

melinath reviewed Jul 17, 2023

View reviewed changes

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb Outdated Show resolved Hide resolved

toVersus force-pushed the feat/k8s-beta-api branch from d0d6802 to a3ef8ad Compare July 18, 2023 03:15

modular-magician requested a review from melinath July 18, 2023 03:15

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 18, 2023

toVersus force-pushed the feat/k8s-beta-api branch from a3ef8ad to f5b8311 Compare July 18, 2023 03:52

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 18, 2023

melinath requested a review from coolkp July 18, 2023 15:37

coolkp approved these changes Jul 18, 2023

View reviewed changes

melinath requested changes Jul 20, 2023

View reviewed changes

toVersus force-pushed the feat/k8s-beta-api branch from f313ab7 to d3b424e Compare July 21, 2023 01:50

melinath requested changes Jul 25, 2023

View reviewed changes

toVersus force-pushed the feat/k8s-beta-api branch from f12421f to 715c029 Compare July 26, 2023 06:44

modular-magician requested a review from melinath July 26, 2023 06:45

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 26, 2023

toVersus added 2 commits July 26, 2023 16:08

feat(google_container_cluster): support enable k8s beta apis

415edb4

Signed-off-by: toVersus <[email protected]> * use TypeSet to avoid the re-ordering being treated as a diff * add custom diff to recreate cluster when removing enabled apis * add unit and integration tests

fix: use contains instead of sets intersection

3ed46bb

toVersus force-pushed the feat/k8s-beta-api branch from 715c029 to 3ed46bb Compare July 26, 2023 07:09

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jul 26, 2023

melinath approved these changes Jul 27, 2023

View reviewed changes

mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown Outdated Show resolved Hide resolved

Update mmv1/third_party/terraform/website/docs/r/container_cluster.ht…

da83bb0

…ml.markdown

modular-magician requested a review from melinath July 27, 2023 18:22

modular-magician added awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Jul 27, 2023

melinath approved these changes Jul 27, 2023

View reviewed changes

melinath merged commit 27213b2 into GoogleCloudPlatform:main Jul 27, 2023
8 checks passed

This was referenced Jul 27, 2023

feat(google_container_cluster): support enable k8s beta apis hashicorp/terraform-provider-google-beta#5961

Merged

feat(google_container_cluster): support enable k8s beta apis hashicorp/terraform-provider-google#15320

Merged

toVersus deleted the feat/k8s-beta-api branch July 28, 2023 00:34

rileykarson mentioned this pull request Mar 27, 2024

Beta apis #8109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(google_container_cluster): support enable k8s beta apis #8355

feat(google_container_cluster): support enable k8s beta apis #8355

toVersus commented Jul 14, 2023

modular-magician commented Jul 14, 2023

melinath left a comment

modular-magician commented Jul 17, 2023

modular-magician commented Jul 17, 2023

modular-magician commented Jul 17, 2023

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

modular-magician commented Jul 18, 2023

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

modular-magician commented Jul 18, 2023

modular-magician commented Jul 18, 2023

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

melinath Jul 20, 2023

toVersus Jul 21, 2023

melinath left a comment

melinath Jul 25, 2023

toVersus Jul 26, 2023

coolkp Jul 26, 2023

modular-magician commented Jul 26, 2023

modular-magician commented Jul 26, 2023

modular-magician commented Jul 26, 2023

melinath left a comment

melinath left a comment

coolkp commented Jul 27, 2023

modular-magician commented Jul 27, 2023

modular-magician commented Jul 27, 2023

modular-magician commented Jul 27, 2023

toVersus commented Jul 28, 2023

feat(google_container_cluster): support enable k8s beta apis #8355

feat(google_container_cluster): support enable k8s beta apis #8355

Conversation

toVersus commented Jul 14, 2023

modular-magician commented Jul 14, 2023

melinath left a comment

Choose a reason for hiding this comment

modular-magician commented Jul 17, 2023

Diff report

modular-magician commented Jul 17, 2023

Tests analytics

Action taken

modular-magician commented Jul 17, 2023

Rerun these tests in REPLAYING mode to catch issues

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

modular-magician commented Jul 18, 2023

Diff report

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

modular-magician commented Jul 18, 2023

Tests analytics

Action taken

modular-magician commented Jul 18, 2023

Rerun these tests in REPLAYING mode to catch issues

coolkp commented Jul 18, 2023

melinath commented Jul 18, 2023

melinath Jul 20, 2023

Choose a reason for hiding this comment

toVersus Jul 21, 2023

Choose a reason for hiding this comment

melinath left a comment

Choose a reason for hiding this comment

melinath Jul 25, 2023

Choose a reason for hiding this comment

toVersus Jul 26, 2023

Choose a reason for hiding this comment

coolkp Jul 26, 2023

Choose a reason for hiding this comment

modular-magician commented Jul 26, 2023

Diff report

modular-magician commented Jul 26, 2023

Tests analytics

Action taken

modular-magician commented Jul 26, 2023

Rerun these tests in REPLAYING mode to catch issues

melinath left a comment

Choose a reason for hiding this comment

melinath left a comment

Choose a reason for hiding this comment

coolkp commented Jul 27, 2023

modular-magician commented Jul 27, 2023

Diff report

modular-magician commented Jul 27, 2023

Tests analytics

Action taken

modular-magician commented Jul 27, 2023

Rerun these tests in REPLAYING mode to catch issues

toVersus commented Jul 28, 2023