Fix role permission ordering and preserve external permissions - #340
Fix role permission ordering and preserve external permissions#340bddvlpr wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 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
matchCloudStackRolePermissionsonly 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.
| for i, desired := range desiredPermissions { | ||
| rp := matchedPermissions[i] | ||
| if rp == nil { | ||
| rp, err = createCloudStackRolePermission(cs, roleID, desired) | ||
| if err != nil { |
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
authorativeis 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.