fix: add target tags to `node_pool_auto_config` for standard clusters #2118

wyardley · 2024-09-26T03:53:53Z

While #1817 added autopilot support for adding tags to node_pool_auto_config when add_cluster_firewall_rules is set to true, the same change did not apply for standard (non-autopilot) clusters with cluster level autoscaling (nodepool autoprovisioning) in place,

Fixes #2104

wyardley

@apeabody I'm not sure this example is one you'd want in examples -- I think I saw something about that; in that case, do I just fold the whole example into fixtures? Or should I just add more comments or adjust the sample code slightly?

apeabody · 2024-09-26T16:26:06Z

/gcbrun

apeabody · 2024-09-26T23:54:44Z

@apeabody I'm not sure this example is one you'd want in examples -- I think I saw something about that; in that case, do I just fold the whole example into fixtures? Or should I just add more comments or adjust the sample code slightly?

Hi @wyardley! I think it's fine to add a relevant example, although we have to be careful with the concurrent test resource usage (might need to also waitFor a subsequent step).

While this repo has a cft test run all --stage init, it doesn't for the subsequent states, so you will need to add something like this:

build/int.cloudbuild.yaml

- id: apply simple-regional-cluster-autoscaling
  waitFor:
    - init-all
    - create-all
  name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS'
  args: ['/bin/bash', '-c', 'cft test run TestSimpleRegionalClusterAutoscaling --stage apply --verbose']
- id: verify simple-regional-cluster-autoscaling
  waitFor:
    - apply simple-regional-cluster-autoscaling
  name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS'
  args: ['/bin/bash', '-c', 'cft test run TestSimpleRegionalClusterAutoscaling --stage verify --verbose']
- id: destroy simple-regional-cluster-autoscaling
  waitFor:
    - verify simple-regional-cluster-autoscaling
  name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS'
  args: ['/bin/bash', '-c', 'cft test run TestSimpleRegionalClusterAutoscaling --stage teardown --verbose']

wyardley · 2024-09-27T00:17:04Z

Hi @wyardley! I think it's fine to add a relevant example,

Yeah, I guess seemed a little niche of a use case; obviously someone copying that one without thinking about what it's doing could end up opening up network holes unnecessarily? And not sure if we should adjust the wording or comments anywhere, or make a more descriptive directory name?

I also picked simple regional as the base for this more or less arbitrarily.

I'll add in the snippet you suggested for the Cloudbuild config (should maybe remove the lint one at some point?) and we can see how it goes.

If the fixture with the expected content is too far off base to adjust, I will probably need your help to figure out how that should be updated / adjusted as well.

apeabody · 2024-09-27T16:37:11Z

/gcbrun

apeabody · 2024-09-27T16:38:57Z

I'll add in the snippet you suggested for the Cloudbuild config (should maybe remove the lint one at some point?) and we can see how it goes.

Sure: #2120

apeabody · 2024-09-27T17:27:34Z

Looks like this will need to also wait on a longer destroy for resource availability.

