From d766989bef75ead3973cba06fee091c48f850f4a Mon Sep 17 00:00:00 2001 From: sachin purohit Date: Mon, 30 Sep 2024 08:52:54 -0700 Subject: [PATCH] =?UTF-8?q?fixing=20deletion=20for=20iam=20members=20after?= =?UTF-8?q?=20destroy=20is=20called=20on=20bigquery=5Fd=E2=80=A6=20(#11853?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../bigquery/iam_bigquery_member_dataset.go | 6 +- ...source_bigquery_dataset_iam_member_test.go | 85 +++++++++++++++++-- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/mmv1/third_party/terraform/services/bigquery/iam_bigquery_member_dataset.go b/mmv1/third_party/terraform/services/bigquery/iam_bigquery_member_dataset.go index 99d94949ab07..a4b58ae67ca8 100644 --- a/mmv1/third_party/terraform/services/bigquery/iam_bigquery_member_dataset.go +++ b/mmv1/third_party/terraform/services/bigquery/iam_bigquery_member_dataset.go @@ -127,9 +127,11 @@ func mergeAccess(newAccess []map[string]interface{}, currAccess []interface{}) [ mergedAccess = append(mergedAccess, newAccess...) for _, item := range currAccess { - // Type assertion to check if it's amap if itemMap, ok := item.(map[string]interface{}); ok { - mergedAccess = append(mergedAccess, itemMap) + // Check if the item has a "dataset" key + if _, ok := itemMap["dataset"]; ok { + mergedAccess = append(mergedAccess, itemMap) + } } } return mergedAccess diff --git a/mmv1/third_party/terraform/services/bigquery/resource_bigquery_dataset_iam_member_test.go b/mmv1/third_party/terraform/services/bigquery/resource_bigquery_dataset_iam_member_test.go index 9591dfbf787d..099bd43cbad2 100644 --- a/mmv1/third_party/terraform/services/bigquery/resource_bigquery_dataset_iam_member_test.go +++ b/mmv1/third_party/terraform/services/bigquery/resource_bigquery_dataset_iam_member_test.go @@ -2,11 +2,16 @@ package bigquery_test import ( "fmt" + "reflect" + "strings" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-provider-google/google/acctest" "github.com/hashicorp/terraform-provider-google/google/envvar" + "github.com/hashicorp/terraform-provider-google/google/tpgresource" + transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport" ) func TestAccBigqueryDatasetIamMember_afterDatasetCreation(t *testing.T) { @@ -17,7 +22,7 @@ func TestAccBigqueryDatasetIamMember_afterDatasetCreation(t *testing.T) { authDatasetID := fmt.Sprintf("tf_test_%s", acctest.RandString(t, 10)) saID := fmt.Sprintf("tf-test-%s", acctest.RandString(t, 10)) - expected := map[string]interface{}{ + expected_auth := map[string]interface{}{ "dataset": map[string]interface{}{ "dataset": map[string]interface{}{ "projectId": projectID, @@ -27,18 +32,27 @@ func TestAccBigqueryDatasetIamMember_afterDatasetCreation(t *testing.T) { }, } + expected_sa := map[string]interface{}{ + "role": "roles/viewer", + "userByEmail": fmt.Sprintf("%s@%s.iam.gserviceaccount.com", saID, envvar.GetTestProjectFromEnv()), + } + acctest.VcrTest(t, resource.TestCase{ PreCheck: func() { acctest.AccTestPreCheck(t) }, ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t), Steps: []resource.TestStep{ { Config: testAccBigqueryDatasetIamMember_afterDatasetAccessCreation(projectID, datasetID, authDatasetID, saID), - Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.dataset", expected), + Check: testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected_auth), }, { - // For iam_member to be non-authoritative, we want access block to be present after destroy + // For iam_member to be non-authoritative, we want authorized datasets to be present after destroy, + // but the iam resources have to be deleted Config: testAccBigqueryDatasetIamMember_destroy(datasetID), - Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.dataset", expected), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected_auth), + testAccCheckBigQueryDatasetIamMemberAbsent(t, "google_bigquery_dataset.dataset", expected_sa), + ), }, }, }) @@ -61,12 +75,12 @@ func TestAccBigqueryDatasetIamMember_serviceAccount(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccBigqueryDatasetIamMember_serviceAccount(datasetID, saID), - Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.dataset", expected), + Check: testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected), }, { // Destroy step instead of CheckDestroy so we can check the access is removed without deleting the dataset Config: testAccBigqueryDatasetIamMember_destroy(datasetID), - Check: testAccCheckBigQueryDatasetAccessAbsent(t, "google_bigquery_dataset.dataset", expected), + Check: testAccCheckBigQueryDatasetIamMemberAbsent(t, "google_bigquery_dataset.dataset", expected), }, }, }) @@ -89,17 +103,72 @@ func TestAccBigqueryDatasetIamMember_iamMember(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccBigqueryDatasetIamMember_iamMember(datasetID, wifIDs), - Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.dataset", expected), + Check: testAccCheckBigQueryDatasetIamMemberPresent(t, "google_bigquery_dataset.dataset", expected), }, { // For iam_member to be non-authoritative, we want access block to be present after destroy Config: testAccBigqueryDatasetIamMember_destroy(datasetID), - Check: testAccCheckBigQueryDatasetAccessPresent(t, "google_bigquery_dataset.dataset", expected), + Check: testAccCheckBigQueryDatasetIamMemberAbsent(t, "google_bigquery_dataset.dataset", expected), }, }, }) } +func testAccCheckBigQueryDatasetIamMemberPresent(t *testing.T, n string, expected map[string]interface{}) resource.TestCheckFunc { + return testAccCheckBigQueryDatasetIamMember(t, n, expected, true) +} + +func testAccCheckBigQueryDatasetIamMemberAbsent(t *testing.T, n string, expected map[string]interface{}) resource.TestCheckFunc { + return testAccCheckBigQueryDatasetIamMember(t, n, expected, false) +} + +func testAccCheckBigQueryDatasetIamMember(t *testing.T, n string, expected map[string]interface{}, expectPresent bool) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Not found: %s", n) + } + + config := acctest.GoogleProviderConfig(t) + url, err := tpgresource.ReplaceVarsForTest(config, rs, "{{BigQueryBasePath}}projects/{{project}}/datasets/{{dataset_id}}") + if err != nil { + return err + } + + ds, err := transport_tpg.SendRequest(transport_tpg.SendRequestOptions{ + Config: config, + Method: "GET", + RawURL: url, + UserAgent: config.UserAgent, + }) + if err != nil { + return err + } + 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) + } + return nil + } + } + if expectPresent { + return fmt.Errorf("Did not find access %+v, expected present", expected) + } + return nil + } +} + func testAccBigqueryDatasetIamMember_destroy(datasetID string) string { return fmt.Sprintf(` resource "google_bigquery_dataset" "dataset" {