This is an automated email from the ASF dual-hosted git repository.
sureshanaparti pushed a commit to branch main
in repository
https://gitbox.apache.org/repos/asf/cloudstack-terraform-provider.git
The following commit(s) were added to refs/heads/main by this push:
new 13756c1 Fix reordering of role permissions (inc. external) (#340)
13756c1 is described below
commit 13756c17d42be5b187f49c12fe7e4905f565be52
Author: bddvlpr <[email protected]>
AuthorDate: Tue Sep 15 09:18:08 2026 +0200
Fix reordering of role permissions (inc. external) (#340)
---
cloudstack/resource_cloudstack_role_permission.go | 91 ++++---
.../resource_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 1769f8d..c6f73d0 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 18eeee6..6196a05 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"
