container: fix updates for node_config.gcfs_config and make optional

[wyardley]

Recent changes introduced a couple of issues with node_config.gcfs_config:

• Perrmadiff when the cluster was created with an older version of the provider (and in the case of older provider versions, force replacement of the nodepool) because of the attribute being required and not computed.
• Lack of support for updating the field in place once initially set. While having force replacement of the node pool as existed before feat: Remove force replacement from gcfs_config #11553 was one way to accomplish this, that fix didn't include the bits to update the node pool in place.

• Handle updates properly for node_config.gcfs_config
• Make node_config.gcfs_config optional and computed
• Add tests to make sure we test the case where it's defined in google_container_cluster.node_config, as well as testing updates for both that and the google_container_node_pool resource.
• Some minor fixes to spacing / formatting (to fix incorrect hard / soft tabs)

Fixes hashicorp/terraform-provider-google#19482
Follow up to #11553

Possible fix for terraform-google-modules/terraform-google-kubernetes-engine#2100

Let me know if there are potential negative effects from having the only item in the block be optional and computed. I have tested creating a cluster with an empty node_config.gcfs_config block, as well as later enabling it from that state, so I don't think having it be optional and the only item within that nested block will be an issue.

Might need to do a custom expand function or some more complicated code for the update case if more elements are added to the subblock, but this seems to work in my testing.

Release Note Template for Downstream PRs (will be copied)

container: fixed the in-place update for `node_config.gcfs_config` in `google_container_cluster` and `google_container_node_pool`

container: fixed a permadiff on `node_config.gcfs_config` in `google_container_cluster` and `google_container_node_pool`

[github-actions]

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

@shuyama1, 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.

[wyardley]

heads up to @dominykasn and @hao-nan-li re: earlier comments in #11553

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

Missing test report

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

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

resource "google_container_cluster" "primary" {
  node_pool {
    node_config {
      gcfs_config {
        enabled = # value needed
      }
    }
  }
}

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 195
Skipped tests: 13
Affected tests: 2

Click here to see the affected service packages

• container

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

• TestAccContainerCluster_withNodeConfigGcfsConfig
• TestAccContainerNodePool_gcfsConfig

Get to know how VCR tests work

[wyardley]

Please add an acceptance test which includes these fields. The test should include the following:

I can add this if really needed, though the fact that it's tested already in both google_container_node_pool and google_container_cluster default pool is probably sufficient.

