Fix the regression of handling deleted IAM members in google_bigquery…

…_dataset_iam* (#11898) (#19857) [upstream:0b8a8585660d86b3ee6494caa91dfa737f3ff507] Signed-off-by: Modular Magician <[email protected]>
hashicorp · Oct 15, 2024 · db6e971 · db6e971
1 parent fc8e5fb
commit db6e971
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 14 deletions.
diff --git a/.changelog/11898.txt b/.changelog/11898.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+bigquery: fixed a regression that caused `google_bigquery_dataset_iam_*` resources to attempt to set deleted IAM members, thereby triggering an API error 
+```
diff --git a/google/services/bigquery/iam_bigquery_dataset.go b/google/services/bigquery/iam_bigquery_dataset.go
@@ -191,7 +191,7 @@ func policyToAccess(policy *cloudresourcemanager.Policy) ([]map[string]interface
 		}
 		for _, member := range binding.Members {
 			// Do not append any deleted members
-			if strings.HasPrefix(member, "deleted:") {
+			if strings.HasPrefix(member, "iamMember:deleted:") {
 				continue
 			}
 			access := map[string]interface{}{
@@ -213,7 +213,7 @@ func policyToAccess(policy *cloudresourcemanager.Policy) ([]map[string]interface
 // Dataset access uses different member types to identify groups, domains, etc.
 // these types are used as keys in the access JSON payload
 func iamMemberToAccess(member string) (string, string, error) {
-	if strings.HasPrefix(member, "deleted:") {
+	if strings.HasPrefix(member, "iamMember:deleted:") {
 		return "", "", fmt.Errorf("BigQuery Dataset IAM member is deleted: %s", member)
 	}
 	pieces := strings.SplitN(member, ":", 2)

diff --git a/google/services/bigquery/iam_bigquery_member_dataset.go b/google/services/bigquery/iam_bigquery_member_dataset.go
@@ -233,7 +233,7 @@ func policyToAccessForIamMember(policy *cloudresourcemanager.Policy) ([]map[stri
 		}
 		for _, member := range binding.Members {
 			// Do not append any deleted members
-			if strings.HasPrefix(member, "deleted:") {
+			if strings.HasPrefix(member, "iamMember:deleted:") {
 				continue
 			}
 			access := map[string]interface{}{
@@ -255,7 +255,7 @@ func policyToAccessForIamMember(policy *cloudresourcemanager.Policy) ([]map[stri
 // Dataset access uses different member types to identify groups, domains, etc.
 // these types are used as keys in the access JSON payload
 func iamMemberToAccessForIamMember(member string) (string, string, error) {
-	if strings.HasPrefix(member, "deleted:") {
+	if strings.HasPrefix(member, "iamMember:deleted:") {
 		return "", "", fmt.Errorf("BigQuery Dataset IAM member is deleted: %s", member)
 	}
 	pieces := strings.SplitN(member, ":", 2)

diff --git a/google/services/bigquery/resource_bigquery_dataset_iam_member_test.go b/google/services/bigquery/resource_bigquery_dataset_iam_member_test.go
@@ -5,7 +5,7 @@ package bigquery_test
 import (
 	"fmt"
 	"reflect"
-	"strings"
+	"regexp"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
@@ -116,6 +116,48 @@ func TestAccBigqueryDatasetIamMember_iamMember(t *testing.T) {
 	})
 }
 
+func TestAccBigqueryDatasetIamMember_withDeletedServiceAccount(t *testing.T) {
+	t.Parallel()
+
+	datasetID := fmt.Sprintf("tf_test_%s", acctest.RandString(t, 10))
+	serviceAccountID := fmt.Sprintf("tf-test-%s", acctest.RandString(t, 10))
+	serviceAccountEmail := fmt.Sprintf("%s@%s.iam.gserviceaccount.com", serviceAccountID, envvar.GetTestProjectFromEnv())
+
+	expected := map[string]interface{}{
+		"role":        "roles/viewer",
+		"userByEmail": serviceAccountEmail,
+	}
+
+	acctest.VcrTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.AccTestPreCheck(t) },
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccBigqueryDatasetIamMember_withServiceAccount(datasetID, serviceAccountID),
+				Check:  testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected),
+			},
+			{
+				// No change from Step 1 except for hard-coding the service account email to remove the explicit resource reference in preparation for the service accoutn deletion.
+				Config: testAccBigqueryDatasetIamMember_withServiceAccountEmail(datasetID, serviceAccountID, serviceAccountEmail),
+				Check:  testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected),
+			},
+			{
+				// Delete the service account but keep the rest the same.
+				// The service account will show up with a "deleted:serviceAccount:" prefix and a "?uid=" suffix when getting dataset access from now on.
+				// The plan should show an attempt to recreate the IAM member resource because the dataset access no longer contains the original account email so there is a diff.
+				Config:             testAccBigqueryDatasetIamMember_withServiceAccountDeleted(datasetID, serviceAccountEmail),
+				ExpectNonEmptyPlan: true,
+			},
+			{
+				// Apply the same plan as the previous step.
+				// It should fail because the specified account has now been deleted, so the account email is invalid.
+				Config:      testAccBigqueryDatasetIamMember_withServiceAccountDeleted(datasetID, serviceAccountEmail),
+				ExpectError: regexp.MustCompile(fmt.Sprintf(".+Service account %s does not exist.+", serviceAccountEmail)),
+			},
+		},
+	})
+}
+
 func testAccCheckBigQueryDatasetIamMemberPresent(t *testing.T, n string, expected map[string]interface{}) resource.TestCheckFunc {
 	return testAccCheckBigQueryDatasetIamMember(t, n, expected, true)
 }
@@ -148,15 +190,6 @@ func testAccCheckBigQueryDatasetIamMember(t *testing.T, n string, expected map[s
 		}
 		access := ds["access"].([]interface{})
 		for _, a := range access {
-			if aMap, ok := a.(map[string]interface{}); ok {
-				if iamMember, ok := aMap["iamMember"].(string); ok {
-					// The iam account may have been deleted but the binding may be present for the dataset.
-					// This case is supposed to always throw an error.
-					if strings.HasPrefix(iamMember, "deleted:") {
-						return fmt.Errorf("Found deleted service account: %s", iamMember)
-					}
-				}
-			}
 			if reflect.DeepEqual(a, expected) {
 				if !expectPresent {
 					return fmt.Errorf("Found access %+v, expected not present", expected)
@@ -260,3 +293,53 @@ resource "google_iam_workload_identity_pool_provider" "wif_provider" {
 }
 `, datasetID, wifIDs, wifIDs)
 }
