No pdcsi disable on create #9557

mattcary · 2023-11-30T23:22:48Z

The PDCSI addon cannot be disabled on cluster creation.

Fixes: hashicorp/terraform-provider-google#16114

Release Note Template for Downstream PRs (will be copied)

container: fixed a bug where disable PDCSI addon `gce_persistent_disk_csi_driver_config ` during creation will result in permadiff in `google_container_cluster` resource

modular-magician · 2023-11-30T23:23:57Z

Hello! I am a robot. It looks like you are a: ~~Community Contributor~~ Googler ~~Core Contributor~~. Tests will run automatically.

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

modular-magician · 2023-11-30T23:34:22Z

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.

Terraform GA: Diff ( 1 file changed, 6 insertions(+))
Terraform Beta: Diff ( 1 file changed, 6 insertions(+))

modular-magician · 2023-12-01T00:50:26Z

$\textcolor{red}{\textsf{The provider crashed while running the VCR tests in REPLAYING mode}}$
$\textcolor{red}{\textsf{Please fix it to complete your PR}}$
View the build log

mattcary · 2023-12-01T22:04:51Z

/restest

mattcary · 2023-12-01T22:04:57Z

/retest

modular-magician · 2023-12-01T22:14:13Z

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.

Terraform GA: Diff ( 1 file changed, 8 insertions(+))
Terraform Beta: Diff ( 1 file changed, 8 insertions(+))

modular-magician · 2023-12-01T23:26:49Z

Tests analytics

Total tests: 3255
Passed tests 2923
Skipped tests: 331
Affected tests: 1

Action taken

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

TestAccLoggingProjectSink_updatePreservesCustomWriter

Get to know how VCR tests work

