Skip to content

Fix role permission ordering and preserve external permissions - #340

Open
bddvlpr wants to merge 1 commit into
apache:mainfrom
bddvlpr:fix/role-permission-drift
Open

Fix role permission ordering and preserve external permissions#340
bddvlpr wants to merge 1 commit into
apache:mainfrom
bddvlpr:fix/role-permission-drift

Conversation

@bddvlpr

@bddvlpr bddvlpr commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fixes ordering and identity handling for cloudstack_role_permissions.

Role perms are evaluated top to bottom so their order affects the resulting access policy. Previously refresh reconstructed managed permissions in Terraform's existing order instead of the order returned by CloudStack. This oob ordering changes and prevented Terraform from detecting and correcting the drift.

Inserting or removing permissions could also shift computed IDs between list elements. Reconciliation could consequently delete and recreate valid permissions, or accidentally delete externally managed permissions when authorative is not turned on.

Tests cover the detecting and correcting oob ordering changes, reordering lists containing two or more permissions, shifted ids after inserting or removing list items, preserving IDs for unchanged perms and producing an empty follow up plan after reconciliation.

Copilot AI lite review requested due to automatic review settings September 7, 2026 07:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new matching/reconcile flow can fail to handle description drift/changes correctly (delete+recreate is needed since the API update can’t change descriptions), which can cause duplicate-rule errors or perpetual diffs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves how cloudstack_role_permission reconciles and refreshes role permissions so Terraform can (a) detect and correct out-of-band (OOB) ordering drift and (b) avoid incorrectly deleting/recreating permissions due to computed ID shifts when list items are inserted/removed.

Changes:

  • Refresh now preserves the CloudStack-returned order for managed permissions, so OOB reorder drift remains visible to Terraform.
  • Reconciliation now uses a dedicated matching routine to map desired permissions to CloudStack permissions more stably across list reorder/insert/remove scenarios.
  • Expanded acceptance/unit coverage around ordering drift detection and ID stability.
File summaries
File Description
cloudstack/resource_cloudstack_role_permission.go Refactors read/reconcile matching and ordering behavior; adds matchCloudStackRolePermissions.
cloudstack/resource_cloudstack_role_permission_test.go Adds acceptance tests for ordering drift and checks for stable IDs; adds unit tests for matcher behavior on shifted IDs.
Review details

Suppressed comments (1)

cloudstack/resource_cloudstack_role_permission.go:443

  • matchCloudStackRolePermissions only matches by ID when both the rule and description match. If the permission ID is stable but the description drifts (or config changes the description), this causes the permission to be treated as "missing" and can lead to duplicate creates or an inability to reconcile description drift. Matching by ID should at least require the rule to match, but not the description, and let reconciliation handle description replacement.
		if desired.ID != "" {
			candidate := permissionsByID[desired.ID]
			if candidate != nil && !used[candidate.Id] && candidate.Rule == desired.Rule && candidate.Description == desired.Description {
				rp = candidate
			}
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +252 to 256
for i, desired := range desiredPermissions {
rp := matchedPermissions[i]
if rp == nil {
rp, err = createCloudStackRolePermission(cs, roleID, desired)
if err != nil {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants