Add EncryptionConfig to Cloud Composer's EnvironmentConfig #4310

dzarmola · 2020-12-11T14:06:34Z

Add a new section (EncryptionConfig) to Cloud Composer's EnvironmentConfig used for environment creation and updating

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

composer: added `encryption_config`  to `google_composer_environment` resource (TPGB only)

dzarmola · 2020-12-14T08:30:26Z

Right now I'm mostly hoping for some guidance regarding running tests, as I'm consistently failing with:

==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccComposerEnvironment_withDatabaseConfig -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
# github.com/hashicorp/terraform-provider-google-beta/v3/google-beta [github.com/hashicorp/terraform-provider-google-beta/v3/google-beta.test]
google-beta/resource_composer_environment.go:711:20: patchObj.Config.EncryptionConfig undefined (type *composer.EnvironmentConfig has no field or method EncryptionConfig)
google-beta/resource_composer_environment.go:711:46: config.EncryptionConfig undefined (type *composer.EnvironmentConfig has no field or method EncryptionConfig)
google-beta/resource_composer_environment.go:839:92: envCfg.EncryptionConfig undefined (type *composer.EnvironmentConfig has no field or method EncryptionConfig)
google-beta/resource_composer_environment.go:887:70: undefined: composer.EncryptionConfig
google-beta/resource_composer_environment.go:1026:13: transformed.EncryptionConfig undefined (type *composer.EnvironmentConfig has no field or method EncryptionConfig)
google-beta/resource_composer_environment.go:1095:111: undefined: composer.EncryptionConfig
google-beta/resource_composer_environment.go:1103:18: undefined: composer.EncryptionConfig
FAIL	github.com/hashicorp/terraform-provider-google-beta/v3/google-beta [build failed]
FAIL
make: *** [GNUmakefile:15: testacc] Error 2

nat-henderson · 2020-12-16T23:15:31Z

third_party/terraform/resources/resource_composer_environment.go.erb

@@ -700,6 +722,18 @@ func resourceComposerEnvironmentUpdate(d *schema.ResourceData, meta interface{})
 				return err
 			}
 		}
+<% end -%>


Probably better to combine these two conditions since they're identical, here and elsewhere.

nat-henderson · 2020-12-16T23:16:05Z

third_party/terraform/tests/resource_composer_environment_test.go.erb

+			},
+			// This is a terrible clean-up step in order to get destroy to succeed,
+			// due to dangling firewall rules left by the Composer Environment blocking network deletion.
+			// TODO(emilyye): Remove this check if firewall rules bug gets fixed by Composer.


maybe update the name on the todo if you still need it?

modular-magician · 2021-01-22T10:25:45Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 4 files changed, 181 insertions(+), 1 deletion(-))

modular-magician · 2021-01-28T15:07:23Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 4 files changed, 163 insertions(+), 1 deletion(-))

dzarmola · 2021-01-28T15:11:28Z

In case anybody finds this while searching for the error I posted earlier - in my case the problem was an outdated Go package describing Composer API: https://pkg.go.dev/google.golang.org/api/composer/v1beta1

slevenick · 2021-01-28T18:20:35Z

@dzarmola You will need to rebase or merge master so that this builds successfully I believe. There were directory structure changes since this was opened

…Config used for environment creation

dzarmola · 2021-01-28T19:10:51Z

Thanks, rebased.

modular-magician · 2021-01-28T19:12:19Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 162 insertions(+))

slevenick · 2021-02-01T22:47:29Z

/gcbrun

The version of the googleapis sdk should be updated now

modular-magician · 2021-02-01T22:47:42Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170348"

modular-magician · 2021-02-01T22:55:56Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 162 insertions(+))

modular-magician · 2021-02-02T00:56:13Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccComposerEnvironment_withEncryptionConfig|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityCmekExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=170443"

slevenick

Are you able to successfully run the added test here?

I'm seeing a strange error:

Error: Error waiting to create Environment: Error waiting for Creating Environment: Error code 2, message: Http error status code: 400
Http error message: BAD REQUEST
Errors in: [GKE cluster]; Error messages:
  {"ResourceType":"gcp-types/container-v1beta1:projects.locations.clusters","ResourceErrorCode":"400","ResourceErrorMessage":{"code":400,"message":"\n\t(1) Network \"tf-test-composer-net-8587385686315371018\" has no route to the default internet gateway. To fix this, add a route with destRange '0.0.0.0/0' and nextHopGateway 'default-internet-gateway'\n\t(2) network \"tf-test-composer-net-8587385686315371018\" does not exist.","status":"INVALID_ARGUMENT",

which seems to indicate that the network isn't set up correctly. But this looks identical to the other tests in this file. This could just be a flaky test, I'll rerun

slevenick · 2021-02-02T22:29:31Z

mmv1/third_party/terraform/resources/resource_composer_environment.go.erb

+									"kms_key_name": {
+										Type:         schema.TypeString,
+										Required:     true,
+										Description: `Optional. Customer-managed Encryption Key available through Google's Key Management Service. Cannot be updated.`,


If this cannot be updated as indicated by the description, it should probably be marked ForceNew: true to force Terraform to recreate the resource if this field is changed.

Thanks, added!

With regard to tests - I've noticed some problems with deletion after the test is complete (a firewall rule related issue, which, based on the comments, is known to happen), but nothing at creation.

modular-magician · 2021-02-03T09:34:36Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 163 insertions(+))

dzarmola · 2021-02-11T09:30:23Z

Hey, is there something I could work on here to bring us closer to merging? It's an important part for Cloud Composer CMEK integration

slevenick

Ah, sorry I started this review but didn't send it!

slevenick · 2021-02-03T17:25:53Z

mmv1/third_party/terraform/resources/resource_composer_environment.go.erb

+										Required:     true,
+										Description: `Optional. Customer-managed Encryption Key available through Google's Key Management Service. Cannot be updated.`,
+									},
+									ForceNew:    true,


This should be one line up. Currently getting:

google-beta/resource_composer_environment.go:440:10: undefined: ForceNew

Thanks! Fixed.

slevenick · 2021-02-11T18:04:43Z

/gcbrun

modular-magician · 2021-02-11T18:04:59Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=172009"

modular-magician · 2021-02-11T18:08:13Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 163 insertions(+))

modular-magician · 2021-02-11T18:13:16Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 4 files changed, 164 insertions(+), 1 deletion(-))

slevenick · 2021-02-17T16:09:15Z

/gcbrun

modular-magician · 2021-02-17T16:09:30Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=172932"

modular-magician · 2021-02-17T16:17:57Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 163 insertions(+))

modular-magician · 2021-02-17T17:21:30Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccRedisInstanceDatasource_basic|TestAccComposerEnvironment_withEncryptionConfig|TestAccRedisInstance_redisInstanceBasicExample|TestAccRedisInstance_redisInstanceFullExample|TestAccRedisInstance_update|TestAccRedisInstance_regionFromLocation|TestAccRedisInstance_redisInstanceAuthEnabled You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=172934"

modular-magician · 2021-02-17T18:12:13Z

Tests failed during RECORDING mode: TestAccComposerEnvironment_withEncryptionConfig Please fix these to complete your PR

slevenick · 2021-02-17T18:17:06Z

Composer encryption test is failing, but unsure if it is because of anything in the provider. Error was:

  Creation of airflow web server version failed. This may be an intermittent issue of the App Engine service. You may retry the operation later.
{"ResourceType":"appengine.v1.version","ResourceErrorCode":"504","ResourceErrorMessage":"Timed out waiting for the flex deployment to become network provisioned."}
Cloud Storage Assertions Failed: Unable to write to GCS bucket.
GCS bucket write check failed.

I'll try running it again, but is it possible that the config doesn't set up the environment correctly?

slevenick · 2021-02-17T18:17:11Z

/gcbrun

modular-magician · 2021-02-17T18:17:25Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=172936"

modular-magician · 2021-02-17T18:25:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+))
Terraform Beta: Diff ( 3 files changed, 163 insertions(+))

modular-magician · 2021-02-17T19:26:25Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccComposerEnvironment_withEncryptionConfig|TestAccRedisInstance_redisInstancePrivateServiceExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=172941"

modular-magician · 2021-02-17T19:53:07Z

Tests failed during RECORDING mode: TestAccRedisInstance_redisInstancePrivateServiceExample Please fix these to complete your PR

slevenick

Looks good to me! Only test failing is flaky.