Error: Error waiting to create Network: Error waiting for Creating Network: Quota 'NETWORKS' exceeded.  Limit: 15.0 globally.
	metric name = compute.googleapis.com/networks
	limit name = NETWORKS-per-project
	limit = 15
	dimensions = map[global:global]


  with google_compute_network.main,
  on network.tf line 27, in resource "google_compute_network" "main":
  27: resource "google_compute_network" "main" {

wyardley · 2024-09-27T17:37:04Z

Looks like this will need to also wait on a longer destroy for resource availability.

Are you suggesting to bump the retry interval in tft.WithRetryableTerraformErrors(), or to override the timeout for the cloudrun job's step?

apeabody · 2024-09-27T17:58:42Z

Looks like this will need to also wait on a longer destroy for resource availability.

Are you suggesting to bump the retry interval in tft.WithRetryableTerraformErrors(), or to override the timeout for the cloudrun job's step?

Sorry - Typo, that should be "later", not "longer. Like this example: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/build/int.cloudbuild.yaml#L458

wyardley · 2024-09-27T18:18:25Z

Sorry - Typo, that should be "later", not "longer. Like this example

Since there's no explicit init step for this one, add it to the apply, right? Is 336cbff what you had in mind?

apeabody · 2024-09-27T18:21:27Z

Sorry - Typo, that should be "later", not "longer. Like this example

Since there's no explicit init step for this one, add it to the apply, right? Is 336cbff what you had in mind?

Yeah - As there is now an init all, we shouldn't need the per test inits, and ideally can be removed at some point.

Looks good, let's see how the tests go:

/gcbrun

apeabody · 2024-09-27T19:59:24Z

FYI: Any step you depend on has to be "earlier" in the cloudbuild file.

invalid build: invalid .steps field: build step https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/issues/24 - "apply simple-regional-cluster-autoscaling" depends on "destroy simple-autopilot-private-local", which has not been defined

apeabody · 2024-09-27T21:59:04Z

/gcbrun

apeabody · 2024-10-01T17:21:42Z

/gcbrun

wyardley · 2024-10-01T17:54:38Z

Are we getting closer to a real failure yet? I can try to do the local setup if needed.

apeabody · 2024-10-01T18:25:23Z

Are we getting closer to a real failure yet? I can try to do the local setup if needed.

I don't think so, there is an API change which is breaking the test data as of this morning. Will re-run this once that is updated.

apeabody · 2024-10-03T15:55:31Z

/gcbrun

apeabody · 2024-10-04T17:56:37Z

/gcbrun

apeabody · 2024-10-08T16:30:30Z

/gcbrun

apeabody · 2024-10-09T15:39:57Z

/gcbrun

wyardley · 2024-10-09T15:53:41Z

@apeabody this should almost definitely still be failing at least partially because of the roughed in / bad fixture, right?
Is there an easy way to collect / create the values that should be in there without setting up the whole integration test setup locally?

apeabody · 2024-10-09T15:58:32Z

@apeabody this should almost definitely still be failing at least partially because of the roughed in / bad fixture, right? Is there an easy way to collect / create the values that should be in there without setting up the whole integration test setup locally?

Hi @wyardley! I didn't have a chance to review. In theory you could use the same command that is used in the verify to gather the details from a "model" project/cluster, but the formatting can be tricky, so this might be simpler:

Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:22:58Z command.go:185: "cloud-foundation-cicd"
Step #25 - "verify simple-regional-cluster-autoscaling":     golden.go:157: 
Step #25 - "verify simple-regional-cluster-autoscaling":         	Error Trace:	/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/golden/golden.go:157
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/workspace/test/integration/simple_regional_cluster_autoscaling/simple_regional_cluster_autoscaling_test.go:59
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #25 - "verify simple-regional-cluster-autoscaling":         	Error:      	Not equal: 
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	expected: "{\n    \"configConnectorConfig\": {},\n    \"dnsCacheConfig\": {},\n    \"gcePersistentDiskCsiDriverConfig\": {\n      \"enabled\": true\n    },\n    \"gcpFilestoreCsiDriverConfig\": {},\n    \"gcsFuseCsiDriverConfig\": {\n      \"enabled\": true\n    },\n    \"gkeBackupAgentConfig\": {},\n    \"horizontalPodAutoscaling\": {},\n    \"httpLoadBalancing\": {},\n    \"kubernetesDashboard\": {\n      \"disabled\": true\n    },\n    \"networkPolicyConfig\": {\n      \"disabled\": true\n    },\n    \"statefulHaConfig\": {\n      \"enabled\": true\n    }\n  }"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	actual  : "{\n    \"configConnectorConfig\": {},\n    \"dnsCacheConfig\": {},\n    \"gcePersistentDiskCsiDriverConfig\": {\n      \"enabled\": true\n    },\n    \"gcpFilestoreCsiDriverConfig\": {},\n    \"gkeBackupAgentConfig\": {},\n    \"horizontalPodAutoscaling\": {},\n    \"httpLoadBalancing\": {},\n    \"kubernetesDashboard\": {\n      \"disabled\": true\n    },\n    \"networkPolicyConfig\": {\n      \"disabled\": true\n    }\n  }"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	Diff:
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	--- Expected
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	+++ Actual
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	@@ -7,5 +7,2 @@
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	     "gcpFilestoreCsiDriverConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-    "gcsFuseCsiDriverConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-      "enabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	     "gkeBackupAgentConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	@@ -18,5 +15,2 @@
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	       "disabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-    "statefulHaConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-      "enabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	     }
Step #25 - "verify simple-regional-cluster-autoscaling":         	Test:       	TestSimpleRegionalClusterAutoscaling
Step #25 - "verify simple-regional-cluster-autoscaling":         	Messages:   	expected addonsConfig to match fixture {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "configConnectorConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "dnsCacheConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "gcePersistentDiskCsiDriverConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	      "enabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "gcpFilestoreCsiDriverConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "gcsFuseCsiDriverConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	      "enabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "gkeBackupAgentConfig": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "horizontalPodAutoscaling": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "httpLoadBalancing": {},
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "kubernetesDashboard": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	      "disabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "networkPolicyConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	      "disabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    },
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "statefulHaConfig": {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	      "enabled": true
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    }
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	  }
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:22:59Z command.go:100: Running command gcloud with args [config get-value project --format json]
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:22:59Z command.go:185: "cloud-foundation-cicd"
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:22:59Z command.go:100: Running command gcloud with args [config get-value project --format json]
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:23:00Z command.go:185: "cloud-foundation-cicd"
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:23:00Z command.go:100: Running command gcloud with args [config get-value project --format json]
Step #25 - "verify simple-regional-cluster-autoscaling": TestSimpleRegionalClusterAutoscaling 2024-10-08T17:23:01Z command.go:185: "cloud-foundation-cicd"
Step #25 - "verify simple-regional-cluster-autoscaling":     golden.go:157: 
Step #25 - "verify simple-regional-cluster-autoscaling":         	Error Trace:	/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/golden/golden.go:157
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/workspace/test/integration/simple_regional_cluster_autoscaling/simple_regional_cluster_autoscaling_test.go:59
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
Step #25 - "verify simple-regional-cluster-autoscaling":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #25 - "verify simple-regional-cluster-autoscaling":         	Error:      	Not equal: 
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	expected: "{\n    \"evaluationMode\": \"PROJECT_SINGLETON_POLICY_ENFORCE\"\n  }"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	actual  : "{}"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	Diff:
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	--- Expected
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	+++ Actual
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	@@ -1,3 +1 @@
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-{
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-    "evaluationMode": "PROJECT_SINGLETON_POLICY_ENFORCE"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	-  }
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	+{}
Step #25 - "verify simple-regional-cluster-autoscaling":         	Test:       	TestSimpleRegionalClusterAutoscaling
Step #25 - "verify simple-regional-cluster-autoscaling":         	Messages:   	expected binaryAuthorization to match fixture {
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	    "evaluationMode": "PROJECT_SINGLETON_POLICY_ENFORCE"
Step #25 - "verify simple-regional-cluster-autoscaling":         	            	  }

