Allow passing node service_account when autopilot enabled #6733

JeremyOT · 2022-10-22T17:50:37Z

Removes the conflict between cluster_autoscaling and autopilot making it possible to supply a service_account:

cluster_autoscaling.auto_provisioning_defaults.service_account

This change also makes cluster_autoscaling.enabled optional to simplify passing args when autopilot is enabled.

As a result it is now possible to pass false when creating autopilot clusters but this will be caught by server side validaton.

fixes hashicorp/terraform-provider-google#9505

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).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
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).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

container: fixed a bug where `cluster_autoscaling.auto_provisioning_defaults.service_account` can not be set when `enable_autopilot = true` for `google_container_cluster`

modular-magician · 2022-10-22T17:51:54Z

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

I've detected that you're a community contributor. @shuyama1, 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.

modular-magician · 2022-10-22T18:07:27Z

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 ( 1 file changed, 14 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 1 file changed, 14 insertions(+), 7 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-22T18:59:09Z

Tests analytics

Total tests: 2203
Passed tests 1963
Skipped tests: 239
Failed tests: 1

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease

modular-magician · 2022-10-22T18:59:32Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

All tests passed
View the build log or the debug log for each test

modax · 2022-10-22T20:08:48Z

@JeremyOT

File changed - 2731 does not look right for this PR. You have included vendor/bundle/rubyin your PR, it does not look like it should be necessary.
Have you tested if setting non-default oauth_scopes under cluster_autoscaling.auto_provisioning_defaults works for Autopilot?
I believe you should tweak documentation regarding these Autopilot related settings. I believe an example in docs would be really helpful.
Last but not least, a new integration test for both service_account and oauth_scopes would really help to avoid regressions (e.g. a test for example from documentation)

modular-magician · 2022-10-23T00:26:53Z

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 ( 1 file changed, 14 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 1 file changed, 14 insertions(+), 7 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-23T01:09:55Z

Tests analytics

Total tests: 2203
Passed tests 1964
Skipped tests: 239
Failed tests: 0

All tests passed in REPLAYING mode
View the build log

JeremyOT · 2022-10-23T01:59:59Z

Accidentally caught the vendor dir with my amend alias - whoops.

Have you tested if setting non-default oauth_scopes under cluster_autoscaling.auto_provisioning_defaults works for Autopilot?

Yes, oauth_scopes may be passed though we recommend only using https://www.googleapis.com/auth/cloud-platform and controlling access with IAM instead.

modular-magician · 2022-10-25T18:10:19Z

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, 121 insertions(+), 39 deletions(-))
Terraform Beta: Diff ( 3 files changed, 123 insertions(+), 40 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-25T18:57:52Z

Tests analytics

Total tests: 2208
Passed tests 1967
Skipped tests: 239
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerClusterCustomServiceAccount_withAutopilot|TestAccContainerCluster_nodeAutoprovisioning

modular-magician · 2022-10-25T19:12:43Z

Tests failed during RECORDING mode:
TestAccContainerClusterCustomServiceAccount_withAutopilot[Error message] [Debug log]
TestAccContainerCluster_nodeAutoprovisioning[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-25T20:29:34Z

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, 121 insertions(+), 39 deletions(-))
Terraform Beta: Diff ( 3 files changed, 129 insertions(+), 45 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-25T21:17:24Z

Tests analytics

Total tests: 2208
Passed tests 1966
Skipped tests: 239
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerClusterCustomServiceAccount_withAutopilot|TestAccContainerCluster_nodeAutoprovisioning|TestAccComputeForwardingRule_update

modular-magician · 2022-10-25T21:32:47Z

Tests passed during RECORDING mode:
TestAccContainerClusterCustomServiceAccount_withAutopilot[Debug log]
TestAccComputeForwardingRule_update[Debug log]

Tests failed during RECORDING mode:
TestAccContainerCluster_nodeAutoprovisioning[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-26T14:54:24Z

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, 127 insertions(+), 39 deletions(-))
Terraform Beta: Diff ( 3 files changed, 135 insertions(+), 45 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-26T15:32:52Z

Tests analytics

Total tests: 2208
Passed tests 1966
Skipped tests: 239
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_nodeAutoprovisioning|TestAccContainerClusterCustomServiceAccount_withAutopilot

modular-magician · 2022-10-26T15:47:36Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccContainerClusterCustomServiceAccount_withAutopilot[Debug log]

Tests failed during RECORDING mode:
TestAccContainerCluster_nodeAutoprovisioning[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-26T18:56:58Z

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, 124 insertions(+), 39 deletions(-))
Terraform Beta: Diff ( 3 files changed, 132 insertions(+), 45 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-26T19:36:10Z

Tests analytics

Total tests: 2208
Passed tests 1966
Skipped tests: 239
Failed tests: 3

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_nodeAutoprovisioning|TestAccContainerClusterCustomServiceAccount_withAutopilot

modular-magician · 2022-10-26T19:51:09Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccContainerClusterCustomServiceAccount_withAutopilot[Debug log]

Tests failed during RECORDING mode:
TestAccContainerCluster_nodeAutoprovisioning[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-28T18:43:40Z

Tests analytics

Total tests: 2211
Passed tests 1971
Skipped tests: 239
Failed tests: 1

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerCluster_nodeAutoprovisioning

modular-magician · 2022-10-28T18:54:10Z

Tests failed during RECORDING mode:
TestAccContainerCluster_nodeAutoprovisioning[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-29T14:25:28Z

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, 122 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 3 files changed, 130 insertions(+), 44 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-29T15:04:50Z

Tests analytics

Total tests: 2211
Passed tests 1970
Skipped tests: 239
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerClusterCustomServiceAccount_withAutopilot

modular-magician · 2022-10-29T15:18:51Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccContainerClusterCustomServiceAccount_withAutopilot[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2022-10-29T20:33:27Z

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, 122 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 3 files changed, 130 insertions(+), 44 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-10-29T21:19:27Z

Tests analytics

Total tests: 2211
Passed tests 1971
Skipped tests: 239
Failed tests: 1

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease

modular-magician · 2022-10-29T21:19:53Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

All tests passed
View the build log or the debug log for each test

JeremyOT · 2022-10-30T17:33:29Z

@shuyama1 it looks like this one is good to go

shuyama1 · 2022-11-03T16:37:38Z

mmv1/third_party/terraform/resources/resource_container_cluster.go.erb

 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"enabled": {
-							Type:        schema.TypeBool,
-							Required:    true,
+							Type:          schema.TypeBool,


I understand that making this optional will simplify user experience, but allowing empty block cluster_autoscaling{ } to be specified usually causes more problem in Terraform. Therefore I would recommend that we keep this field as required.

I'm happy to make that change, but we're seeing a lot of friction with terraform's client side validation. It keeps causing unnecessary delays in support for new features. Especially when those features are ~support for existing fields (like this one).

shuyama1 · 2022-11-08T18:48:00Z

mmv1/third_party/terraform/resources/resource_container_cluster.go.erb

+	enableAutopilot := false
+	if v, ok := d.GetOk("enable_autopilot"); ok && v == true {
+		enableAutopilot = true
+	}
 	if !ok || l == nil || len(l) == 0 || l[0] == nil {
-		if v, ok := d.GetOk("enable_autopilot"); ok && v == true {
+		if enableAutopilot {


I am trying to understand the change here. Does this new change cover any cases that the previous code does not cover?

It's not a logical change, however it makes additional autopilot conditionals simpler in the future

shuyama1 · 2022-11-09T16:11:21Z

mmv1/third_party/terraform/resources/resource_container_cluster.go.erb

@@ -78,6 +78,13 @@ var (
 	forceNewClusterNodeConfigFields = []string{
 		"workload_metadata_config",
 	}
+
+	suppressDiffForAutopilot = schema.SchemaDiffSuppressFunc(func(k, oldValue, newValue string, d *schema.ResourceData) bool {


Just want to confirm if my understanding is correct. This DSF is added to prevent premadiff for some fields that return computed values when enable_autopilot == true. If so, do you think making the field optional + computed will solve the problem? Plus, I've seen some fields have default values. Does the API return different values. If they are fixed default values, having them set from the Terraform side should not cause any permadiffs.

The problem is that these fields have default values, which conflicts with optional/computed. We can't remove the default values, but they will be ignored for Autopilot and the result is a permadiff

Ah, like for autopilot, the API will not return those values?

It returns different values, or no values for different fields. The intent here is essentially the field is optional + computed only when enable_autopilot == true

Got it! Thanks! Then it sounds like that optional + computed make more sense for such fields than having default values on Terraform side.
For cases other than enable_autopilot == true, do these fields have the same server side defaults (plus the API also return these values) as the Terraform side ones. If so, I guess it would be relatively safe to change default values to optional + computed.

We don't have the same defaults when Autopilot is enabled, and these may change over time as these values are implementation details for Autopilot, whereas they matter directly in GKE Standard.

This is another side effect of the dual purpose cluster APIs, but when Autopilot is enabled we cannot make any assumptions about server side values as their future changes are not considered breaking

Discussed offline - The fields are required when Autopilot is not enabled. So changing to optional + computed will not work. DSF may be the best way to handle perma-diffs in this case.

Removes the conflict between cluster_autoscaling and autopilot making it possible to supply a service_account: `cluster_autoscaling.auto_provisioning_defaults.service_account` This change also makes `cluster_autoscaling.enabled` optional to simplify passing args when autopilot is enabled. As a result it is now possible to pass false when creating autopilot clusters but this will be caught by server side validaton. fixes hashicorp/terraform-provider-google#9505

modular-magician · 2022-11-11T20:40:47Z

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, 122 insertions(+), 38 deletions(-))
Terraform Beta: Diff ( 3 files changed, 130 insertions(+), 44 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-11-11T21:32:49Z

Tests analytics

Total tests: 2244
Passed tests 2001
Skipped tests: 241
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccContainerClusterCustomServiceAccount_withAutopilot|TestAccComputeForwardingRule_update

modular-magician · 2022-11-11T21:47:50Z

Tests passed during RECORDING mode:
TestAccContainerClusterCustomServiceAccount_withAutopilot[Debug log]
TestAccComputeForwardingRule_update[Debug log]

All tests passed
View the build log or the debug log for each test

shuyama1

Thanks!

…dPlatform#6733) fixes hashicorp/terraform-provider-google#9505

modular-magician requested a review from shuyama1 October 22, 2022 17:51

JeremyOT mentioned this pull request Oct 22, 2022

GKE autopilot is always created with default service account II hashicorp/terraform-provider-google#9505

Closed

modax mentioned this pull request Oct 22, 2022

Allow node_pool and enable_autopilot together for custom service account and oauth scopes #6611

Closed

5 tasks

JeremyOT force-pushed the napsa branch from 6b42a98 to 2d24ce6 Compare October 23, 2022 00:12

rileykarson mentioned this pull request Oct 24, 2022

Allow custom serviceAccount for GKE Autopilot clusters #6732

Closed

3 tasks

JeremyOT force-pushed the napsa branch from 2d24ce6 to 56827e4 Compare October 25, 2022 17:56

JeremyOT force-pushed the napsa branch from 56827e4 to bb7cf3f Compare October 25, 2022 20:14

JeremyOT force-pushed the napsa branch 2 times, most recently from 3750ed9 to 3fda6af Compare October 26, 2022 14:40

JeremyOT force-pushed the napsa branch from 3fda6af to 3878cac Compare October 26, 2022 18:42

JeremyOT force-pushed the napsa branch from 528eb0d to 067cca2 Compare October 29, 2022 14:09

JeremyOT force-pushed the napsa branch from 067cca2 to 73165a4 Compare October 29, 2022 20:17

JeremyOT marked this pull request as ready for review October 30, 2022 17:32

shuyama1 reviewed Nov 3, 2022

View reviewed changes

shuyama1 reviewed Nov 8, 2022

View reviewed changes

shuyama1 reviewed Nov 9, 2022

View reviewed changes

JeremyOT force-pushed the napsa branch from 73165a4 to fa2e1ad Compare November 11, 2022 20:24

modular-magician requested a review from shuyama1 November 11, 2022 20:25

shuyama1 approved these changes Nov 12, 2022

View reviewed changes

shuyama1 merged commit afa5b7f into GoogleCloudPlatform:main Nov 12, 2022

googlerjk pushed a commit to googlerjk/magic-modules that referenced this pull request Nov 22, 2022

Allow passing node service_account when autopilot enabled (GoogleClou…

7d47c7e

…dPlatform#6733) fixes hashicorp/terraform-provider-google#9505

googlerjk pushed a commit to googlerjk/magic-modules that referenced this pull request Nov 25, 2022

Allow passing node service_account when autopilot enabled (GoogleClou…

c7a0002

…dPlatform#6733) fixes hashicorp/terraform-provider-google#9505

hao-nan-li pushed a commit to hao-nan-li/magic-modules that referenced this pull request Dec 6, 2022

Allow passing node service_account when autopilot enabled (GoogleClou…

b0eebab

…dPlatform#6733) fixes hashicorp/terraform-provider-google#9505

rojomisin mentioned this pull request Feb 24, 2023

beta-autopilot-private-cluster uses default service account terraform-google-modules/terraform-google-kubernetes-engine#1488

Closed

Allow passing node service_account when autopilot enabled #6733

Allow passing node service_account when autopilot enabled #6733

Conversation

JeremyOT commented Oct 22, 2022 • edited by shuyama1

modular-magician commented Oct 22, 2022

modular-magician commented Oct 22, 2022

Diff report

modular-magician commented Oct 22, 2022

Tests analytics

Action taken

modular-magician commented Oct 22, 2022

modax commented Oct 22, 2022

modular-magician commented Oct 23, 2022

Diff report

modular-magician commented Oct 23, 2022

Tests analytics

JeremyOT commented Oct 23, 2022

modular-magician commented Oct 25, 2022

Diff report

modular-magician commented Oct 25, 2022

Tests analytics

Action taken

modular-magician commented Oct 25, 2022

modular-magician commented Oct 25, 2022

Diff report

modular-magician commented Oct 25, 2022

Tests analytics

Action taken

modular-magician commented Oct 25, 2022

modular-magician commented Oct 26, 2022

Diff report

modular-magician commented Oct 26, 2022

Tests analytics

Action taken

modular-magician commented Oct 26, 2022

modular-magician commented Oct 26, 2022

Diff report

modular-magician commented Oct 26, 2022

Tests analytics

Action taken

modular-magician commented Oct 26, 2022

modular-magician commented Oct 28, 2022

Tests analytics

Action taken

modular-magician commented Oct 28, 2022

modular-magician commented Oct 29, 2022

Diff report

modular-magician commented Oct 29, 2022

Tests analytics

Action taken

modular-magician commented Oct 29, 2022

modular-magician commented Oct 29, 2022

Diff report

modular-magician commented Oct 29, 2022

Tests analytics

Action taken

modular-magician commented Oct 29, 2022

JeremyOT commented Oct 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuyama1 Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuyama1 Nov 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Nov 11, 2022

Diff report

modular-magician commented Nov 11, 2022

Tests analytics

Action taken

modular-magician commented Nov 11, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

JeremyOT commented Oct 22, 2022 •

edited by shuyama1

shuyama1 Nov 9, 2022 •

edited

shuyama1 Nov 10, 2022 •

edited