Repository navigation
Fix role_permission description edits and authoritative drift detection - #350
Merged
Merged
Conversation
vishesh92
changed the title
This was referenced Sep 15, 2026
vishesh92
force-pushed
the
fixup-cloudstack-role-permission
branch
from
September 15, 2026 07:18
1903f15 to
c17fdbc
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Fix ID-based deletion behavior and move the drift assertion to the correct test step.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates CloudStack role-permission reconciliation for description edits, authoritative drift detection, and duplicate-rule validation.
Changes:
- Reconcile permission description changes.
- Detect unmanaged permissions in authoritative mode.
- Add regression and acceptance coverage.
File summaries
| File | Summary |
|---|---|
cloudstack/resource_cloudstack_role_permission.go |
Updates reconciliation, drift detection, deletion, and validation logic. |
cloudstack/resource_cloudstack_role_permission_test.go |
Adds coverage for description edits, drift detection, and duplicate rules. |
Review details
Suppressed comments (2)
cloudstack/resource_cloudstack_role_permission.go:338
- When an authoritative resource is refreshed, the new Read path writes unmanaged permissions into state with their real IDs. If
authoritativeis then changed to false,oldPermissionsincludes those external IDs and this branch treats them as previously managed, deleting them even though non-authoritative mode is supposed to preserve out-of-band permissions. Keep unmanaged state entries distinguishable from managed entries (or otherwise track the prior managed set) before usingoldManagedIDshere.
for oldID := range oldManagedIDs {
if managedIDSet[oldID] || deleted[oldID] {
continue
}
if rolePermissionsByID[oldID] == nil {
continue
}
if err := deleteCloudStackRolePermission(cs, oldID); err != nil {
return err
cloudstack/resource_cloudstack_role_permission_test.go:738
- This assertion is on the initial step, whose create plan is necessarily non-empty; the external permission is added only by
Check, after that initial plan. It therefore does not assert that an unchanged configuration produces a non-empty plan from authoritative drift. MoveExpectNonEmptyPlan: trueto the second same-config step (and remove it here) so this regression test fails ifReaddoes not surface the externally added permission.
ExpectNonEmptyPlan: true,
- 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.
vishesh92
force-pushed
the
fixup-cloudstack-role-permission
branch
from
September 15, 2026 07:43
c17fdbc to
31664fb
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comment on lines
+52
to
+54
| CustomizeDiff: func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error { | ||
| return validateUniqueRolePermissionRules(rolePermissionSpecs(d.Get("permission").([]interface{}))) | ||
| }, |
Comment on lines
+242
to
+249
| // 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 { |
Contributor
|
Hi @bddvlpr Can you check/review this PR. |
vishesh92
force-pushed
the
fixup-cloudstack-role-permission
branch
from
September 15, 2026 10:16
31664fb to
5a41b76
Compare
kiranchavala
approved these changes
Sep 15, 2026
Member
There was a problem hiding this comment.
LGTM tested manually
resource "cloudstack_role" "test" {
name = "tf-pr350-role"
type = "User"
}
resource "cloudstack_role_permission" "test" {
role_id = cloudstack_role.test.id
authoritative = true
permission {
rule = "listVirtualMachines"
permission = "allow"
description = "original"
}
permission {
rule = "listZones"
permission = "allow"
description = "zones"
}
}
terraform apply
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# cloudstack_role.test will be created
+ resource "cloudstack_role" "test" {
+ description = (known after apply)
+ id = (known after apply)
+ is_public = true
+ name = "tf-pr350-role"
+ type = "User"
}
# cloudstack_role_permission.test will be created
+ resource "cloudstack_role_permission" "test" {
+ authoritative = true
+ id = (known after apply)
+ role_id = (known after apply)
+ permission {
+ description = "original"
+ id = (known after apply)
+ permission = "allow"
+ rule = "listVirtualMachines"
}
+ permission {
+ description = "zones"
+ id = (known after apply)
+ permission = "allow"
+ rule = "listZones"
}
}
Plan: 2 to add, 0 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_role.test: Creating...
cloudstack_role.test: Creation complete after 1s [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Creating...
cloudstack_role_permission.test: Creation complete after 2s [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Apply complete! Resources: 2 added, 0 changed, 0 destroyed.
Update the description to updated
description = "original-updated"
terraform apply
cloudstack_role.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# cloudstack_role_permission.test will be updated in-place
~ resource "cloudstack_role_permission" "test" {
id = "062367a5-8ee9-4534-b87c-128d40d112c7"
# (2 unchanged attributes hidden)
~ permission {
~ description = "original" -> "original-updated"
id = "75e5a99a-46b0-4c97-bd10-1dcb488bb465"
# (2 unchanged attributes hidden)
}
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_role_permission.test: Modifying... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Modifications complete after 2s [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
authoritative drift
- With the above applied, add a permission out of band:
cmk create rolepermission roleid= rule=listVolumes permission=allow description=external
[root@ref-trl-12298-k-Mr8-suresh-anaparti-mgmt1 ~]# cmk create rolepermission roleid=062367a5-8ee9-4534-b87c-128d40d112c7 rule=listVolumes permission=allow description=external
Loaded in-built API cache. Failed to read API cache, please run 'sync'.
{
"rolepermission": {
"description": "external",
"id": "0a5feff0-4535-4458-b642-f2243f33f093",
"permission": "allow",
"roleid": "062367a5-8ee9-4534-b87c-128d40d112c7",
"rule": "listVolumes"
}
}
- terraform plan
cloudstack_role.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# cloudstack_role_permission.test will be updated in-place
~ resource "cloudstack_role_permission" "test" {
id = "062367a5-8ee9-4534-b87c-128d40d112c7"
# (2 unchanged attributes hidden)
- permission {
- description = "external" -> null
id = null
- permission = "allow" -> null
- rule = "listVolumes" -> null
}
# (2 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
- Terraform apply
terraform apply
cloudstack_role.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Refreshing state... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# cloudstack_role_permission.test will be updated in-place
~ resource "cloudstack_role_permission" "test" {
id = "062367a5-8ee9-4534-b87c-128d40d112c7"
# (2 unchanged attributes hidden)
- permission {
- description = "external" -> null
id = null
- permission = "allow" -> null
- rule = "listVolumes" -> null
}
# (2 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_role_permission.test: Modifying... [id=062367a5-8ee9-4534-b87c-128d40d112c7]
cloudstack_role_permission.test: Modifications complete after 1s [id=062367a5-8ee9-4534-b87c-128d40d112c7]
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.