From 538f6b81959c43c7f53fa34eae940757f40c4fc7 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Wed, 1 Mar 2023 23:41:54 +0000 Subject: [PATCH 1/5] Bootstrap the required permissions --- .../resource_composer_environment_test.go.erb | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb index a5c2b2d57405..7ad67535882a 100644 --- a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb @@ -342,8 +342,9 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer1(t *testing.T) { kms := BootstrapKMSKeyInLocation(t, "us-central1") pid := GetTestProjectFromEnv() - envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t)) - network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t)) + grantEncrypterDecrypterRoleToServiceAgents(t) + envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, randInt(t)) + network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, randInt(t)) subnetwork := network + "-1" VcrTest(t, resource.TestCase{ @@ -376,9 +377,10 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer2(t *testing.T) { t.Parallel() kms := BootstrapKMSKeyInLocation(t, "us-central1") - pid := GetTestProjectFromEnv() - envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t)) - network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t)) + pid := getTestProjectFromEnv() + grantEncrypterDecrypterRoleToServiceAgents(t) + envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, randInt(t)) + network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, randInt(t)) subnetwork := network + "-1" VcrTest(t, resource.TestCase{ @@ -960,6 +962,21 @@ func TestAccComposerEnvironment_fixPyPiPackages(t *testing.T) { }) } +// This bootstraps the IAM roles needed for the service agents when using encryption. +func grantEncrypterDecrypterRoleToServiceAgents(t *testing.T) { + serviceAgents := []string{ + "cloudcomposer-accounts", + "compute-system", + "container-engine-robot", + "gcp-sa-artifactregistry", + "gcp-sa-pubsub", + } + if policyChanged := BootstrapAllPSARole(t, serviceAgents, "roles/cloudkms.cryptoKeyEncrypterDecrypter"); policyChanged { + // Fail this test run because the policy needs time to reconcile. + t.Fatalf("Granted crypto key encrypter/decrypter role for %v in test project's IAM policy. Retry the test in a few minutes.", serviceAgents) + } +} + func testAccComposerEnvironmentDestroyProducer(t *testing.T) func(s *terraform.State) error { return func(s *terraform.State) error { config := GoogleProviderConfig(t) @@ -2325,6 +2342,7 @@ resource "google_project_iam_member" "composer-worker" { `, environment, network, subnetwork, serviceAccount) } + /** * CLEAN UP HELPER FUNCTIONS * Because the environments are flaky and bucket deletion rates can be From f4d81fb4410dd53febdce2fd32344a660b8dd88f Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Thu, 2 Mar 2023 20:41:08 +0000 Subject: [PATCH 2/5] Make BootstrapAllPSARoles actually work Also adds some helper functions for debugging what the bootstrap function does. It will now log the roles that were missing in the policy. --- .../resource_composer_environment_test.go.erb | 2 +- .../terraform/utils/bootstrap_utils_test.go | 4 ++ mmv1/third_party/terraform/utils/iam.go.erb | 57 +++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb index 7ad67535882a..b34a16e7e02f 100644 --- a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb @@ -973,7 +973,7 @@ func grantEncrypterDecrypterRoleToServiceAgents(t *testing.T) { } if policyChanged := BootstrapAllPSARole(t, serviceAgents, "roles/cloudkms.cryptoKeyEncrypterDecrypter"); policyChanged { // Fail this test run because the policy needs time to reconcile. - t.Fatalf("Granted crypto key encrypter/decrypter role for %v in test project's IAM policy. Retry the test in a few minutes.", serviceAgents) + t.Fatalf("Granted crypto key encrypter/decrypter role for service agents %v in test project's IAM policy. Retry the test in a few minutes.", serviceAgents) } } diff --git a/mmv1/third_party/terraform/utils/bootstrap_utils_test.go b/mmv1/third_party/terraform/utils/bootstrap_utils_test.go index bcb464bb4d36..8571e7e2ce90 100644 --- a/mmv1/third_party/terraform/utils/bootstrap_utils_test.go +++ b/mmv1/third_party/terraform/utils/bootstrap_utils_test.go @@ -561,7 +561,11 @@ func BootstrapAllPSARoles(t *testing.T, agentNames, roles []string) bool { mergedBindings := MergeBindings(append(policy.Bindings, newBindings...)) if !compareBindings(policy.Bindings, mergedBindings) { + for _, missingBinding := range missingBindings(policy.Bindings, mergedBindings) { + log.Printf("[DEBUG] Missing binding: %v", missingBinding) + } // The policy must change. + policy.Bindings = mergedBindings setPolicyRequest := &cloudresourcemanager.SetIamPolicyRequest{Policy: policy} policy, err = client.Projects.SetIamPolicy(project.ProjectId, setPolicyRequest).Do() if err != nil { diff --git a/mmv1/third_party/terraform/utils/iam.go.erb b/mmv1/third_party/terraform/utils/iam.go.erb index 3b3bb45f45d9..453fb27f18d9 100644 --- a/mmv1/third_party/terraform/utils/iam.go.erb +++ b/mmv1/third_party/terraform/utils/iam.go.erb @@ -448,6 +448,63 @@ func compareBindings(a, b []*cloudresourcemanager.Binding) bool { return reflect.DeepEqual(aMap, bMap) } +// Returns a map representing iam bindings that are in one map but not the other. +func missingBindingsMap(aMap, bMap map[iamBindingKey]map[string]struct{}) map[iamBindingKey]map[string]struct{} { + results := make(map[iamBindingKey]map[string]struct{}) + for key, aMembers := range aMap { + if bMembers, ok := bMap[key]; ok { + // The key is in both maps. + resultMembers := make(map[string]struct{}) + + for aMember := range aMembers { + if _, ok := bMembers[aMember]; !ok { + // The member is in a but not in b. + resultMembers[aMember] = struct{}{} + } + } + for bMember := range bMembers { + if _, ok := aMembers[bMember]; !ok { + // The member is in b but not in a. + resultMembers[bMember] = struct{}{} + } + } + + if len(resultMembers) > 0 { + results[key] = resultMembers + } + } else { + // The key is in map a but not map b. + results[key] = aMembers + } + } + + for key, bMembers := range bMap { + if _, ok := aMap[key]; !ok { + // The key is in map b but not map a. + results[key] = bMembers + } + } +} + +// Returns the bindings that are in one set of bindings and not the other. +func missingBindings(a, b []*cloudresourcemanager.Binding) []*cloudresourcemanager.Binding { + aMap := createIamBindingsMap(a) + bMap := createIamBindingsMap(b) + + var results []*cloudresourcemanager.Binding + for key, membersSet := range missingBindingsMap(aMap, bMap) { + members := make([]string, 0, len(membersSet)) + for member := range membersSet { + members = append(members, member) + } + results = append(results, &cloudresourcemanager.Binding{ + Role: key.Role, + Members: members, + }) + } + return results +} + func compareAuditConfigs(a, b []*cloudresourcemanager.AuditConfig) bool { aMap := createIamAuditConfigsMap(a) bMap := createIamAuditConfigsMap(b) From 5de7a957a8dbb148a69f546cd2ffc32434f59432 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Fri, 3 Mar 2023 01:59:59 +0000 Subject: [PATCH 3/5] Add missing return, fix nits --- .../terraform/tests/resource_composer_environment_test.go.erb | 3 ++- mmv1/third_party/terraform/utils/iam.go.erb | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb index b34a16e7e02f..edb537f7e1a2 100644 --- a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb @@ -971,7 +971,8 @@ func grantEncrypterDecrypterRoleToServiceAgents(t *testing.T) { "gcp-sa-artifactregistry", "gcp-sa-pubsub", } - if policyChanged := BootstrapAllPSARole(t, serviceAgents, "roles/cloudkms.cryptoKeyEncrypterDecrypter"); policyChanged { + role := "roles/cloudkms.cryptoKeyEncrypterDecrypter" + if policyChanged := BootstrapAllPSARole(t, serviceAgents, role); policyChanged { // Fail this test run because the policy needs time to reconcile. t.Fatalf("Granted crypto key encrypter/decrypter role for service agents %v in test project's IAM policy. Retry the test in a few minutes.", serviceAgents) } diff --git a/mmv1/third_party/terraform/utils/iam.go.erb b/mmv1/third_party/terraform/utils/iam.go.erb index 453fb27f18d9..8a08ce36b796 100644 --- a/mmv1/third_party/terraform/utils/iam.go.erb +++ b/mmv1/third_party/terraform/utils/iam.go.erb @@ -484,6 +484,8 @@ func missingBindingsMap(aMap, bMap map[iamBindingKey]map[string]struct{}) map[ia results[key] = bMembers } } + + return results } // Returns the bindings that are in one set of bindings and not the other. From 29b9a6abcde9971f699e30f893240deef51d357d Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 6 Mar 2023 09:31:39 -0800 Subject: [PATCH 4/5] Fix typo in service agent name --- mmv1/products/healthcare/terraform.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mmv1/products/healthcare/terraform.yaml b/mmv1/products/healthcare/terraform.yaml index c7a2ea01d731..c47422024bb4 100644 --- a/mmv1/products/healthcare/terraform.yaml +++ b/mmv1/products/healthcare/terraform.yaml @@ -60,7 +60,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides pubsub_topic: "fhir-notifications" bq_dataset_name: "bq_example_dataset" test_vars_overrides: - policyChanged: 'BootstrapPSARoles(t, "gsp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})' + policyChanged: 'BootstrapPSARoles(t, "gcp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})' - !ruby/object:Provider::Terraform::Examples name: "healthcare_fhir_store_notification_config" primary_resource_id: "default" @@ -110,7 +110,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides bq_dataset_name: "dicom_bq_ds" bq_table_name: "dicom_bq_tb" test_vars_overrides: - policyChanged: 'BootstrapPSARoles(t, "gsp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})' + policyChanged: 'BootstrapPSARoles(t, "gcp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})' properties: creationTime: !ruby/object:Overrides::Terraform::PropertyOverride exclude: true From f0e33a430bde0352ef0ccb6e7d3472235821bc5b Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 6 Mar 2023 09:55:43 -0800 Subject: [PATCH 5/5] Account for newly exported test functions --- .../tests/resource_composer_environment_test.go.erb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb index edb537f7e1a2..0b61d8d2592f 100644 --- a/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb +++ b/mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb @@ -343,8 +343,8 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer1(t *testing.T) { kms := BootstrapKMSKeyInLocation(t, "us-central1") pid := GetTestProjectFromEnv() grantEncrypterDecrypterRoleToServiceAgents(t) - envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, randInt(t)) - network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, randInt(t)) + envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t)) + network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t)) subnetwork := network + "-1" VcrTest(t, resource.TestCase{ @@ -377,10 +377,10 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer2(t *testing.T) { t.Parallel() kms := BootstrapKMSKeyInLocation(t, "us-central1") - pid := getTestProjectFromEnv() + pid := GetTestProjectFromEnv() grantEncrypterDecrypterRoleToServiceAgents(t) - envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, randInt(t)) - network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, randInt(t)) + envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t)) + network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t)) subnetwork := network + "-1" VcrTest(t, resource.TestCase{