+
+func testAccBigqueryDatasetIamMember_withServiceAccount(datasetID, serviceAccountID string) string {
+	return fmt.Sprintf(`
+resource "google_bigquery_dataset" "dataset" {
+  dataset_id = "%s"
+}
+
+resource "google_service_account" "sa" {
+  account_id = "%s"
+}
+
+resource "google_bigquery_dataset_iam_member" "access" {
+  dataset_id = google_bigquery_dataset.dataset.dataset_id
+  role       = "roles/viewer"
+  member     = "serviceAccount:${google_service_account.sa.email}"
+}
+`, datasetID, serviceAccountID)
+}
+
+func testAccBigqueryDatasetIamMember_withServiceAccountEmail(datasetID, serviceAccountID, serviceAccountEmail string) string {
+	return fmt.Sprintf(`
+resource "google_bigquery_dataset" "dataset" {
+  dataset_id = "%s"
+}
+
+resource "google_service_account" "sa" {
+  account_id = "%s"
+}
+
+resource "google_bigquery_dataset_iam_member" "access" {
+  dataset_id = google_bigquery_dataset.dataset.dataset_id
+  role       = "roles/viewer"
+  member     = "serviceAccount:%s"
+}
+`, datasetID, serviceAccountID, serviceAccountEmail)
+}
+
+func testAccBigqueryDatasetIamMember_withServiceAccountDeleted(datasetID, serviceAccountEmail string) string {
+	return fmt.Sprintf(`
+resource "google_bigquery_dataset" "dataset" {
+  dataset_id = "%s"
+}
+
+resource "google_bigquery_dataset_iam_member" "access" {
+  dataset_id = google_bigquery_dataset.dataset.dataset_id
+  role       = "roles/viewer"
+  member     = "serviceAccount:%s"
+}
+`, datasetID, serviceAccountEmail)
+}