Edit: That said, the node_pool block actually seems to just force replacement. So it shouldn't have update issues, though this is obviously not ideal.

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withNodeConfigGcfsConfig[Debug log]
TestAccContainerNodePool_gcfsConfig[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

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

Missing test report

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

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

resource "google_container_cluster" "primary" {
  node_pool {
    node_config {
      gcfs_config {
        enabled = # value needed
      }
    }
  }
}

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

Missing test report

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

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

resource "google_container_cluster" "primary" {
  node_pool {
    node_config {
      gcfs_config {
        enabled = # value needed
      }
    }
  }
}

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

Missing test report

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

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

resource "google_container_cluster" "primary" {
  node_pool {
    node_config {
      gcfs_config {
        enabled = # value needed
      }
    }
  }
}

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 197
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

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 197
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

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 197
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

[shuyama1]

I'm inclined to make the block optional + computed while keep the subfield enabled required to prevent users from sending empty blocks gcfs_config{}. WDYT?

[wyardley]

I can give it a shot and see how the tests (and functional testing) look; I'm Ok with it as long as everything still works.

That said, from what I understand, empty blocks are legal Terraform (and we've seen cases, like in terraform-google-modules/terraform-google-kubernetes-engine where there's a lot of templating, dynamic blocks, etc that this sometimes is a thing)? See for example the sample code in hashicorp/terraform-provider-google#19428 with an empty node_pool_defaults.node_config_defaults block... I think the requirement to have at least one field in a nested block used to be a common practice, but that there are now better safeguards? I doubt other fields would be added to this block, but #11572 is an example of a field that was defined as required more or less arbitrarily at one point in the past, and ended up causing issues down the line.

FWIW, in the functional testing I did, I'm pretty sure I tested the behavior with that block empty and other scenarios, and didn't see any issues with it, though of course it's possible I missed something.

You would probably know better, but what effect does making a top level field Computed do exactly, since it doesn't have any value of its own? Does it just get inherited by everything underneath?

[wyardley]

Basically like this, is what you're thinking?

diff --git a/mmv1/third_party/terraform/services/container/node_config.go.erb b/mmv1/third_party/terraform/services/container/node_config.go.erb
index 94e1e04c0..df45e8d50 100644
--- a/mmv1/third_party/terraform/services/container/node_config.go.erb
+++ b/mmv1/third_party/terraform/services/container/node_config.go.erb
@@ -104,14 +104,14 @@ func schemaGcfsConfig() *schema.Schema {
 	return &schema.Schema{
 		Type:        schema.TypeList,
 		Optional:    true,
+		Computed:    true,
 		MaxItems:    1,
 		Description: `GCFS configuration for this node.`,
 		Elem:        &schema.Resource{
 			Schema: map[string]*schema.Schema{
 				"enabled": {
 					Type:        schema.TypeBool,
-					Optional:    true,
-					Computed:    true,
+					Required:    true,
 					Description: `Whether or not GCFS is enabled`,
 				},
 			},
diff --git a/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown b/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown
index bf4ab7772..df2af254e 100644
--- a/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown
+++ b/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown
@@ -1038,7 +1038,7 @@ sole_tenant_config {
 
 <a name="nested_gcfs_config"></a>The `gcfs_config` block supports:
 
-* `enabled` (Optional) - Whether or not the Google Container Filesystem (GCFS) is enabled
+* `enabled` (Required) - Whether or not the Google Container Filesystem (GCFS) is enabled
 
 <a name="nested_gvnic"></a>The `gvnic` block supports:

[wyardley]

I can validate that at a basic level, it has the effect of preventing enabled from being skipped. I can also create the cluster without it set, add it later, and even remove the block after applying (in which case it's a noop -- terraform will get the existing computed value but won't try to set it)

I'll complete running the tests locally, and push up this change as a new commit if it passes, though would appreciate if you're able to expedite any additional tests / review on your end if we go this way (esp. since I've got another change in the same area of the codebase that may end up having some conflicts when one or the other goes in)

│ Error: Missing required argument
│ 
│   on cluster.tf line 7, in resource "google_container_cluster" "test_gcfs_config":
│    7:     gcfs_config {
│ 
│ The argument "enabled" is required, but no definition was found.

Tests pass, at least for TestAccContainerCluster_withNodeConfigGcfsConfig and TestAccContainerNodePool_gcfsConfig:

--- PASS: TestAccContainerCluster_withNodeConfigGcfsConfig (931.54s)
PASS
ok  	github.com/hashicorp/terraform-provider-google/google/services/container	932.504s

--- PASS: TestAccContainerNodePool_gcfsConfig (1003.20s)
PASS
ok  	github.com/hashicorp/terraform-provider-google/google/services/container	1004.177s

[wyardley]

Ok - I pushed that update; it wouldn't be too hard to adjust later if it turns out it does need to be optional after all.

We can revert 3b6d99f if it turns out the other behavior is preferred after all.

[wyardley]

@shuyama1 assuming this probably won't qualify to get cherry-picked into 5.xx, right (though guessing it would solve some problems if it could be).

[wyardley]

Also, feel free to adjust the release notes as needed. I have it as made node_config.gcfs_config optional, which is still technically true.

[shuyama1]

@shuyama1 assuming this probably won't qualify to get cherry-picked into 5.xx, right (though guessing it would solve some problems if it could be).

I'll discuss with my team. But it may not be needed(?), given hashicorp/terraform-provider-google#19365 is only released in 6.x and was not backported to 5.x

[wyardley]

But it may not be needed(?), given hashicorp/terraform-provider-google#19365 is only released in 6.x and was not backported to 5.x

IIRC, I ran into some similar problems (problems that #19365 was intended to fix) when I was testing something else with the older provider, but could be wrong.

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

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 197
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

[shuyama1]

/gcbrun

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

[modular-magician]

Tests analytics

Total tests: 210
Passed tests: 195
Skipped tests: 13
Affected tests: 2

Click here to see the affected service packages

• container

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

• TestAccContainerCluster_withNodeConfigGcfsConfig
• TestAccContainerNodePool_gcfsConfig

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withNodeConfigGcfsConfig[Debug log]
TestAccContainerNodePool_gcfsConfig[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