modular-magician · 2023-12-01T23:28:37Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccLoggingProjectSink_updatePreservesCustomWriter[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

mattcary · 2023-12-01T23:55:05Z

The acceptance tests are failing, including the same cannot-create-key error that the VCR test above is failing with.

My test shouldn't be affecting any of that so I'm hoping it's an infra flake.

shuyama1

The current failing VCR test seems unrelated to this change. No need to worry about it. Sorry for the noise. We skipped the test TestAccContainerCluster_withAddons from running completely since it's consistently failing. You'll need to remove the t.Skipf flag in the test:

magic-modules/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb

Line 130 in 6d9fb14

    
           t.Skipf("Skipping test %s due to https://github.com/hashicorp/terraform-provider-google/issues/16114", t.Name())

, so it will be running in the presubmit checks to test if the change is resolving the issue.

modular-magician · 2023-12-04T20:46:01Z

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.

Terraform GA: Diff ( 2 files changed, 8 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 8 insertions(+), 1 deletion(-))

shuyama1 · 2023-12-04T23:25:22Z

/gcbrun

modular-magician · 2023-12-04T23:35:22Z

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.

Terraform GA: Diff ( 2 files changed, 8 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 8 insertions(+), 1 deletion(-))

modular-magician · 2023-12-05T00:47:43Z

Tests analytics

Total tests: 3260
Passed tests 2927
Skipped tests: 331
Affected tests: 2

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_withAddons|TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

modular-magician · 2023-12-05T00:59:23Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAddons[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

shuyama1

The test failed due to the same permadiff:

vcr_utils.go:152: Step 1/4 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        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.primary will be updated in-place
          ~ resource "google_container_cluster" "primary" {
                id                          = "projects/ci-test-project-188019/locations/us-central1-a/clusters/tf-test-cluster-xgap7otdsy"
                name                        = "tf-test-cluster-xgap7otdsy"
                # (28 unchanged attributes hidden)
        
              ~ addons_config {
        
                  ~ gce_persistent_disk_csi_driver_config {
                      ~ enabled = true -> false
                    }
        
                    # (11 unchanged blocks hidden)
                }
        
                # (21 unchanged blocks hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.

It means the the user set gce_persistent_disk_csi_driver_config.enable = false but end up getting gce_persistent_disk_csi_driver_config.enable = true in the TF state. Since GcePersistentDiskCsiDriver cannot be disabled at cluster create, only on cluster update, what we need to do is first make a create call with gce_persistent_disk_csi_driver_config.enable = true and then make a update call to update the field to false to match users' config. And all these will be happen in the Terraform resourceContainerClusterCreate method, so that when users try to set gce_persistent_disk_csi_driver_config.enable = false when creating the resource initially, they will have the resource as intended.

Similar example like privateClusterConfig.EnablePrivateEndpoint as you mentioned in the ticket. It won't support enable_private_endpoint on create. We need to set it to false in the create call, and then make an update call if users set it as true in the first place.

Please let me know if you have any questions.

mattcary · 2023-12-05T18:06:16Z

Oh, I see. I missed that we want to emulate create with false by doing an update. Thx.

mattcary · 2023-12-05T18:10:06Z

Do we care that we may be doing several updates if, eg, pdcsi is disabled and privateClusterConfig is similarly set?

In theory I think we could just do a single update after create in order to set values that can only be set on update. But I don't know if that's going to complicate things too much given how resourceContainerClusterCreate is structured.

shuyama1 · 2023-12-05T18:40:02Z

Do we care that we may be doing several updates if, eg, pdcsi is disabled and privateClusterConfig is similarly set?

In theory I think we could just do a single update after create in order to set values that can only be set on update. But I don't know if that's going to complicate things too much given how resourceContainerClusterCreate is structured.

I think that's a good concern. I'm not sure why it's set up this way in the first place, it may due to that we need to update features one by one during the time it was implemented or it may be easier to check and update a single feature instead of multiple ones. It's implemented a few years ago so I'm only guessing here. So if you see that pdcsi can be combined to one of the current update calls, feel free to add it in and see if it works.

modular-magician · 2023-12-07T23:22:58Z

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.

Terraform GA: Diff ( 2 files changed, 41 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 2 files changed, 41 insertions(+), 9 deletions(-))

modular-magician · 2023-12-08T00:36:28Z

Tests analytics

Total tests: 3275
Passed tests 2941
Skipped tests: 333
Affected tests: 1

Action taken

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

TestAccContainerCluster_withAddons

Get to know how VCR tests work

mattcary · 2023-12-08T00:39:57Z

Hmm, locally the _withAddons test failed with the following. I'm not sure if that's due to my changes are not, I'll look into it but while I do so are there any known flakes around this?

        Error: googleapi: Error 400: This operation will exceed max secondary ranges per subnetwork (30) for subnet "gke-cluster", consider reusing existing secondary ranges or use a different subnetwork.
        Details:
        [
          {
            "@type": "type.googleapis.com/google.rpc.RequestInfo",
            "requestId": "0x7846e95b8198a8ea"
          },
          {
            "@type": "type.googleapis.com/google.rpc.DebugInfo",
            "detail": "INVALID_ARGUMENT: this operation will exceed max secondary ranges per subnetwork (30) for subnet \"gke-cluster\", consider reusing existing secondary ranges or use a different subnetwork",

modular-magician · 2023-12-08T00:48:56Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerCluster_withAddons[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

shuyama1 · 2023-12-08T17:43:24Z

The test failed probably due to a missing } after

<% unless version == 'ga' -%>
    istio_config {
      disabled = false
      auth     = "AUTH_NONE"
    }
    kalm_config {
      enabled = true
    }
<% end -%>

in the testAccContainerCluster_updateAddons.

The error that you run into locally is due to that it's hitting the max (30) of secondary ranges for subnet "gke-cluster" in your local testing environment, you may want to clean some of those up and see if the error still occurs.

mattcary · 2023-12-08T18:48:47Z

Thanks! I pushed the changes up to test upstream while I sort out my project.

modular-magician · 2023-12-08T18:58:45Z

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.

Terraform GA: Diff ( 2 files changed, 48 insertions(+), 15 deletions(-))
Terraform Beta: Diff ( 2 files changed, 48 insertions(+), 15 deletions(-))

modular-magician · 2023-12-08T20:11:09Z

Tests analytics

Total tests: 3275
Passed tests 2941
Skipped tests: 333
Affected tests: 1

Action taken

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

TestAccContainerCluster_withAddons

Get to know how VCR tests work

modular-magician · 2023-12-08T20:48:16Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerCluster_withAddons[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\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

shuyama1

LGTM overall. only some nit-picks and small comments.

shuyama1 · 2023-12-11T21:23:46Z

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

@@ -2384,12 +2384,29 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er
 		cluster.SecurityPostureConfig = expandSecurityPostureConfig(v)
 	}

+  needUpdateAfterCreate := false


Suggested change

needUpdateAfterCreate := false

needUpdateAfterCreate := false

nit: we use tabs instead spaces for indentation in this file. Please also update the rest of the file. Sorry for the nitpick and thank you!

Done! Hopefully it worked right, I'll upload and see how github shows the changes.

Hmm, I tabified but it seems to have affected quite a bit of the file. WDYT?

Thanks for updating the file. Sorry I was not clear before. I mean let's update the indentation of the code changed in this PR but not the whole file. Sorry about it. Plus, we use spaces for inline spacing and tabs for indentation for resource.go file and in test.go file, we use spaces for resource configuration blocks in the tests: https://github.com/mattcary/magic-modules/blob/pdcsi-create/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb#L5134.

Sorry if I've made it more confused, you can check generated resource and test files. https://github.com/hashicorp/terraform-provider-google-beta/blob/main/google-beta/services/cloudrunv2/resource_cloud_run_v2_job.go
https://github.com/hashicorp/terraform-provider-google-beta/blob/main/google-beta/services/cloudrunv2/resource_cloud_run_v2_job_generated_test.go
Handwritten files should also follow that format.

Anyways, let's only update the code touched in this PR - this is to prevent merge conflicts for this and other ongoing PRs. Thank you and sorry for the confusion.

Okay, I think I got it. I'm not totally sure how to check that tabs are in the right places though.

shuyama1 · 2023-12-11T22:17:37Z

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

+    if cluster.AddonsConfig == nil {
+      cluster.AddonsConfig = &container.AddonsConfig{}
+    }
+    if cluster.AddonsConfig.GcePersistentDiskCsiDriverConfig == nil {
+      cluster.AddonsConfig.GcePersistentDiskCsiDriverConfig = &container.GcePersistentDiskCsiDriverConfig{}
+    }


Do we need these two checks here? I'd assume we wouldn't reach these two conditions since we'd have enablePDCSI = true if cluster.AddonsConfig == nil || cluster.AddonsConfig.GcePersistentDiskCsiDriverConfig == nil? Or I could be missing some cases.

There are several other addons besides pdcsi. So it definitely could be the case that addons config != nil but the pdcsi config is nil.

However, as mentioned in the comment above I'm being paranoid because if pdcsi is not enabled, the config must have been explicitly defined.

But it seems an easy check to make and who knows how the code might change in the future.

Yeah, that makes sense. I was just curious since these checks are under if !enablePDCSI. But let's keep them just in case. Thanks!

modular-magician · 2023-12-13T20:46:56Z

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.

Terraform GA: Diff ( 2 files changed, 41 insertions(+), 9 deletions(-))
Terraform Beta: Diff ( 2 files changed, 41 insertions(+), 9 deletions(-))

modular-magician · 2023-12-13T21:46:35Z

Tests analytics

Total tests: 3292
Passed tests 2954
Skipped tests: 337
Affected tests: 1

Action taken

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

TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

modular-magician · 2023-12-13T21:49:31Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountJwt[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\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

shuyama1

LGTM. Thanks!

modular-magician requested a review from shuyama1 November 30, 2023 23:23

mattcary force-pushed the pdcsi-create branch from 869e1a6 to 4e6a7ca Compare December 1, 2023 22:04

mattcary marked this pull request as ready for review December 1, 2023 23:50

shuyama1 requested changes Dec 4, 2023

View reviewed changes

mattcary force-pushed the pdcsi-create branch from 4e6a7ca to 493819d Compare December 4, 2023 20:36

modular-magician requested a review from shuyama1 December 4, 2023 20:37

shuyama1 requested changes Dec 5, 2023

View reviewed changes

mattcary force-pushed the pdcsi-create branch from 493819d to 1c938f0 Compare December 7, 2023 23:12

modular-magician requested a review from shuyama1 December 7, 2023 23:14

mattcary force-pushed the pdcsi-create branch from 1c938f0 to 75bd248 Compare December 8, 2023 18:48

shuyama1 reviewed Dec 11, 2023

View reviewed changes

mattcary force-pushed the pdcsi-create branch 2 times, most recently from b4d24c4 to fc95fba Compare December 13, 2023 18:45

modular-magician requested a review from shuyama1 December 13, 2023 18:46

mattcary force-pushed the pdcsi-create branch 2 times, most recently from 8c73cdf to 69785e2 Compare December 13, 2023 20:35

disable pdcsi on cluster creation

634c658

mattcary force-pushed the pdcsi-create branch from 69785e2 to 634c658 Compare December 13, 2023 20:36

shuyama1 approved these changes Dec 13, 2023

View reviewed changes

shuyama1 merged commit f8feaf0 into GoogleCloudPlatform:main Dec 13, 2023
13 checks passed

This was referenced Dec 13, 2023

No pdcsi disable on create hashicorp/terraform-provider-google-beta#6751

Merged

No pdcsi disable on create hashicorp/terraform-provider-google#16794

Merged

No pdcsi disable on create GoogleCloudPlatform/terraform-google-conversion#1789

Merged

kapreus pushed a commit to kapreus/magic-modules that referenced this pull request Jan 2, 2024

No pdcsi disable on create (GoogleCloudPlatform#9557)

b7b4c7c

balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024

No pdcsi disable on create (GoogleCloudPlatform#9557)

8e44ac8

	needUpdateAfterCreate := false
	needUpdateAfterCreate := false

No pdcsi disable on create #9557

No pdcsi disable on create #9557

Conversation

mattcary commented Nov 30, 2023 • edited by shuyama1 Loading

modular-magician commented Nov 30, 2023

modular-magician commented Nov 30, 2023

Diff report

modular-magician commented Dec 1, 2023

mattcary commented Dec 1, 2023

mattcary commented Dec 1, 2023

modular-magician commented Dec 1, 2023

Diff report

modular-magician commented Dec 1, 2023

Tests analytics

Action taken

modular-magician commented Dec 1, 2023

mattcary commented Dec 1, 2023

shuyama1 left a comment

Choose a reason for hiding this comment

modular-magician commented Dec 4, 2023

Diff report

shuyama1 commented Dec 4, 2023

modular-magician commented Dec 4, 2023

Diff report

modular-magician commented Dec 5, 2023

Tests analytics

Action taken

modular-magician commented Dec 5, 2023

Rerun these tests in REPLAYING mode to catch issues

shuyama1 left a comment

Choose a reason for hiding this comment

mattcary commented Dec 5, 2023

mattcary commented Dec 5, 2023

shuyama1 commented Dec 5, 2023

modular-magician commented Dec 7, 2023

Diff report

modular-magician commented Dec 8, 2023

Tests analytics

Action taken

mattcary commented Dec 8, 2023

modular-magician commented Dec 8, 2023

shuyama1 commented Dec 8, 2023 • edited Loading

mattcary commented Dec 8, 2023

modular-magician commented Dec 8, 2023

Diff report

modular-magician commented Dec 8, 2023

Tests analytics

Action taken

modular-magician commented Dec 8, 2023

Rerun these tests in REPLAYING mode to catch issues

shuyama1 left a comment

Choose a reason for hiding this comment

shuyama1 Dec 11, 2023

Choose a reason for hiding this comment

mattcary Dec 13, 2023

Choose a reason for hiding this comment

mattcary Dec 13, 2023

Choose a reason for hiding this comment

shuyama1 Dec 13, 2023

Choose a reason for hiding this comment

mattcary Dec 13, 2023

Choose a reason for hiding this comment

shuyama1 Dec 11, 2023

Choose a reason for hiding this comment

mattcary Dec 13, 2023

Choose a reason for hiding this comment

shuyama1 Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

modular-magician commented Dec 13, 2023

Diff report

modular-magician commented Dec 13, 2023

Tests analytics

Action taken

modular-magician commented Dec 13, 2023

Rerun these tests in REPLAYING mode to catch issues

shuyama1 left a comment

Choose a reason for hiding this comment

mattcary commented Nov 30, 2023 •

edited by shuyama1

Loading

shuyama1 commented Dec 8, 2023 •

edited

Loading

shuyama1 Dec 13, 2023 •

edited

Loading