While terraform-google-modules#1817 added autopilot support for adding tags to `node_pool_auto_config` when `add_cluster_firewall_rules` is set to `true`, the same change did not apply for standard (non-autopilot) clusters with cluster level autoscaling (nodepool autoprovisioning) in place, Fixes terraform-google-modules#2104 Signed-off-by: William Yardley <[email protected]>

apeabody · 2024-10-09T17:39:12Z

/gcbrun

wyardley · 2024-10-09T19:33:49Z

Sorry, can you check if I missed something and / or anything else needs to be adjusted?

apeabody · 2024-10-09T19:34:36Z

/gcbrun

apeabody · 2024-10-09T20:54:12Z

Sorry, can you check if I missed something and / or anything else needs to be adjusted?

Looks good!

wyardley · 2024-10-09T20:55:43Z

Ah, ok guess that last failure was another spurious one 🤷

thanks, glad this one is passing

apeabody

Thanks for the contribution @wyardley!

wyardley commented Sep 26, 2024

View reviewed changes

wyardley force-pushed the wyardley/fix_2104 branch 2 times, most recently from 1c97eef to 5882a43 Compare September 26, 2024 04:30

wyardley force-pushed the wyardley/fix_2104 branch from fc75afc to 200550d Compare September 27, 2024 00:17

