Move IAM bootstrap to its own file, improve error messages (#7424)

* Move IAM bootstrap to its own file, improve errors Also bootstrap roles/cloudbuild.builds.builder for cloudbuild service agent. * Re-add BootstrapConfig (accidentally deleted) * Fix wrong variable name * Bootstrap the role previously hardcoded for pubsub * Move error message back into bootstrap function This will dedup the code that calls this function. It now returns a boolean and sends the more useful error through t.Error. * Bootstrap the permissions for pubsub service agent * Bootstrap the role in the correct test * Fix formatting
GoogleCloudPlatform · Mar 14, 2023 · 6595653 · 6595653
1 parent f48da95
commit 6595653
Show file tree

Hide file tree

Showing 8 changed files with 212 additions and 174 deletions.
diff --git a/mmv1/products/cloudfunctions2/terraform.yaml b/mmv1/products/cloudfunctions2/terraform.yaml
@@ -10,7 +10,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
- 
+
 --- !ruby/object:Provider::Terraform::Config
 overrides: !ruby/object:Overrides::ResourceOverrides
   function:  !ruby/object:Overrides::Terraform::ResourceOverride
@@ -69,6 +69,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         test_vars_overrides:
           zip_path: "\"./test-fixtures/cloudfunctions2/function-source-eventarc-gcs.zip\""
           primary_resource_id: "\"terraform-test\""
+          policyChanged: "BootstrapPSARole(t, \"service-\", \"gcp-sa-pubsub\", \"roles/cloudkms.cryptoKeyEncrypterDecrypter\")"
         # ignore these fields during import step
         ignore_read_extra:
           - "build_config.0.source.0.storage_source.0.object"
@@ -87,6 +88,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         test_vars_overrides:
           zip_path: "\"./test-fixtures/cloudfunctions2/function-source-eventarc-gcs.zip\""
           primary_resource_id: "\"terraform-test\""
+          policyChanged: "BootstrapPSARole(t, \"service-\", \"gcp-sa-pubsub\", \"roles/cloudkms.cryptoKeyEncrypterDecrypter\")"
         # ignore these fields during import step
         ignore_read_extra:
           - "build_config.0.source.0.storage_source.0.object"
@@ -104,6 +106,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         test_vars_overrides:
           zip_path: "\"./test-fixtures/cloudfunctions2/function-source.zip\""
           location: "\"us-central1\""
+          policyChanged: "BootstrapPSARole(t, \"service-\", \"gcp-sa-pubsub\", \"roles/cloudkms.cryptoKeyEncrypterDecrypter\")"
         # ignore these fields during import step
         ignore_read_extra:
           - "build_config.0.source.0.storage_source.0.object"
@@ -121,6 +124,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         test_vars_overrides:
           zip_path: "\"./test-fixtures/cloudfunctions2/function-source.zip\""
           location: "\"us-central1\""
+          policyChanged: "BootstrapPSARole(t, \"service-\", \"gcp-sa-pubsub\", \"roles/cloudkms.cryptoKeyEncrypterDecrypter\")"
         # ignore these fields during import step
         ignore_read_extra:
           - "build_config.0.source.0.storage_source.0.object"
@@ -141,7 +145,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         # ignore these fields during import step
         ignore_read_extra:
           - "build_config.0.source.0.storage_source.0.object"
-          - "build_config.0.source.0.storage_source.0.bucket"            
+          - "build_config.0.source.0.storage_source.0.bucket"
     properties:
       name: !ruby/object:Overrides::Terraform::PropertyOverride
         custom_flatten: 'templates/terraform/custom_flatten/name_from_self_link.erb'
@@ -161,7 +165,7 @@ overrides: !ruby/object:Overrides::ResourceOverrides
       serviceConfig.serviceAccountEmail: !ruby/object:Overrides::Terraform::PropertyOverride
         default_from_api: true
       serviceConfig.secretVolumes.versions: !ruby/object:Overrides::Terraform::PropertyOverride
-        default_from_api: true        
+        default_from_api: true
       eventTrigger.pubsubTopic: !ruby/object:Overrides::Terraform::PropertyOverride
         default_from_api: true
       eventTrigger.serviceAccountEmail: !ruby/object:Overrides::Terraform::PropertyOverride

diff --git a/mmv1/products/healthcare/terraform.yaml b/mmv1/products/healthcare/terraform.yaml
@@ -60,7 +60,9 @@ overrides: !ruby/object:Overrides::ResourceOverrides
           pubsub_topic: "fhir-notifications"
           bq_dataset_name: "bq_example_dataset"
         test_vars_overrides:
-          policyChanged: 'BootstrapPSARoles(t, "gcp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})'
+          policyChanged: '
+            BootstrapPSARoles(t, "service-", "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 +112,9 @@ overrides: !ruby/object:Overrides::ResourceOverrides
           bq_dataset_name: "dicom_bq_ds"
           bq_table_name: "dicom_bq_tb"
         test_vars_overrides:
-          policyChanged: 'BootstrapPSARoles(t, "gcp-sa-healthcare", []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})'
+          policyChanged: '
+            BootstrapPSARoles(t, "service-", "gcp-sa-healthcare",
+                []string{"roles/bigquery.dataEditor", "roles/bigquery.jobUser"})'
     properties:
       creationTime: !ruby/object:Overrides::Terraform::PropertyOverride
         exclude: true
@@ -178,7 +182,11 @@ overrides: !ruby/object:Overrides::ResourceOverrides
       - !ruby/object:Provider::Terraform::Examples
         name: "healthcare_consent_store_basic"
         primary_resource_id: "my-consent"
-        primary_resource_name: "fmt.Sprintf(\"projects/%s/locations/%s/datasets/tf-test-my-dataset%s\",  GetTestProjectFromEnv(), GetTestRegionFromEnv(), context[\"random_suffix\"]) , fmt.Sprintf(\"tf-test-my-consent-store%s\", context[\"random_suffix\"])"
+        primary_resource_name: '
+          fmt.Sprintf("projects/%s/locations/%s/datasets/tf-test-my-dataset%s",
+            GetTestProjectFromEnv(), GetTestRegionFromEnv(),
+            context["random_suffix"]),
+          fmt.Sprintf("tf-test-my-consent-store%s", context["random_suffix"])'
         vars:
           dataset_id: "my-dataset"
           consent_id: "my-consent-store"

diff --git a/mmv1/third_party/terraform/tests/resource_cloudfunction2_function_test.go b/mmv1/third_party/terraform/tests/resource_cloudfunction2_function_test.go
@@ -57,18 +57,18 @@ resource "google_storage_bucket" "bucket" {
   location = "US"
   uniform_bucket_level_access = true
 }
- 
+
 resource "google_storage_bucket_object" "object" {
   name   = "function-source.zip"
   bucket = google_storage_bucket.bucket.name
   source = "%{zip_path}"
 }
- 
+
 resource "google_cloudfunctions2_function" "terraform-test2" {
   name = "tf-test-test-function%{random_suffix}"
   location = "us-central1"
   description = "a new function"
- 
+
   build_config {
     runtime = "nodejs12"
     entry_point = "helloHttp"
@@ -79,7 +79,7 @@ resource "google_cloudfunctions2_function" "terraform-test2" {
       }
     }
   }
- 
+
   service_config {
     max_instance_count  = 1
     available_memory    = "1536Mi"
@@ -96,18 +96,18 @@ resource "google_storage_bucket" "bucket" {
   location = "US"
   uniform_bucket_level_access = true
 }
- 
+
 resource "google_storage_bucket_object" "object" {
   name   = "function-source.zip"
   bucket = google_storage_bucket.bucket.name
   source = "%{zip_path}"
 }
- 
+
 resource "google_cloudfunctions2_function" "terraform-test2" {
   name = "tf-test-test-function%{random_suffix}"
   location = "us-central1"
   description = "an updated function"
- 
+
   build_config {
     runtime = "nodejs12"
     entry_point = "helloHttp"
@@ -118,7 +118,7 @@ resource "google_cloudfunctions2_function" "terraform-test2" {
       }
     }
   }
- 
+
   service_config {
     max_instance_count  = 1
     available_memory    = "1536Mi"
@@ -135,18 +135,18 @@ resource "google_storage_bucket" "bucket" {
   location = "US"
   uniform_bucket_level_access = true
 }
- 
+
 resource "google_storage_bucket_object" "object" {
   name   = "function-source.zip"
   bucket = google_storage_bucket.bucket.name
   source = "%{zip_path}"
 }
- 
+
 resource "google_cloudfunctions2_function" "terraform-test2" {
   name = "tf-test-test-function%{random_suffix}"
   location = "us-west1"
   description = "function test"
- 
+
   build_config {
     runtime = "nodejs16"
     entry_point = "helloHttp"
@@ -160,7 +160,7 @@ resource "google_cloudfunctions2_function" "terraform-test2" {
       }
     }
   }
- 
+
   service_config {
     max_instance_count  = 5
     min_instance_count = 1
@@ -183,6 +183,10 @@ func TestAccCloudFunctions2Function_fullUpdate(t *testing.T) {
 		"random_suffix": RandString(t, 10),
 	}
 
+	if BootstrapPSARole(t, "service-", "gcp-sa-pubsub", "roles/cloudkms.cryptoKeyEncrypterDecrypter") {
+		t.Fatal("Stopping the test because a binding was added.")
+	}
+
 	VcrTest(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    TestAccProviders,
@@ -212,7 +216,7 @@ resource "google_storage_bucket" "source-bucket" {
   location = "US"
   uniform_bucket_level_access = true
 }
- 
+
 resource "google_storage_bucket_object" "object" {
   name   = "function-source.zip"
   bucket = google_storage_bucket.source-bucket.name
@@ -262,7 +266,7 @@ resource "google_cloudfunctions2_function" "function" {
   name = "tf-test-gcf-function%{random_suffix}"
   location = "us-central1"
   description = "a new function"
- 
+
   build_config {
     runtime     = "nodejs12"
     entry_point = "entryPoint" # Set the entry point in the code
@@ -276,7 +280,7 @@ resource "google_cloudfunctions2_function" "function" {
       }
     }
   }
- 
+
   service_config {
     max_instance_count  = 3
     min_instance_count = 1

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
@@ -27,6 +27,16 @@ func init() {
 	})
 }
 
+func allComposerServiceAgents() []string {
+	return []string{
+		"cloudcomposer-accounts",
+		"compute-system",
+		"container-engine-robot",
+		"gcp-sa-artifactregistry",
+		"gcp-sa-pubsub",
+	}
+}
+
 func TestComposerImageVersionDiffSuppress(t *testing.T) {
 	t.Parallel()
 
@@ -308,6 +318,9 @@ func TestAccComposerEnvironment_withWebServerConfig(t *testing.T) {
 	network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t))
 	subnetwork := network + "-1"
 
+
+	grantServiceAgentsRole(t, "service-", []string{"gcp-sa-cloudbuild"}, "roles/cloudbuild.builds.builder")
+
 	VcrTest(t, resource.TestCase{
 		PreCheck:		 func() { testAccPreCheck(t) },
 		Providers:		TestAccProviders,
@@ -342,7 +355,7 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer1(t *testing.T) {
 
 	kms := BootstrapKMSKeyInLocation(t, "us-central1")
 	pid := GetTestProjectFromEnv()
-	grantEncrypterDecrypterRoleToServiceAgents(t)
+	grantServiceAgentsRole(t, "service-", allComposerServiceAgents(), "roles/cloudkms.cryptoKeyEncrypterDecrypter")
 	envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t))
 	network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t))
 	subnetwork := network + "-1"
@@ -378,7 +391,7 @@ func TestAccComposerEnvironment_withEncryptionConfigComposer2(t *testing.T) {
 
 	kms := BootstrapKMSKeyInLocation(t, "us-central1")
 	pid := GetTestProjectFromEnv()
-	grantEncrypterDecrypterRoleToServiceAgents(t)
+	grantServiceAgentsRole(t, "service-", allComposerServiceAgents(), "roles/cloudkms.cryptoKeyEncrypterDecrypter")
 	envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, RandInt(t))
 	network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, RandInt(t))
 	subnetwork := network + "-1"
@@ -962,19 +975,11 @@ 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",
-	}
-	role := "roles/cloudkms.cryptoKeyEncrypterDecrypter"
-	if policyChanged := BootstrapAllPSARole(t, serviceAgents, role); policyChanged {
+// This bootstraps the IAM roles needed for the service agents.
+func grantServiceAgentsRole(t *testing.T, prefix string, agentNames []string, role string) {
+	if BootstrapAllPSARole(t, prefix, agentNames, role) {
 		// 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)
+		t.Fatal("Stopping test because permissions were added.")
 	}
 }
 

diff --git a/mmv1/third_party/terraform/tests/resource_pubsub_topic_test.go b/mmv1/third_party/terraform/tests/resource_pubsub_topic_test.go
@@ -45,6 +45,10 @@ func TestAccPubsubTopic_cmek(t *testing.T) {
 	kms := BootstrapKMSKey(t)
 	topicName := fmt.Sprintf("tf-test-%s", RandString(t, 10))
 
+	if BootstrapPSARole(t, "service-", "gcp-sa-pubsub", "roles/cloudkms.cryptoKeyEncrypterDecrypter") {
+		t.Fatal("Stopping the test because a role was added to the policy.")
+	}
+
 	VcrTest(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    TestAccProviders,