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 7a9e48b Fix role_permission description edits and authoritative drift
detection (#350)
7a9e48b is described below
commit 7a9e48bccf03f4e78881560203c5d09c6baabf9d
Author: Vishesh <[email protected]>
AuthorDate: Tue Sep 15 18:03:09 2026 +0530
Fix role_permission description edits and authoritative drift detection
(#350)
---
cloudstack/resource_cloudstack_role_permission.go | 144 ++++++++++++-------
.../resource_cloudstack_role_permission_test.go | 152 +++++++++++++++++++++
2 files changed, 248 insertions(+), 48 deletions(-)
diff --git a/cloudstack/resource_cloudstack_role_permission.go
b/cloudstack/resource_cloudstack_role_permission.go
index c6f73d0..2a9a436 100644
--- a/cloudstack/resource_cloudstack_role_permission.go
+++ b/cloudstack/resource_cloudstack_role_permission.go
@@ -20,6 +20,7 @@
package cloudstack
import (
+ "context"
"fmt"
"log"
"sync"
@@ -44,6 +45,20 @@ func resourceCloudStackRolePermission() *schema.Resource {
Read: resourceCloudStackRolePermissionRead,
Update: resourceCloudStackRolePermissionUpdate,
Delete: resourceCloudStackRolePermissionDelete,
+ // Duplicate rules are rejected at plan time; by Update the
invalid list is already
+ // in state. Rules still unknown at plan time are skipped (they
read as "").
+ CustomizeDiff: func(_ context.Context, d *schema.ResourceDiff,
_ interface{}) error {
+ if !d.NewValueKnown("permission") {
+ return nil
+ }
+ permissions :=
rolePermissionSpecs(d.Get("permission").([]interface{}))
+ for i := range permissions {
+ if
!d.NewValueKnown(fmt.Sprintf("permission.%d.rule", i)) {
+ permissions[i].Rule = ""
+ }
+ }
+ return validateUniqueRolePermissionRules(permissions)
+ },
Schema: map[string]*schema.Schema{
"role_id": {
Type: schema.TypeString,
@@ -138,7 +153,7 @@ func resourceCloudStackRolePermissionRead(d
*schema.ResourceData, meta interface
// 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))
+ readPermissions := make([]interface{}, 0,
len(rolePermissions)+len(missingPermissions))
for _, rp := range rolePermissions {
if !used[rp.Id] {
continue
@@ -150,6 +165,23 @@ func resourceCloudStackRolePermissionRead(d
*schema.ResourceData, meta interface
Description: rp.Description,
}))
}
+
+ // When authoritative, undeclared permissions must appear in state or
no diff is
+ // produced and Update never runs. They carry no id: an id marks a
managed permission,
+ // and reconcile deletes formerly managed ones when authoritative is
switched off.
+ if d.Get("authoritative").(bool) {
+ for _, rp := range rolePermissions {
+ if used[rp.Id] {
+ continue
+ }
+ readPermissions = append(readPermissions,
rolePermissionState(rolePermissionSpec{
+ Rule: rp.Rule,
+ Permission: rp.Permission,
+ Description: rp.Description,
+ }))
+ }
+ }
+
readPermissions = append(readPermissions, missingPermissions...)
if err := d.Set("permission", readPermissions); err != nil {
@@ -205,24 +237,14 @@ func resourceCloudStackRolePermissionDelete(d
*schema.ResourceData, meta interfa
return nil
}
- rolePermissionsByID := make(map[string]*cloudstack.RolePermission)
- for _, rp := range rolePermissions {
- rolePermissionsByID[rp.Id] = rp
- }
-
- used := make(map[string]bool)
- for _, permission := range
rolePermissionSpecs(d.Get("permission").([]interface{})) {
- ruleID := permission.ID
- if ruleID == "" {
- if rp := findMatchingRolePermission(rolePermissions,
permission, used); rp != nil {
- ruleID = rp.Id
- }
- }
- if ruleID == "" || rolePermissionsByID[ruleID] == nil {
+ // Not authoritative: remove only the permissions this resource manages
and
+ // leave anything added outside Terraform in place.
+ desiredPermissions :=
rolePermissionSpecs(d.Get("permission").([]interface{}))
+ for _, rp := range matchCloudStackRolePermissions(rolePermissions,
desiredPermissions) {
+ if rp == nil {
continue
}
- used[ruleID] = true
- if err := deleteCloudStackRolePermission(cs, ruleID); err !=
nil {
+ if err := deleteCloudStackRolePermission(cs, rp.Id); err != nil
{
return err
}
}
@@ -234,6 +256,11 @@ func reconcileCloudStackRolePermissions(d
*schema.ResourceData, meta interface{}
cs := meta.(*cloudstack.CloudStackClient)
roleID := d.Get("role_id").(string)
+ desiredPermissions :=
rolePermissionSpecs(d.Get("permission").([]interface{}))
+ if err := validateUniqueRolePermissionRules(desiredPermissions); err !=
nil {
+ return err
+ }
+
rolePermissions, err := listCloudStackRolePermissions(cs, roleID)
if err != nil {
return fmt.Errorf("Error listing Role Permissions: %s", err)
@@ -246,9 +273,26 @@ func reconcileCloudStackRolePermissions(d
*schema.ResourceData, meta interface{}
managedIDs := make([]string, 0)
managedIDSet := make(map[string]bool)
- desiredPermissions :=
rolePermissionSpecs(d.Get("permission").([]interface{}))
+ deleted := make(map[string]bool)
matchedPermissions := matchCloudStackRolePermissions(rolePermissions,
desiredPermissions)
+ // Descriptions cannot be updated in place, so a changed one is
recreated. Delete
+ // first: creating the replacement while the old one exists fails with
"Rule already exists".
+ for i, desired := range desiredPermissions {
+ rp := matchedPermissions[i]
+ if rp == nil || rp.Description == desired.Description {
+ continue
+ }
+
+ if err := deleteCloudStackRolePermission(cs, rp.Id); err != nil
{
+ return err
+ }
+
+ deleted[rp.Id] = true
+ delete(rolePermissionsByID, rp.Id)
+ matchedPermissions[i] = nil
+ }
+
for i, desired := range desiredPermissions {
rp := matchedPermissions[i]
if rp == nil {
@@ -275,16 +319,17 @@ func reconcileCloudStackRolePermissions(d
*schema.ResourceData, meta interface{}
if d.Get("authoritative").(bool) {
for _, rp := range rolePermissions {
- if managedIDSet[rp.Id] {
+ if managedIDSet[rp.Id] || deleted[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] {
+ if managedIDSet[oldID] || deleted[oldID] {
continue
}
if rolePermissionsByID[oldID] == nil {
@@ -293,6 +338,7 @@ func reconcileCloudStackRolePermissions(d
*schema.ResourceData, meta interface{}
if err := deleteCloudStackRolePermission(cs, oldID);
err != nil {
return err
}
+ deleted[oldID] = true
}
}
@@ -412,48 +458,50 @@ func rolePermissionState(permission rolePermissionSpec)
map[string]interface{} {
}
}
-func findMatchingRolePermission(rolePermissions []*cloudstack.RolePermission,
desired rolePermissionSpec, used map[string]bool) *cloudstack.RolePermission {
- for _, rp := range rolePermissions {
- if used[rp.Id] {
- continue
- }
- if rp.Rule == desired.Rule && rp.Description ==
desired.Description {
- return rp
- }
- }
-
- return nil
-}
-
+// matchCloudStackRolePermissions pairs each desired permission with the
existing one for
+// the same rule. A rule may appear once per role, so it is the permission's
identity.
func matchCloudStackRolePermissions(rolePermissions
[]*cloudstack.RolePermission, desiredPermissions []rolePermissionSpec)
[]*cloudstack.RolePermission {
- permissionsByID := make(map[string]*cloudstack.RolePermission,
len(rolePermissions))
+ permissionsByRule := make(map[string]*cloudstack.RolePermission,
len(rolePermissions))
for _, rp := range rolePermissions {
- permissionsByID[rp.Id] = rp
+ if _, ok := permissionsByRule[rp.Rule]; !ok {
+ permissionsByRule[rp.Rule] = rp
+ }
}
matchedPermissions := make([]*cloudstack.RolePermission,
len(desiredPermissions))
- used := make(map[string]bool)
+ used := make(map[string]bool, len(desiredPermissions))
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
+ rp := permissionsByRule[desired.Rule]
+ if rp == nil || used[rp.Id] {
+ continue
}
+
+ used[rp.Id] = true
matchedPermissions[i] = rp
}
return matchedPermissions
}
+// validateUniqueRolePermissionRules rejects a rule listed more than once.
+func validateUniqueRolePermissionRules(permissions []rolePermissionSpec) error
{
+ seen := make(map[string]int, len(permissions))
+ for i, permission := range permissions {
+ if permission.Rule == "" {
+ continue
+ }
+ if first, ok := seen[permission.Rule]; ok {
+ return fmt.Errorf(
+ "duplicate rule %q in permission entries %d and
%d: a rule may appear at most once per role",
+ permission.Rule, first+1, i+1)
+ }
+ seen[permission.Rule] = i
+ }
+
+ return nil
+}
+
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 6196a05..6d8606e 100644
--- a/cloudstack/resource_cloudstack_role_permission_test.go
+++ b/cloudstack/resource_cloudstack_role_permission_test.go
@@ -26,6 +26,8 @@ import (
"github.com/apache/cloudstack-go/v2/cloudstack"
"github.com/hashicorp/terraform-plugin-testing/helper/resource"
"github.com/hashicorp/terraform-plugin-testing/terraform"
+ "regexp"
+ "strings"
)
func TestAccCloudStackRolePermission_basic(t *testing.T) {
@@ -645,3 +647,153 @@ resource "cloudstack_role_permission" "foo" {
}
}
`
+
+func TestMatchCloudStackRolePermissions_descriptionChangeMatchesByRule(t
*testing.T) {
+ rolePermissions := []*cloudstack.RolePermission{
+ {Id: "list-id", Rule: "listVirtualMachines", Permission:
"allow", Description: "old"},
+ {Id: "deploy-id", Rule: "deployVirtualMachine", Permission:
"deny", Description: "no deploy"},
+ }
+ desiredPermissions := []rolePermissionSpec{
+ {ID: "list-id", Rule: "listVirtualMachines", Permission:
"allow", Description: "new"},
+ {ID: "deploy-id", Rule: "deployVirtualMachine", Permission:
"allow", Description: "no deploy"},
+ }
+
+ matchedPermissions := matchCloudStackRolePermissions(rolePermissions,
desiredPermissions)
+ assertRolePermissionIDs(t, matchedPermissions, []string{"list-id",
"deploy-id"})
+}
+
+func TestValidateUniqueRolePermissionRules(t *testing.T) {
+ if err := validateUniqueRolePermissionRules([]rolePermissionSpec{
+ {Rule: "listVirtualMachines"}, {Rule: "listVolumes"},
+ }); err != nil {
+ t.Fatalf("unexpected error for unique rules: %s", err)
+ }
+
+ if err := validateUniqueRolePermissionRules([]rolePermissionSpec{
+ {Rule: ""}, {Rule: "listVolumes"}, {Rule: ""},
+ }); err != nil {
+ t.Fatalf("unexpected error for unknown (empty) rules: %s", err)
+ }
+
+ err := validateUniqueRolePermissionRules([]rolePermissionSpec{
+ {Rule: "listVirtualMachines"}, {Rule: "listVolumes"}, {Rule:
"listVirtualMachines"},
+ })
+ if err == nil {
+ t.Fatal("expected an error for a duplicated rule")
+ }
+ for _, want := range []string{`"listVirtualMachines"`, "entries 1 and
3"} {
+ if !strings.Contains(err.Error(), want) {
+ t.Fatalf("error %q should mention %s", err, want)
+ }
+ }
+}
+
+func TestAccCloudStackRolePermission_descriptionChange(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackRolePermissionDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackRolePermission_basic,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"),
+
resource.TestCheckResourceAttr("cloudstack_role_permission.foo",
"permission.0.description", "terraform test role permission"),
+ ),
+ },
+ {
+ Config:
testAccCloudStackRolePermission_descriptionChanged,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"),
+
testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo",
[]string{"listVirtualMachines"}),
+
resource.TestCheckResourceAttr("cloudstack_role_permission.foo",
"permission.0.description", "terraform test role permission (updated)"),
+ ),
+ },
+ },
+ })
+}
+
+// Unlike _authoritative, the config is identical in both steps, so only the
refresh
+// can detect the externally added permission.
+func TestAccCloudStackRolePermission_authoritativeDrift(t *testing.T) {
+ var externalRuleID string
+
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackRolePermissionDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackRolePermission_authoritative,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"),
+
testAccCreateCloudStackRolePermission("cloudstack_role_permission.foo",
"listVirtualMachines", "allow", "external role permission", &externalRuleID),
+ ),
+ // The Check adds a permission out of band; the
post-step refresh seeing it
+ // as drift is the behaviour under test.
+ ExpectNonEmptyPlan: true,
+ },
+ {
+ Config:
testAccCloudStackRolePermission_authoritative,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackRolePermissionExists("cloudstack_role_permission.foo"),
+
testAccCheckCloudStackRolePermissionRuleMissing("cloudstack_role_permission.foo",
&externalRuleID),
+
testAccCheckCloudStackRolePermissionOrder("cloudstack_role_permission.foo",
[]string{"listZones"}),
+ ),
+ },
+ },
+ })
+}
+
+const testAccCloudStackRolePermission_descriptionChanged = `
+resource "cloudstack_role" "foo" {
+ name = "terraform-role"
+ type = "User"
+}
+
+resource "cloudstack_role_permission" "foo" {
+ role_id = cloudstack_role.foo.id
+
+ permission {
+ rule = "listVirtualMachines"
+ permission = "allow"
+ description = "terraform test role permission (updated)"
+ }
+}
+`
+
+func TestAccCloudStackRolePermission_duplicateRuleRejected(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackRolePermissionDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config:
testAccCloudStackRolePermission_duplicateRule,
+ PlanOnly: true,
+ ExpectError: regexp.MustCompile(`duplicate rule
"listVirtualMachines" in permission entries 1 and 2`),
+ },
+ },
+ })
+}
+
+const testAccCloudStackRolePermission_duplicateRule = `
+resource "cloudstack_role" "foo" {
+ name = "terraform-role"
+ type = "User"
+}
+
+resource "cloudstack_role_permission" "foo" {
+ role_id = cloudstack_role.foo.id
+
+ permission {
+ rule = "listVirtualMachines"
+ permission = "allow"
+ }
+
+ permission {
+ rule = "listVirtualMachines"
+ permission = "deny"
+ }
+}
+`