wyardley force-pushed the wyardley/fix_2104 branch from 812e4d0 to 336cbff Compare September 27, 2024 18:18

wyardley force-pushed the wyardley/fix_2104 branch from da4d74d to 9eac6e7 Compare September 27, 2024 20:04

wyardley added 3 commits October 9, 2024 08:59

add cloudbuild config

2b8dcdb

add waitfor in cloudbuild apply step

e73b227

wyardley added 3 commits October 9, 2024 09:00

add defaultverify back

f2c9fe2

depend on previous job destroy step vs. autopilot one

537355c

update expectations

0b97459

wyardley force-pushed the wyardley/fix_2104 branch from 966579e to 0b97459 Compare October 9, 2024 16:04

wyardley marked this pull request as ready for review October 9, 2024 17:37

wyardley requested review from ericyz, gtsorbo and a team as code owners October 9, 2024 17:37

Merge branch 'master' into wyardley/fix_2104

8b54ae1

apeabody approved these changes Oct 9, 2024

View reviewed changes

apeabody merged commit d5f66e8 into terraform-google-modules:master Oct 9, 2024
4 checks passed

release-please bot mentioned this pull request Oct 9, 2024

chore(master): release 34.0.0 #2133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add target tags to `node_pool_auto_config` for standard clusters #2118

fix: add target tags to `node_pool_auto_config` for standard clusters #2118

wyardley commented Sep 26, 2024

wyardley left a comment

apeabody commented Sep 26, 2024

apeabody commented Sep 26, 2024

wyardley commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

wyardley commented Sep 27, 2024

apeabody commented Sep 27, 2024

wyardley commented Sep 27, 2024 •

edited

Loading

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Oct 1, 2024

wyardley commented Oct 1, 2024

apeabody commented Oct 1, 2024

apeabody commented Oct 3, 2024

apeabody commented Oct 4, 2024

apeabody commented Oct 8, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody commented Oct 9, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody commented Oct 9, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody left a comment

fix: add target tags to node_pool_auto_config for standard clusters #2118

fix: add target tags to node_pool_auto_config for standard clusters #2118

Conversation

wyardley commented Sep 26, 2024

wyardley left a comment

Choose a reason for hiding this comment

apeabody commented Sep 26, 2024

apeabody commented Sep 26, 2024

wyardley commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

wyardley commented Sep 27, 2024

apeabody commented Sep 27, 2024

wyardley commented Sep 27, 2024 • edited Loading

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Sep 27, 2024

apeabody commented Oct 1, 2024

wyardley commented Oct 1, 2024

apeabody commented Oct 1, 2024

apeabody commented Oct 3, 2024

apeabody commented Oct 4, 2024

apeabody commented Oct 8, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody commented Oct 9, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody commented Oct 9, 2024

apeabody commented Oct 9, 2024

wyardley commented Oct 9, 2024

apeabody left a comment

Choose a reason for hiding this comment

fix: add target tags to `node_pool_auto_config` for standard clusters #2118

fix: add target tags to `node_pool_auto_config` for standard clusters #2118

wyardley commented Sep 27, 2024 •

edited

Loading