magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

Autopilot Suppressed Diff Prevents using `additive_vpc_scope_dns_domain`

Open EmileHofsink opened this issue 1 year ago • 12 comments

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

EmileHofsink avatar Sep 18 '24 10:09 EmileHofsink

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.

github-actions[bot] avatar Sep 18 '24 10:09 github-actions[bot]

we had a conversation regarding this in this PR also: https://github.com/GoogleCloudPlatform/magic-modules/pull/11562

maci0 avatar Sep 20 '24 06:09 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" {
│ 

maci0 avatar Sep 20 '24 06:09 maci0

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.

EmileHofsink avatar Sep 23 '24 00:09 EmileHofsink

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

github-actions[bot] avatar Sep 23 '24 09:09 github-actions[bot]

@slevenick could you take a look at this one?

maci0 avatar Sep 24 '24 08:09 maci0

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 avatar Sep 24 '24 18:09 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 avatar Sep 24 '24 18:09 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

modular-magician avatar Sep 24 '24 18:09 modular-magician

/gcbrun

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

rileykarson avatar Sep 24 '24 20:09 rileykarson

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.

EmileHofsink avatar Sep 24 '24 21:09 EmileHofsink

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

Image showing the re-request review button

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

github-actions[bot] avatar Oct 08 '24 09:10 github-actions[bot]

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

Image showing the re-request review button

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

github-actions[bot] avatar Oct 22 '24 09:10 github-actions[bot]

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

Image showing the re-request review button

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

github-actions[bot] avatar Nov 01 '24 09:11 github-actions[bot]

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

github-actions[bot] avatar Nov 05 '24 09:11 github-actions[bot]

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?

hadleyrich avatar Dec 04 '24 20:12 hadleyrich

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

maci0 avatar Dec 05 '24 08:12 maci0