Thanks for the addition

google-cla bot added the cla: yes label Dec 11, 2020

dzarmola mentioned this pull request Dec 14, 2020

Add support for Composer CMEK encryption hashicorp/terraform-provider-google#7996

Closed

rileykarson requested review from a team and nat-henderson and removed request for a team December 14, 2020 20:27

nat-henderson reviewed Dec 16, 2020

View reviewed changes

dzarmola force-pushed the encryption branch from 71e3b26 to f80a2e5 Compare January 22, 2021 10:17

nat-henderson requested review from slevenick and nat-henderson and removed request for nat-henderson January 28, 2021 18:15

Aleksandra Jarmolińska added 4 commits January 28, 2021 20:03

Adds a new section (EncryptionConfig) to Cloud Composer's Environment…

04babd3

…Config used for environment creation

Add missing part of the config.

e6f6357

Fixes to how the kms key is passed

bef372a

Fix testing call

642206d

dzarmola force-pushed the encryption branch from fa17fa2 to 642206d Compare January 28, 2021 19:04

slevenick reviewed Feb 2, 2021

View reviewed changes

slevenick requested changes Feb 11, 2021

View reviewed changes

Fix nesting of ForceNew in encryption_config

a2d8ace

dzarmola force-pushed the encryption branch from 61ba97c to a2d8ace Compare February 11, 2021 17:59

slevenick self-requested a review February 17, 2021 20:30

slevenick approved these changes Feb 17, 2021

View reviewed changes

slevenick merged commit 1c35689 into GoogleCloudPlatform:master Feb 17, 2021

This was referenced Feb 17, 2021

Add EncryptionConfig to Cloud Composer's EnvironmentConfig hashicorp/terraform-provider-google-beta#2967

Merged

Add EncryptionConfig to Cloud Composer's EnvironmentConfig hashicorp/terraform-provider-google#8490

Merged

upodroid mentioned this pull request Mar 5, 2021

Add support for CMEK in Composer hashicorp/terraform-provider-google#8618

Closed

Add EncryptionConfig to Cloud Composer's EnvironmentConfig #4310

Add EncryptionConfig to Cloud Composer's EnvironmentConfig #4310

Conversation

dzarmola commented Dec 11, 2020 • edited Loading

dzarmola commented Dec 14, 2020

nat-henderson Dec 16, 2020

Choose a reason for hiding this comment

nat-henderson Dec 16, 2020

Choose a reason for hiding this comment

modular-magician commented Jan 22, 2021

Diff report:

modular-magician commented Jan 28, 2021

Diff report:

dzarmola commented Jan 28, 2021

slevenick commented Jan 28, 2021

dzarmola commented Jan 28, 2021

modular-magician commented Jan 28, 2021

Diff report:

slevenick commented Feb 1, 2021

modular-magician commented Feb 1, 2021

modular-magician commented Feb 1, 2021

Diff report:

modular-magician commented Feb 2, 2021

slevenick left a comment

Choose a reason for hiding this comment

slevenick Feb 2, 2021

Choose a reason for hiding this comment

dzarmola Feb 3, 2021

Choose a reason for hiding this comment

modular-magician commented Feb 3, 2021

Diff report:

dzarmola commented Feb 11, 2021

slevenick left a comment

Choose a reason for hiding this comment

slevenick Feb 3, 2021

Choose a reason for hiding this comment

dzarmola Feb 11, 2021

Choose a reason for hiding this comment

slevenick commented Feb 11, 2021

modular-magician commented Feb 11, 2021

modular-magician commented Feb 11, 2021

Diff report:

modular-magician commented Feb 11, 2021

Diff report:

slevenick commented Feb 17, 2021

modular-magician commented Feb 17, 2021

modular-magician commented Feb 17, 2021

Diff report:

modular-magician commented Feb 17, 2021

modular-magician commented Feb 17, 2021

slevenick commented Feb 17, 2021

slevenick commented Feb 17, 2021

modular-magician commented Feb 17, 2021

modular-magician commented Feb 17, 2021

Diff report:

modular-magician commented Feb 17, 2021

modular-magician commented Feb 17, 2021

slevenick left a comment

Choose a reason for hiding this comment

dzarmola commented Dec 11, 2020 •

edited

Loading