Skip to content
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

Bootstrap required permissions for composer environment tests #7391

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mmv1/products/healthcare/terraform.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ 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))
subnetwork := network + "-1"
Expand Down Expand Up @@ -377,6 +378,7 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer2(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))
subnetwork := network + "-1"
Expand Down Expand Up @@ -960,6 +962,22 @@ 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",
trodge marked this conversation as resolved.
Show resolved Hide resolved
"compute-system",
"container-engine-robot",
"gcp-sa-artifactregistry",
"gcp-sa-pubsub",
}
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)
}
}

func testAccComposerEnvironmentDestroyProducer(t *testing.T) func(s *terraform.State) error {
return func(s *terraform.State) error {
config := GoogleProviderConfig(t)
Expand Down Expand Up @@ -2325,6 +2343,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
Expand Down
4 changes: 4 additions & 0 deletions mmv1/third_party/terraform/utils/bootstrap_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the function actually do what it's supposed to.

setPolicyRequest := &cloudresourcemanager.SetIamPolicyRequest{Policy: policy}
policy, err = client.Projects.SetIamPolicy(project.ProjectId, setPolicyRequest).Do()
if err != nil {
Expand Down
59 changes: 59 additions & 0 deletions mmv1/third_party/terraform/utils/iam.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,65 @@ 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions might go better in another file but I'm not sure which one. Their main purpose is to provide more information about two sets of bindings than compareBindings does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My inclination would be to but this in bootstrap_utils_test for now because that's the only code using it - but I don't feel super strongly about it.

I do think it could make sense to rename the functions to something like bindingsDiff or diffBindings to indicate that it returns the diff instead of just an equality check. (It's not a true diff since it doesn't let you know which were missing in the actual vs expected, but I think that would probably still be less confusing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning to use this to generate a better error message on line 977 of resource_composer_environment_test.go.erb, but I'll do that in a followup PR.

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
}
}

return results
}

// 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)
Expand Down