fix: allow null enable_gcfs setting in defined nodepools

[wyardley]

Even with 6.4.0 or 5.44.1 (with my upstream fix), I am still seeing the issue described in #2100, since the earlier fixes were related to the default node-pool vs explicitly defined ones.

Fixes #2100
This basically replicates the fixes from #2093, #2095, but at the scope of implicitly defined nodepools.

[apeabody]

/gcbrun

[apeabody]

Thanks @wyardley!

Do you know if we test this in any of the examples?

[wyardley]

Do you know if we test this in any of the examples?

terraform-google-kubernetes-engine/examples/node_pool/main.tf

Line 54 in 0733908

enable_gcfs = false

We test the case where it's false here; since the other node pools in this example don't have it defined, I assume that implicitly tests the null case, though it's interesting - if the test is already checking for idempotence (is it?), you'd think it would have already caught the diff in plan already? Maybe because in the case where we're creating it and the provider creates it as false, it stays that way, but just speculating.

Is the failure in CI from one of these changes, or a spurious / transient one?

[apeabody]

I believe this change is intentional, so just need to update the test data to match.

https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/test/integration/private_zonal_with_networking/testdata/TestPrivateZonalWithNetworking.json#L213

Step #98 - "verify private-zonal-with-networking":         	Error Trace:	/workspace/test/integration/private_zonal_with_networking/private_zonal_with_networking_test.go:81
Step #98 - "verify private-zonal-with-networking":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:638
Step #98 - "verify private-zonal-with-networking":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #98 - "verify private-zonal-with-networking":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
Step #98 - "verify private-zonal-with-networking":         	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:670
Step #98 - "verify private-zonal-with-networking":         	Error:      	Not equal: 
Step #98 - "verify private-zonal-with-networking":         	            	expected: map[string]interface {}{"diskSizeGb":100, "diskType":"pd-standard", "gcfsConfig":map[string]interface {}{}, "imageType":"COS_CONTAINERD", "labels":map[string]interface {}{"cluster_name":"CLUSTER_NAME", "node_pool":"default-node-pool"}, "loggingConfig":map[string]interface {}{"variantConfig":map[string]interface {}{"variant":"DEFAULT"}}, "machineType":"e2-medium", "metadata":map[string]interface {}{"cluster_name":"CLUSTER_NAME", "disable-legacy-endpoints":"true", "node_pool":"default-node-pool"}, "oauthScopes":[]interface {}{"https://www.googleapis.com/auth/cloud-platform"}, "serviceAccount":"SERVICE_ACCOUNT", "shieldedInstanceConfig":map[string]interface {}{"enableIntegrityMonitoring":true}, "tags":[]interface {}{"gke-CLUSTER_NAME", "gke-CLUSTER_NAME-default-node-pool"}, "windowsNodeConfig":map[string]interface {}{}, "workloadMetadataConfig":map[string]interface {}{"mode":"GKE_METADATA"}}
Step #98 - "verify private-zonal-with-networking":         	            	actual  : map[string]interface {}{"diskSizeGb":100, "diskType":"pd-standard", "imageType":"COS_CONTAINERD", "labels":map[string]interface {}{"cluster_name":"CLUSTER_NAME", "node_pool":"default-node-pool"}, "loggingConfig":map[string]interface {}{"variantConfig":map[string]interface {}{"variant":"DEFAULT"}}, "machineType":"e2-medium", "metadata":map[string]interface {}{"cluster_name":"CLUSTER_NAME", "disable-legacy-endpoints":"true", "node_pool":"default-node-pool"}, "oauthScopes":[]interface {}{"https://www.googleapis.com/auth/cloud-platform"}, "serviceAccount":"SERVICE_ACCOUNT", "shieldedInstanceConfig":map[string]interface {}{"enableIntegrityMonitoring":true}, "tags":[]interface {}{"gke-CLUSTER_NAME", "gke-CLUSTER_NAME-default-node-pool"}, "windowsNodeConfig":map[string]interface {}{}, "workloadMetadataConfig":map[string]interface {}{"mode":"GKE_METADATA"}}
Step #98 - "verify private-zonal-with-networking":         	            	
Step #98 - "verify private-zonal-with-networking":         	            	Diff:
Step #98 - "verify private-zonal-with-networking":         	            	--- Expected
Step #98 - "verify private-zonal-with-networking":         	            	+++ Actual
Step #98 - "verify private-zonal-with-networking":         	            	@@ -1,6 +1,4 @@
Step #98 - "verify private-zonal-with-networking":         	            	-(map[string]interface {}) (len=14) {
Step #98 - "verify private-zonal-with-networking":         	            	+(map[string]interface {}) (len=13) {
Step #98 - "verify private-zonal-with-networking":         	            	  (string) (len=10) "diskSizeGb": (float64) 100,
Step #98 - "verify private-zonal-with-networking":         	            	  (string) (len=8) "diskType": (string) (len=11) "pd-standard",
Step #98 - "verify private-zonal-with-networking":         	            	- (string) (len=10) "gcfsConfig": (map[string]interface {}) {
Step #98 - "verify private-zonal-with-networking":         	            	- },
Step #98 - "verify private-zonal-with-networking":         	            	  (string) (len=9) "imageType": (string) (len=14) "COS_CONTAINERD",
Step #98 - "verify private-zonal-with-networking":         	Test:       	TestPrivateZonalWithNetworking

[apeabody]

Do you know if we test this in any of the examples?

terraform-google-kubernetes-engine/examples/node_pool/main.tf

Line 54 in 0733908

enable_gcfs = false

We test the case where it's false here; since the other node pools in this example don't have it defined, I assume that implicitly tests the null case, though it's interesting - if the test is already checking for idempotence (is it?), you'd think it would have already caught the diff in plan already? Maybe because in the case where we're creating it and the provider creates it as false, it stays that way, but just speculating.

Is the failure in CI from one of these changes, or a spurious / transient one?

It looks like most of the idempotence tests are disabled(e.g. https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/test/integration/private_zonal_with_networking/private_zonal_with_networking_test.go#L36), so I actually started work to bring back in: #2060

[wyardley]

so just need to update the test data to match.

Just to confirm, you're suggesting removing that line, right? and that's the only one where it's failing? I did a quick search for gcfsConfig in other testdata directories, and mostly turned up autopilot stuff.

[apeabody]

so just need to update the test data to match.

Just to confirm, you're suggesting removing that line, right? and that's the only one where it's failing? I did a quick search for gcfsConfig in other testdata directories, and mostly turned up autopilot stuff.

That's as far as the tests got in that run, but yes, that particular test doesn't define, so with this change the gcfsConfig block should not be created.

[wyardley]

Cool. I didn’t see another from a quick look, and removed it in that last commit.

[apeabody]

/gcbrun

[apeabody]

Thanks for the contribution @wyardley!