Implemented simpler IAM bootstrapping logic (GoogleCloudPlatform#12796)

Author: melinath

mmv1/third_party/terraform/acctest/bootstrap_iam_test_utils.go

@@ -3,21 +3,25 @@ package acctest
 import (
 	"fmt"
 	"log"
+	"strconv"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/terraform-provider-google/google/envvar"
 	"github.com/hashicorp/terraform-provider-google/google/tpgiamresource"
 	cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1"
 )
 
-// BootstrapAllPSARoles ensures that the given project's IAM
-// policy grants the given service agents the given roles.
-// prefix is usually "service-" and indicates the service agent should have the
-// given prefix before the project number.
-// This is important to bootstrap because using iam policy resources means that
-// deleting them removes permissions for concurrent tests.
-// Return whether the bindings changed.
-func BootstrapAllPSARoles(t *testing.T, prefix string, agentNames, roles []string) bool {
+type IamMember struct {
+	Member, Role string
+}
+
+// BootstrapIamMembers ensures that a given set of member/role pairs exist in the default
+// test project. This should be used to avoid race conditions that can happen on the
+// default project due to parallel tests managing the same member/role pairings. Members
+// will have `{project_number}` replaced with the default test project's project number.
+func BootstrapIamMembers(t *testing.T, members []IamMember) {
 	config := BootstrapConfig(t)
 	if config == nil {
 		t.Fatal("Could not bootstrap a config for BootstrapAllPSARoles.")
@@ -36,17 +40,12 @@ func BootstrapAllPSARoles(t *testing.T, prefix string, agentNames, roles []strin
 		t.Fatalf("Error getting project iam policy: %v", err)
 	}
 
-	members := make([]string, len(agentNames))
-	for i, agentName := range agentNames {
-		members[i] = fmt.Sprintf("serviceAccount:%s%d@%s.iam.gserviceaccount.com", prefix, project.ProjectNumber, agentName)
-	}
-
 	// Create the bindings we need to add to the policy.
 	var newBindings []*cloudresourcemanager.Binding
-	for _, role := range roles {
+	for _, member := range members {
 		newBindings = append(newBindings, &cloudresourcemanager.Binding{
-			Role:    role,
-			Members: members,
+			Role:    member.Role,
+			Members: []string{strings.ReplaceAll(member.Member, "{project_number}", strconv.FormatInt(project.ProjectNumber, 10))},
 		})
 	}
 
@@ -68,10 +67,32 @@ func BootstrapAllPSARoles(t *testing.T, prefix string, agentNames, roles []strin
 		for _, binding := range addedBindings {
 			msg += fmt.Sprintf("Members: %q, Role: %q\n", binding.Members, binding.Role)
 		}
-		msg += "Retry the test in a few minutes."
-		t.Error(msg)
-		return true
+		msg += "Waiting for IAM to propagate."
+		t.Log(msg)
+		time.Sleep(3 * time.Minute)
+	}
+}
+
+// BootstrapAllPSARoles ensures that the given project's IAM
+// policy grants the given service agents the given roles.
+// prefix is usually "service-" and indicates the service agent should have the
+// given prefix before the project number.
+// This is important to bootstrap because using iam policy resources means that
+// deleting them removes permissions for concurrent tests.
+// Return whether the bindings changed.
+func BootstrapAllPSARoles(t *testing.T, prefix string, agentNames, roles []string) bool {
+	var members []IamMember
+	for _, agentName := range agentNames {
+		member := fmt.Sprintf("serviceAccount:%s{project_number}@%s.iam.gserviceaccount.com", prefix, agentName)
+		for _, role := range roles {
+			members = append(members, IamMember{
+				Member: member,
+				Role:   role,
+			})
+		}
 	}
+	BootstrapIamMembers(t, members)
+	// Always return false because we now wait for IAM propagation.
 	return false
 }
 

mmv1/third_party/terraform/services/composer/resource_composer_environment_test.go.tmpl

@@ -22,14 +22,29 @@ const testComposerNetworkPrefix = "tf-test-composer-net"
 const testComposerBucketPrefix = "tf-test-composer-bucket"
 const testComposerNetworkAttachmentPrefix = "tf-test-composer-nta"
 
-func allComposerServiceAgents() []string {
-	return []string{
-		"cloudcomposer-accounts",
-		"compute-system",
-		"container-engine-robot",
-		"gcp-sa-artifactregistry",
-		"gcp-sa-pubsub",
-	}
+func bootstrapComposerServiceAgents(t *testing.T) {
+	acctest.BootstrapIamMembers(t, []acctest.IamMember{
+		{
+			Member: "serviceAccount:service-{project_number}@cloudcomposer-accounts.iam.gserviceaccount.com",
+			Role:   "roles/cloudkms.cryptoKeyEncrypterDecrypter",
+		},
+		{
+			Member: "serviceAccount:service-{project_number}@compute-system.iam.gserviceaccount.com",
+			Role:   "roles/cloudkms.cryptoKeyEncrypterDecrypter",
+		},
+		{
+			Member: "serviceAccount:service-{project_number}@container-engine-robot.iam.gserviceaccount.com",
+			Role:   "roles/cloudkms.cryptoKeyEncrypterDecrypter",
+		},
+		{
+			Member: "serviceAccount:service-{project_number}@gcp-sa-artifactregistry.iam.gserviceaccount.com",
+			Role:   "roles/cloudkms.cryptoKeyEncrypterDecrypter",
+		},
+		{
+			Member: "serviceAccount:service-{project_number}@gcp-sa-pubsub.iam.gserviceaccount.com",
+			Role:   "roles/cloudkms.cryptoKeyEncrypterDecrypter",
+		},
+	})
 }
 
 // Checks environment creation with minimum required information.
@@ -245,7 +260,7 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer1(t *testing.T) {
 
 	kms := acctest.BootstrapKMSKeyWithPurposeInLocationAndName(t, "ENCRYPT_DECRYPT", "us-central1", "tf-bootstrap-composer1-key1")
 	pid := envvar.GetTestProjectFromEnv()
-	grantServiceAgentsRole(t, "service-", allComposerServiceAgents(), "roles/cloudkms.cryptoKeyEncrypterDecrypter")
+	bootstrapComposerServiceAgents(t)
 	envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, acctest.RandInt(t))
 	network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, acctest.RandInt(t))
 	subnetwork := network + "-1"
@@ -282,7 +297,7 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer2(t *testing.T) {
 
 	kms := acctest.BootstrapKMSKeyWithPurposeInLocationAndName(t, "ENCRYPT_DECRYPT", "us-central1", "tf-bootstrap-composer2-key1")
 	pid := envvar.GetTestProjectFromEnv()
-	grantServiceAgentsRole(t, "service-", allComposerServiceAgents(), "roles/cloudkms.cryptoKeyEncrypterDecrypter")
+	bootstrapComposerServiceAgents(t)
 	envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, acctest.RandInt(t))
 	network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, acctest.RandInt(t))
 	subnetwork := network + "-1"
@@ -970,14 +985,6 @@ func TestAccComposerEnvironment_fixPyPiPackages(t *testing.T) {
 	})
 }
 
-// This bootstraps the IAM roles needed for the service agents.
-func grantServiceAgentsRole(t *testing.T, prefix string, agentNames []string, role string) {
-	if acctest.BootstrapAllPSARole(t, prefix, agentNames, role) {
-		// Fail this test run because the policy needs time to reconcile.
-		t.Fatal("Stopping test because permissions were added.")
-	}
-}
-
 func testAccComposerEnvironmentDestroyProducer(t *testing.T) func(s *terraform.State) error {
 	return func(s *terraform.State) error {
 		config := acctest.GoogleProviderConfig(t)