From 0d5cf98b41ba9b6a9c05620af192d22b40e272ca Mon Sep 17 00:00:00 2001 From: gurusai-voleti Date: Wed, 29 Oct 2025 11:27:25 +0000 Subject: [PATCH 1/2] fix: customdiff --- .../storage/resource_storage_bucket_acl.go | 86 +++++++++++++------ .../resource_storage_bucket_acl_test.go | 16 +++- 2 files changed, 76 insertions(+), 26 deletions(-) diff --git a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go index 7ace4bc0cf9a..b1d7ce27ca47 100644 --- a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go +++ b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go @@ -59,35 +59,62 @@ func ResourceStorageBucketAcl() *schema.Resource { } func resourceStorageRoleEntityCustomizeDiff(_ context.Context, diff *schema.ResourceDiff, meta interface{}) error { - keys := diff.GetChangedKeysPrefix("role_entity") - if len(keys) < 1 { - return nil - } - count := diff.Get("role_entity.#").(int) - if count < 1 { + oldData, newData := diff.GetChange("role_entity") + + oldList, _ := oldData.([]interface{}) + newList, _ := newData.([]interface{}) + + if len(oldList) == 0 || len(newList) == 0 { return nil } - state := map[string]struct{}{} - conf := map[string]struct{}{} - for i := 0; i < count; i++ { - old, new := diff.GetChange(fmt.Sprintf("role_entity.%d", i)) - state[old.(string)] = struct{}{} - conf[new.(string)] = struct{}{} + + newSet := make(map[string]struct{}) + for _, item := range newList { + newSet[fmt.Sprint(item)] = struct{}{} } - if len(state) != len(conf) { - return nil + + visited := make(map[string]struct{}) + + var finalAcls []interface{} + + // Preserve order from oldList and handle removals, this will help avoid permadiff + // Iterate through the original list to maintain its order. + for _, item := range oldList { + key := fmt.Sprint(item) + if _, exists := newSet[key]; exists || isDefaultGcpAcl(key) { + visited[key] = struct{}{} + finalAcls = append(finalAcls, item) + } } - for k := range state { - if _, ok := conf[k]; !ok { - // project-owners- is explicitly stripped from the roles that this - // resource will delete - if strings.Contains(k, "OWNER:project-owners-") { - continue - } - return nil + + // Append any new additions + // Iterate through the new config to find items that weren't in the old list or newly added + for _, item := range newList { + key := fmt.Sprint(item) + if _, exists := visited[key]; !exists { + visited[key] = struct{}{} + finalAcls = append(finalAcls, item) } } - return diff.Clear("role_entity") + + // finaclAcls will be applied if any change in existing config otherwise not diff is displayed + if err := diff.SetNew("role_entity", finalAcls); err != nil { + return fmt.Errorf("error setting new role entities in CustomizeDiff: %w", err) + } + return nil +} + +func isDefaultGcpAcl(key string) bool { + if strings.HasPrefix(key, "OWNER:project-owners-") { + return true + } + if strings.HasPrefix(key, "OWNER:project-editors-") { + return true + } + if strings.HasPrefix(key, "READER:project-viewers-") { + return true + } + return false } type RoleEntity struct { @@ -173,6 +200,7 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er break } } + if alreadyInserted { log.Printf("[DEBUG]: pair %s-%s already exists, not trying to insert again\n", pair.Role, pair.Entity) continue @@ -293,7 +321,6 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er old_re_map[res.Entity] = res.Role } - for _, v := range new_re { pair, err := GetRoleEntityPair(v.(string)) @@ -315,7 +342,6 @@ func resourceStorageBucketAclUpdate(d *schema.ResourceData, meta interface{}) er return fmt.Errorf("Error updating ACL for bucket %s: %v", bucket, err) } } - for entity, role := range old_re_map { if entity == fmt.Sprintf("project-owners-%s", project) && role == "OWNER" { log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", role, entity) @@ -388,6 +414,16 @@ func resourceStorageBucketAclDelete(d *schema.ResourceData, meta interface{}) er continue } + if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { + log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) + continue + } + + if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { + log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) + continue + } + log.Printf("[DEBUG]: removing entity %s", res.Entity) err = config.NewStorageClient(userAgent).BucketAccessControls.Delete(bucket, res.Entity).Do() diff --git a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl_test.go b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl_test.go index a2a862e89204..b7981b49e1bc 100644 --- a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl_test.go +++ b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl_test.go @@ -65,7 +65,7 @@ func TestAccStorageBucketAcl_upgrade(t *testing.T) { }, { - Config: testGoogleStorageBucketsAclBasic2(bucketName), + Config: testGoogleStorageBucketsAclBasic2Owner(bucketName), Check: resource.ComposeTestCheckFunc( testAccCheckGoogleStorageBucketAcl(t, bucketName, roleEntityBasic2), testAccCheckGoogleStorageBucketAcl(t, bucketName, roleEntityBasic3_owner), @@ -309,6 +309,20 @@ resource "google_storage_bucket_acl" "acl" { `, bucketName, roleEntityOwners, roleEntityEditors, roleEntityViewers, roleEntityBasic2, roleEntityBasic3_owner) } +func testGoogleStorageBucketsAclBasic2Owner(bucketName string) string { + return fmt.Sprintf(` +resource "google_storage_bucket" "bucket" { + name = "%s" + location = "US" +} + +resource "google_storage_bucket_acl" "acl" { + bucket = google_storage_bucket.bucket.name + role_entity = ["%s", "%s", "%s", "%s", "%s", "%s"] +} +`, bucketName, roleEntityOwners, roleEntityEditors, roleEntityViewers, roleEntityBasic1, roleEntityBasic2, roleEntityBasic3_owner) +} + func testGoogleStorageBucketsAclBasicDelete(bucketName string) string { return fmt.Sprintf(` resource "google_storage_bucket" "bucket" { From 7d80028f92cff5a6f0a444b3c315552df946222a Mon Sep 17 00:00:00 2001 From: gurusai-voleti Date: Mon, 3 Nov 2025 05:25:31 +0000 Subject: [PATCH 2/2] update --- .../storage/resource_storage_bucket_acl.go | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go index b1d7ce27ca47..c0c56422a391 100644 --- a/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go +++ b/mmv1/third_party/terraform/services/storage/resource_storage_bucket_acl.go @@ -70,7 +70,7 @@ func resourceStorageRoleEntityCustomizeDiff(_ context.Context, diff *schema.Reso newSet := make(map[string]struct{}) for _, item := range newList { - newSet[fmt.Sprint(item)] = struct{}{} + newSet[item.(string)] = struct{}{} } visited := make(map[string]struct{}) @@ -80,7 +80,7 @@ func resourceStorageRoleEntityCustomizeDiff(_ context.Context, diff *schema.Reso // Preserve order from oldList and handle removals, this will help avoid permadiff // Iterate through the original list to maintain its order. for _, item := range oldList { - key := fmt.Sprint(item) + key := item.(string) if _, exists := newSet[key]; exists || isDefaultGcpAcl(key) { visited[key] = struct{}{} finalAcls = append(finalAcls, item) @@ -90,7 +90,7 @@ func resourceStorageRoleEntityCustomizeDiff(_ context.Context, diff *schema.Reso // Append any new additions // Iterate through the new config to find items that weren't in the old list or newly added for _, item := range newList { - key := fmt.Sprint(item) + key := item.(string) if _, exists := visited[key]; !exists { visited[key] = struct{}{} finalAcls = append(finalAcls, item) @@ -105,16 +105,9 @@ func resourceStorageRoleEntityCustomizeDiff(_ context.Context, diff *schema.Reso } func isDefaultGcpAcl(key string) bool { - if strings.HasPrefix(key, "OWNER:project-owners-") { - return true - } - if strings.HasPrefix(key, "OWNER:project-editors-") { - return true - } - if strings.HasPrefix(key, "READER:project-viewers-") { - return true - } - return false + return strings.HasPrefix(key, "OWNER:project-owners-") || + strings.HasPrefix(key, "OWNER:project-editors-") || + strings.HasPrefix(key, "READER:project-viewers-") } type RoleEntity struct { @@ -415,12 +408,12 @@ func resourceStorageBucketAclDelete(d *schema.ResourceData, meta interface{}) er } if res.Entity == fmt.Sprintf("project-editors-%s", project) && res.Role == "OWNER" { - log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) + log.Printf("[WARN]: Skipping %s-%s; not deleting editors ACL.", res.Role, res.Entity) continue } if res.Entity == fmt.Sprintf("project-viewers-%s", project) && res.Role == "READER" { - log.Printf("[WARN]: Skipping %s-%s; not deleting owner ACL.", res.Role, res.Entity) + log.Printf("[WARN]: Skipping %s-%s; not deleting viewers ACL.", res.Role, res.Entity) continue }