-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Dataproc Metastore CMEK config #5881
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @melinath, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 46 insertions(+)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 46 insertions(+)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 46 insertions(+)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 46 insertions(+)) |
I don't see anything useful in the logs for the TerraformVCRCommunity failure, is there somewhere I'm missing to see if its flakes or my change? |
It looks like the test timed out. I'll rerun it. Are you able to run your new tests successfully locally? /gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 46 insertions(+)) |
Tests rerun |
Tests count: |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekExample|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccComputeBackendService_backendServiceNetworkEndpointExample|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_basic|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekExample failed with the following error:
Error: Error creating Service: googleapi: Error 400: Found 1 problem:
1) The Cloud KMS key (projects/ci-test-project-188019/locations/us/keyRings/tf-test-example-keyringl2vjah08wi/cryptoKeys/tf-test-example-keyl2vjah08wi) could not be validated. Please ensure that the key's purpose is configured as `ENCRYPT_DECRYPT`, and the Dataproc Metastore service agent (service-1067888929963@gcp-sa-metastore.iam.gserviceaccount.com) has been granted `roles/cloudkms.cryptoKeyEncrypterDecrypter`.
The other test failure is unrelated to your change. Please make sure TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekExample passes for you locally and then re-request review.
Hey @melinath, ENCRYPT_DECRYPT is the default for the keys in the example, however, the SA has no permission in this project, am I allowed to try to set the SA to have permissions via the TF example? Or is there a best practice there? |
You'll probably need to match what the tests for kms crypto key do. It looks like the examples are only for documentation:
The actual tests are handwritten:
But that might not actually be required in your case. The main point is that you'll need to create a project to put the crypto key inside:
There should be other test-only examples that use a similar pattern. (If you can make this work as an example, that would be preferred.) |
Alright, I got it working as an example. Sorry for how long it took, took some iterations to get a working combination. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, glad you were able to make it work. It looks like the tests are failing and I'm not sure why; as a first step, could you rebase your branch off of main? There's been some changes to the CI/CD pipeline recently that might have fixed this issue.
/gcbrun in case it helps. |
This comment was marked as outdated.
This comment was marked as outdated.
Rerunning tests because the vcr pipeline should be stable now. /gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 41 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccAccessContextManager|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentFullExample|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekTestExample|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerCluster_withAuthenticatorGroupsConfig |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Looks like my test failed because the Cloud Storage SA didn't have correct permissions. Sorry, hard to reconcile what's different about my local project and how the test project will react. This should be the final change. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 41 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekTestExample|TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic |
Unfortunately at this point I'm a little lost on what to do: The test is failing because it successfully creates then runs a GET request that throws a 404, everything /seems/ to match up, but I'm not sure how my test could've introduced this issue. Could it be a dependency issue in my terraform infra? |
OK, with a run with TF_LOG=debug, I saw that we actually have a bug in our Dataproc Metastore resource, if version wasn't specified it would force recreation during the test. Should be fixed in #5921. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 1 file changed, 45 insertions(+)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataprocMetastoreService_dataprocMetastoreServiceCmekTestExample |
@Noremac201 do you want to tweak the test in this PR to supply a version, or do you want to wait for the other PR to be merged? |
(I would recommend tweaking the test in this PR.) |
@melinath Yes, I believe I tweaked it in this PR, both the example and actual test have the version set and it's passing now. This PR should be good to go from what I can tell. |
This LGTM generally - just want to confirm that kms_key is an |
Yes that is correct, it is IMMUTABLE in our API definition, though I also don't see it anywhere in our docs, I'll have to get that update to make it clearer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds support for configuring CMEK during creation for Dataproc Metastore configurations.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)