From 8642107515afc675656b89ddab925b891c83f5dd Mon Sep 17 00:00:00 2001 From: Stijn Simons Date: Mon, 7 Sep 2026 09:37:19 +0200 Subject: [PATCH] Fix reordering of role permissions (inc. external) --- .../resource_cloudstack_role_permission.go | 91 +++--- ...esource_cloudstack_role_permission_test.go | 269 ++++++++++++++++++ 2 files changed, 321 insertions(+), 39 deletions(-) diff --git a/cloudstack/resource_cloudstack_role_permission.go b/cloudstack/resource_cloudstack_role_permission.go index 1769f8d0..c6f73d03 100644 --- a/cloudstack/resource_cloudstack_role_permission.go +++ b/cloudstack/resource_cloudstack_role_permission.go @@ -118,30 +118,31 @@ func resourceCloudStackRolePermissionRead(d *schema.ResourceData, meta interface return fmt.Errorf("Error listing Role Permissions: %s", err) } - permissionsByID := make(map[string]*cloudstack.RolePermission) - for _, rp := range rolePermissions { - permissionsByID[rp.Id] = rp - } - var missing bool - var readPermissions []interface{} + var missingPermissions []interface{} used := make(map[string]bool) + desiredPermissions := rolePermissionSpecs(d.Get("permission").([]interface{})) + matchedPermissions := matchCloudStackRolePermissions(rolePermissions, desiredPermissions) - for _, desired := range rolePermissionSpecs(d.Get("permission").([]interface{})) { - var rp *cloudstack.RolePermission - if desired.ID != "" { - rp = permissionsByID[desired.ID] - } - if rp == nil { - rp = findMatchingRolePermission(rolePermissions, desired, used) - } + for i, desired := range desiredPermissions { + rp := matchedPermissions[i] if rp == nil { missing = true - readPermissions = append(readPermissions, rolePermissionState(desired)) + missingPermissions = append(missingPermissions, rolePermissionState(desired)) continue } used[rp.Id] = true + } + + // Keep managed permissions in the order returned by CloudStack. Otherwise an + // out-of-band reorder is hidden by refresh and Terraform cannot restore the + // order declared in the configuration. + readPermissions := make([]interface{}, 0, len(used)+len(missingPermissions)) + for _, rp := range rolePermissions { + if !used[rp.Id] { + continue + } readPermissions = append(readPermissions, rolePermissionState(rolePermissionSpec{ ID: rp.Id, Rule: rp.Rule, @@ -149,6 +150,7 @@ func resourceCloudStackRolePermissionRead(d *schema.ResourceData, meta interface Description: rp.Description, })) } + readPermissions = append(readPermissions, missingPermissions...) if err := d.Set("permission", readPermissions); err != nil { return fmt.Errorf("Error setting Role Permissions: %s", err) @@ -242,28 +244,13 @@ func reconcileCloudStackRolePermissions(d *schema.ResourceData, meta interface{} rolePermissionsByID[rp.Id] = rp } - used := make(map[string]bool) - deleted := make(map[string]bool) managedIDs := make([]string, 0) managedIDSet := make(map[string]bool) + desiredPermissions := rolePermissionSpecs(d.Get("permission").([]interface{})) + matchedPermissions := matchCloudStackRolePermissions(rolePermissions, desiredPermissions) - for _, desired := range rolePermissionSpecs(d.Get("permission").([]interface{})) { - rp := rolePermissionsByID[desired.ID] - if rp != nil && (rp.Rule != desired.Rule || rp.Description != desired.Description) { - if exactMatch := findMatchingRolePermission(rolePermissions, desired, used); exactMatch != nil { - rp = exactMatch - } else { - if err := deleteCloudStackRolePermission(cs, rp.Id); err != nil { - return err - } - deleted[rp.Id] = true - used[rp.Id] = true - rp = nil - } - } else if rp == nil { - rp = findMatchingRolePermission(rolePermissions, desired, used) - } - + for i, desired := range desiredPermissions { + rp := matchedPermissions[i] if rp == nil { rp, err = createCloudStackRolePermission(cs, roleID, desired) if err != nil { @@ -275,7 +262,6 @@ func reconcileCloudStackRolePermissions(d *schema.ResourceData, meta interface{} } } - used[rp.Id] = true managedIDs = append(managedIDs, rp.Id) managedIDSet[rp.Id] = true } @@ -289,17 +275,16 @@ func reconcileCloudStackRolePermissions(d *schema.ResourceData, meta interface{} if d.Get("authoritative").(bool) { for _, rp := range rolePermissions { - if managedIDSet[rp.Id] || deleted[rp.Id] { + if managedIDSet[rp.Id] { continue } if err := deleteCloudStackRolePermission(cs, rp.Id); err != nil { return err } - deleted[rp.Id] = true } } else { for oldID := range oldManagedIDs { - if managedIDSet[oldID] || deleted[oldID] { + if managedIDSet[oldID] { continue } if rolePermissionsByID[oldID] == nil { @@ -308,7 +293,6 @@ func reconcileCloudStackRolePermissions(d *schema.ResourceData, meta interface{} if err := deleteCloudStackRolePermission(cs, oldID); err != nil { return err } - deleted[oldID] = true } } @@ -441,6 +425,35 @@ func findMatchingRolePermission(rolePermissions []*cloudstack.RolePermission, de return nil } +func matchCloudStackRolePermissions(rolePermissions []*cloudstack.RolePermission, desiredPermissions []rolePermissionSpec) []*cloudstack.RolePermission { + permissionsByID := make(map[string]*cloudstack.RolePermission, len(rolePermissions)) + for _, rp := range rolePermissions { + permissionsByID[rp.Id] = rp + } + + matchedPermissions := make([]*cloudstack.RolePermission, len(desiredPermissions)) + used := make(map[string]bool) + + for i, desired := range desiredPermissions { + var rp *cloudstack.RolePermission + if desired.ID != "" { + candidate := permissionsByID[desired.ID] + if candidate != nil && !used[candidate.Id] && candidate.Rule == desired.Rule && candidate.Description == desired.Description { + rp = candidate + } + } + if rp == nil { + rp = findMatchingRolePermission(rolePermissions, desired, used) + } + if rp != nil { + used[rp.Id] = true + } + matchedPermissions[i] = rp + } + + return matchedPermissions +} + func rolePermissionLock(roleID string) *sync.Mutex { lock, _ := rolePermissionLocks.LoadOrStore(roleID, &sync.Mutex{}) return lock.(*sync.Mutex) diff --git a/cloudstack/resource_cloudstack_role_permission_test.go b/cloudstack/resource_cloudstack_role_permission_test.go index 18eeee63..6196a05a 100644 --- a/cloudstack/resource_cloudstack_role_permission_test.go +++ b/cloudstack/resource_cloudstack_role_permission_test.go @@ -61,6 +61,8 @@ func TestAccCloudStackRolePermission_basic(t *testing.T) { } func TestAccCloudStackRolePermission_orderAfterRecreate(t *testing.T) { + var wildcardRuleID string + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, @@ -71,6 +73,7 @@ func TestAccCloudStackRolePermission_orderAfterRecreate(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"listZones", "*"}), + testAccCaptureCloudStackRolePermissionID("cloudstack_role_permission.foo", "*", &wildcardRuleID), ), }, { @@ -78,13 +81,131 @@ func TestAccCloudStackRolePermission_orderAfterRecreate(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"*"}), + testAccCheckCloudStackRolePermissionID("cloudstack_role_permission.foo", "*", &wildcardRuleID), ), }, + { + Config: testAccCloudStackRolePermission_orderWithoutSpecificRule, + PlanOnly: true, + }, { Config: testAccCloudStackRolePermission_orderWithSpecificRule, Check: resource.ComposeTestCheckFunc( testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"listZones", "*"}), + testAccCheckCloudStackRolePermissionID("cloudstack_role_permission.foo", "*", &wildcardRuleID), + ), + }, + { + Config: testAccCloudStackRolePermission_orderWithSpecificRule, + PlanOnly: true, + }, + }, + }) +} + +func TestMatchCloudStackRolePermissions_shiftedIDsAfterRemoval(t *testing.T) { + rolePermissions := []*cloudstack.RolePermission{ + {Id: "detach-id", Rule: "detachIso", Permission: "allow"}, + {Id: "start-id", Rule: "startSystemVm", Permission: "allow"}, + {Id: "wildcard-id", Rule: "*", Permission: "deny"}, + } + desiredPermissions := []rolePermissionSpec{ + {ID: "removed-attach-id", Rule: "detachIso", Permission: "allow"}, + {ID: "detach-id", Rule: "startSystemVm", Permission: "allow"}, + {ID: "start-id", Rule: "*", Permission: "deny"}, + } + + matchedPermissions := matchCloudStackRolePermissions(rolePermissions, desiredPermissions) + assertRolePermissionIDs(t, matchedPermissions, []string{"detach-id", "start-id", "wildcard-id"}) +} + +func TestMatchCloudStackRolePermissions_shiftedIDsAfterInsertion(t *testing.T) { + rolePermissions := []*cloudstack.RolePermission{ + {Id: "detach-id", Rule: "detachIso", Permission: "allow"}, + {Id: "start-id", Rule: "startSystemVm", Permission: "allow"}, + } + desiredPermissions := []rolePermissionSpec{ + {ID: "detach-id", Rule: "attachIso", Permission: "allow"}, + {ID: "start-id", Rule: "detachIso", Permission: "allow"}, + {Rule: "startSystemVm", Permission: "allow"}, + } + + matchedPermissions := matchCloudStackRolePermissions(rolePermissions, desiredPermissions) + assertRolePermissionIDs(t, matchedPermissions, []string{"", "detach-id", "start-id"}) +} + +func assertRolePermissionIDs(t *testing.T, rolePermissions []*cloudstack.RolePermission, expectedIDs []string) { + t.Helper() + + if len(rolePermissions) != len(expectedIDs) { + t.Fatalf("Expected %d matched Role Permissions, got %d", len(expectedIDs), len(rolePermissions)) + } + + for i, expectedID := range expectedIDs { + var actualID string + if rolePermissions[i] != nil { + actualID = rolePermissions[i].Id + } + if actualID != expectedID { + t.Errorf("Expected matched Role Permission %d to have ID %q, got %q", i, expectedID, actualID) + } + } +} + +func TestAccCloudStackRolePermission_orderChange(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckCloudStackRolePermissionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccCloudStackRolePermission_orderWithSpecificRule, + // The check deliberately changes the API-side order to verify drift detection. + ExpectNonEmptyPlan: true, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"listZones", "*"}), + testAccReorderCloudStackRolePermissions("cloudstack_role_permission.foo", []string{"*", "listZones"}), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"*", "listZones"}), + ), + }, + { + Config: testAccCloudStackRolePermission_orderWithSpecificRule, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"listZones", "*"}), + ), + }, + { + Config: testAccCloudStackRolePermission_orderReversed, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"*", "listZones"}), + ), + }, + }, + }) +} + +func TestAccCloudStackRolePermission_orderChangeThreeRules(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckCloudStackRolePermissionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccCloudStackRolePermission_orderThreeRules, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"listZones", "listVirtualMachines", "*"}), + ), + }, + { + Config: testAccCloudStackRolePermission_orderThreeRulesReordered, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"), + testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo", []string{"*", "listZones", "listVirtualMachines"}), ), }, }, @@ -168,6 +289,81 @@ func testAccCheckCloudStackRolePermissionOrder(n string, rules []string) resourc } } +func testAccReorderCloudStackRolePermissions(n string, rules []string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rolePermissions, err := testAccListCloudStackRolePermissions(s, n) + if err != nil { + return err + } + + ruleIDs := make([]string, 0, len(rules)) + for _, rule := range rules { + var ruleID string + for _, rp := range rolePermissions { + if rp.Rule == rule { + ruleID = rp.Id + break + } + } + if ruleID == "" { + return fmt.Errorf("Role Permission rule %q not found", rule) + } + ruleIDs = append(ruleIDs, ruleID) + } + + cs := testAccProvider.Meta().(*cloudstack.CloudStackClient) + p := cs.Role.NewUpdateRolePermissionParams(s.RootModule().Resources[n].Primary.Attributes["role_id"]) + p.SetRuleorder(ruleIDs) + if _, err := cs.Role.UpdateRolePermission(p); err != nil { + return fmt.Errorf("Error ordering Role Permissions: %s", err) + } + + return nil + } +} + +func testAccCaptureCloudStackRolePermissionID(n, rule string, ruleID *string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rolePermissions, err := testAccListCloudStackRolePermissions(s, n) + if err != nil { + return err + } + + for _, rp := range rolePermissions { + if rp.Rule == rule { + *ruleID = rp.Id + return nil + } + } + + return fmt.Errorf("Role Permission rule %q not found", rule) + } +} + +func testAccCheckCloudStackRolePermissionID(n, rule string, expectedRuleID *string) resource.TestCheckFunc { + return func(s *terraform.State) error { + if *expectedRuleID == "" { + return fmt.Errorf("No expected Role Permission ID is set for rule %q", rule) + } + + rolePermissions, err := testAccListCloudStackRolePermissions(s, n) + if err != nil { + return err + } + + for _, rp := range rolePermissions { + if rp.Rule == rule { + if rp.Id != *expectedRuleID { + return fmt.Errorf("Expected Role Permission rule %q to keep ID %s, got %s", rule, *expectedRuleID, rp.Id) + } + return nil + } + } + + return fmt.Errorf("Role Permission rule %q not found", rule) + } +} + func testAccCreateCloudStackRolePermission(n, rule, permission, description string, ruleID *string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -344,6 +540,79 @@ resource "cloudstack_role_permission" "foo" { } ` +const testAccCloudStackRolePermission_orderReversed = ` +resource "cloudstack_role" "foo" { + name = "terraform-role" + type = "User" +} + +resource "cloudstack_role_permission" "foo" { + role_id = cloudstack_role.foo.id + + permission { + rule = "*" + permission = "deny" + } + + permission { + rule = "listZones" + permission = "allow" + } +} +` + +const testAccCloudStackRolePermission_orderThreeRules = ` +resource "cloudstack_role" "foo" { + name = "terraform-role" + type = "User" +} + +resource "cloudstack_role_permission" "foo" { + role_id = cloudstack_role.foo.id + + permission { + rule = "listZones" + permission = "allow" + } + + permission { + rule = "listVirtualMachines" + permission = "allow" + } + + permission { + rule = "*" + permission = "deny" + } +} +` + +const testAccCloudStackRolePermission_orderThreeRulesReordered = ` +resource "cloudstack_role" "foo" { + name = "terraform-role" + type = "User" +} + +resource "cloudstack_role_permission" "foo" { + role_id = cloudstack_role.foo.id + + permission { + rule = "*" + permission = "deny" + } + + permission { + rule = "listZones" + permission = "allow" + } + + permission { + rule = "listVirtualMachines" + permission = "allow" + } +} +` + const testAccCloudStackRolePermission_subset = ` resource "cloudstack_role" "foo" { name = "terraform-role"