This is an automated email from the ASF dual-hosted git repository. vishesh92 pushed a commit to branch fixup-cloudstack-role-permission in repository https://gitbox.apache.org/repos/asf/cloudstack-terraform-provider.git
commit 1903f15e84b241a9326f4ea56440a67fedf006b5 Author: vishesh92 <[email protected]> AuthorDate: Tue Sep 15 12:01:08 2026 +0530 Fix role_permission description edits and authoritative drift detection --- cloudstack/resource_cloudstack_role_permission.go | 150 ++++++++++++------- .../resource_cloudstack_role_permission_test.go | 159 +++++++++++++++++++++ 2 files changed, 261 insertions(+), 48 deletions(-) diff --git a/cloudstack/resource_cloudstack_role_permission.go b/cloudstack/resource_cloudstack_role_permission.go index c6f73d0..ab39a9c 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,13 @@ func resourceCloudStackRolePermission() *schema.Resource { Read: resourceCloudStackRolePermissionRead, Update: resourceCloudStackRolePermissionUpdate, Delete: resourceCloudStackRolePermissionDelete, + // Reject duplicate rules at plan time. Doing it only in Update is too late: + // SDKv2 merges the planned permission list into state before Update runs, so + // an error raised there persists the invalid list even though CloudStack was + // never called. + CustomizeDiff: func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error { + return validateUniqueRolePermissionRules(rolePermissionSpecs(d.Get("permission").([]interface{}))) + }, Schema: map[string]*schema.Schema{ "role_id": { Type: schema.TypeString, @@ -138,7 +146,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 +158,27 @@ func resourceCloudStackRolePermissionRead(d *schema.ResourceData, meta interface Description: rp.Description, })) } + + // When authoritative, a permission that exists on the role but is not declared in + // the configuration is drift. It has to appear in state for Terraform to produce a + // diff at all -- otherwise Update, where the authoritative cleanup lives, is never + // called and the permission silently survives. When not authoritative such + // permissions are intentionally unmanaged, so surfacing them would only create a + // permanent diff. + if d.Get("authoritative").(bool) { + for _, rp := range rolePermissions { + if used[rp.Id] { + continue + } + readPermissions = append(readPermissions, rolePermissionState(rolePermissionSpec{ + ID: rp.Id, + Rule: rp.Rule, + Permission: rp.Permission, + Description: rp.Description, + })) + } + } + readPermissions = append(readPermissions, missingPermissions...) if err := d.Set("permission", readPermissions); err != nil { @@ -205,24 +234,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 +253,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 +270,28 @@ 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) + // A permission's description cannot be changed in place: updateRolePermission only + // accepts the permission and the rule order. Such a change has to be applied by + // recreating the permission, and the delete has to happen before any create so the + // new permission does not collide with the one it replaces. + 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 +318,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 +337,7 @@ func reconcileCloudStackRolePermissions(d *schema.ResourceData, meta interface{} if err := deleteCloudStackRolePermission(cs, oldID); err != nil { return err } + deleted[oldID] = true } } @@ -412,48 +457,57 @@ 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 +// permission for the same rule. +// +// CloudStack allows a given rule to appear at most once per role (creating a second +// one fails with "Rule already exists for the role"), so the rule is a permission's +// identity. Matching on the rule alone -- rather than on rule plus description -- +// lets a changed description or permission be recognised as an edit of an existing +// permission instead of being mistaken for a brand new one, which would collide with +// the permission it was meant to replace. 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 configuration that lists the same rule +// twice. CloudStack would reject the second one with "Rule already exists for the +// role" partway through reconciliation, leaving the role half-updated; failing up +// front keeps the change atomic and names the offending entries. +func validateUniqueRolePermissionRules(permissions []rolePermissionSpec) error { + seen := make(map[string]int, len(permissions)) + for i, permission := range permissions { + 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..ad285de 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,160 @@ resource "cloudstack_role_permission" "foo" { } } ` + +// A changed description must still match the existing permission for that rule. +// Before, the matcher required rule+description equality, so a description edit +// looked like a brand new permission and its creation collided with the old one +// ("Rule already exists for the role"). +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) + } + + 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) + } + } +} + +// Editing only a permission's description must apply cleanly. CloudStack cannot change +// a description in place, so the provider has to recreate that permission, deleting the +// old one first so the new one does not collide with it. +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)"), + ), + }, + }, + }) +} + +// With authoritative = true, a permission added outside Terraform must show up as +// drift and be removed on the next apply -- WITHOUT anything else in the +// configuration changing. The existing _authoritative test flips the flag between +// steps, which forces an update for an unrelated reason and so never exercised this. +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 above adds a permission behind Terraform's back. Because the + // resource is authoritative, the post-step refresh must see that as drift -- + // a non-empty plan here is the behaviour under test, not a failure. + ExpectNonEmptyPlan: true, + }, + { + // Same config as the previous step: only the refresh can detect the + // externally added permission, so this proves Read surfaces it. + 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)" + } +} +` + +// A duplicated rule must be rejected at plan time, before anything reaches state. +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" + } +} +`
