Autopilot Suppressed Diff Prevents using additive_vpc_scope_dns_domain

[EmileHofsink]

Currently when creating an Autopilot Cluster in Terraform it is not possible to use additive_vpc_scope_dns_domain within dns_config, while this feature is supported by the console and API currently. This looks like it is due to the suppressed diff configuration for Autopilot.

To adjust for this I have moved the suppressed diff configuration from the top level dns_config block to just apply to the elements that are unchangeable for Autopilot.

Release Note Template for Downstream PRs (will be copied)

container: added `additive_vpc_scope_dns_domain` field to `google_container_cluster` resource by removing suppressed diff

[github-actions]

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

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

[maci0]

we had a conversation regarding this in this PR also: #11562

[maci0]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # google_container_cluster.default will be updated in-place
  ~ resource "google_container_cluster" "default" {
        id                                       = "projects/enablement-404602/locations/us-central1/clusters/autopilot"
        name                                     = "autopilot"
        # (35 unchanged attributes hidden)

      ~ dns_config {
          ~ additive_vpc_scope_dns_domain = "foo.bar" -> "foo.baz"
            # (3 unchanged attributes hidden)
        }

        # (31 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

google_container_cluster.default: Modifying... [id=projects/enablement-404602/locations/us-central1/clusters/autopilot]
╷
│ Error: googleapi: Error 400: Changing DNSConfig is not supported in Autopilot clusters.
│ Details:
│ [
│   {
│     "@type": "type.googleapis.com/google.rpc.RequestInfo",
│     "requestId": "0xc3f07455c848394d"
│   }
│ ]
│ , badRequest
│ 
│   with google_container_cluster.default,
│   on main.tf line 15, in resource "google_container_cluster" "default":
│   15: resource "google_container_cluster" "default" {
│ 

[EmileHofsink]

Thanks for taking a look @maci0 ! Previous discussion looks good and all makes sense. What I've done (and hopefully haven't overstepped anything here) is keep my previous change of moving the other settings to be suppressed except for additive_vpc_dns_scope_domain, but, have now included a component of containerClusterAutopilotCustomizeDiff that checks to see if the cluster is an autopilot and if additive_vpc_dns_scope_domain has changed and if so request recreation of the cluster.

[github-actions]

@rileykarson This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[maci0]

@slevenick could you take a look at this one?

[rileykarson]

Running tests- I think this change looks reasonable, will LGTM+merge after success.

[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 ( 1 file changed, 25 insertions(+), 18 deletions(-))
google-beta provider: Diff ( 1 file changed, 25 insertions(+), 18 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

• Field dns_config lost its diff suppress function - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

[modular-magician]

Tests analytics

Total tests: 212
Passed tests: 185
Skipped tests: 13
Affected tests: 14

Click here to see the affected service packages

• container

Action taken

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

• TestAccContainerClusterCustomServiceAccount_withAutopilot
• TestAccContainerCluster_autopilot_minimal
• TestAccContainerCluster_autopilot_net_admin
• TestAccContainerCluster_enableCiliumPolicies_withAutopilot
• TestAccContainerCluster_withAutopilot
• TestAccContainerCluster_withAutopilotGcpFilestoreCsiDriver
• TestAccContainerCluster_withAutopilotKubeletConfig
• TestAccContainerCluster_withAutopilotNetworkTags
• TestAccContainerCluster_withAutopilotResourceManagerTags
• TestAccContainerCluster_withAutopilot_withNodePoolDefaults
• TestAccContainerCluster_withBinaryAuthorizationEvaluationModeAutopilot
• TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock_withAutopilot
• TestAccContainerCluster_withWorkloadALTSConfigAutopilot
• TestAccContainerCluster_withWorkloadIdentityConfigAutopilot

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerClusterCustomServiceAccount_withAutopilot[Error message] [Debug log]
TestAccContainerCluster_autopilot_minimal[Error message] [Debug log]
TestAccContainerCluster_autopilot_net_admin[Error message] [Debug log]
TestAccContainerCluster_enableCiliumPolicies_withAutopilot[Error message] [Debug log]
TestAccContainerCluster_withAutopilot[Error message] [Debug log]
TestAccContainerCluster_withAutopilotGcpFilestoreCsiDriver[Error message] [Debug log]
TestAccContainerCluster_withAutopilotKubeletConfig[Error message] [Debug log]
TestAccContainerCluster_withAutopilotNetworkTags[Error message] [Debug log]
TestAccContainerCluster_withAutopilotResourceManagerTags[Error message] [Debug log]
TestAccContainerCluster_withAutopilot_withNodePoolDefaults[Error message] [Debug log]
TestAccContainerCluster_withBinaryAuthorizationEvaluationModeAutopilot[Error message] [Debug log]
TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock_withAutopilot[Error message] [Debug log]
TestAccContainerCluster_withWorkloadALTSConfigAutopilot[Error message] [Debug log]
TestAccContainerCluster_withWorkloadIdentityConfigAutopilot[Error message] [Debug log]

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$

View the build log or the debug log for each test

[rileykarson]

/gcbrun

Hmm- we shouldn't have hit quotas there. Weird. Maybe other runs going on at the same time.

[EmileHofsink]

Thanks @rileykarson, I'm not sure if this would be considered a breaking change as it's an optional field and the diff suppress only affected autopilot clusters. But I could be missing something.

[rileykarson]

Yeah, I was looking into that in old test logs. Forgot I had to retrigger the tests (which also would have caught it).

See specific reason I don't think we can make this change as-is inline.

[rileykarson]

Ah, this ends up being a breaking change in practice. Autopilot clusters that don't set a value at all get this back in the response:

  "dnsConfig": {
   "clusterDns": "CLOUD_DNS",
   "clusterDnsScope": "CLUSTER_SCOPE",
   "clusterDnsDomain": "cluster.local"
  },

Terraform will see a diff on that by removing the DSF on the top-level field, even though the subfields will be supressed. If there was not a default value for the block this would be safe.

[github-actions]

@EmileHofsink, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[github-actions]

@EmileHofsink, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[github-actions]

@EmileHofsink, this PR is waiting for action from you. If no action is taken, this PR will be closed in 2 weekdays.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[github-actions]

@EmileHofsink, this PR is being closed due to inactivity.

[hadleyrich]

I've just run into this issue and am unable to create an autopilot cluster with additive_vpc_scope_dns_domain. Is there another issue or PR I'm missing?

[maci0]

There is no other PR for this currently. This one was closed due to inactivity and breaking some of the tests.