Fix role permission ordering and preserve external permissions

[bddvlpr]

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]

🟡 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.

[kiranchavala]

LGTM, Tested manually

resource "cloudstack_role" "test" {
  name = "tf-pr340-role"
  type = "User"
}

resource "cloudstack_role_permission" "test" {
  role_id = cloudstack_role.test.id

  permission {
    rule       = "listZones"
    permission = "allow"
  }

  permission {
    rule       = "listVirtualMachines"
    permission = "allow"
  }

  permission {
    rule       = "*"
    permission = "deny"
  }
}

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-pr340-role"
      + type        = "User"
    }

  # cloudstack_role_permission.test will be created
  + resource "cloudstack_role_permission" "test" {
      + authoritative = false
      + id            = (known after apply)
      + role_id       = (known after apply)

      + permission {
          + id         = (known after apply)
          + permission = "allow"
          + rule       = "listZones"
        }
      + permission {
          + id         = (known after apply)
          + permission = "allow"
          + rule       = "listVirtualMachines"
        }
      + permission {
          + id         = (known after apply)
          + permission = "deny"
          + rule       = "*"
        }
    }

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=91abd76b-a522-4324-b622-2a3d35d6dbe7]
cloudstack_role_permission.test: Creating...
cloudstack_role_permission.test: Creation complete after 1s [id=91abd76b-a522-4324-b622-2a3d35d6dbe7]

Apply complete! Resources: 2 added, 0 changed, 0 destroyed.

╰─ terraform destroy
cloudstack_role.test: Refreshing state... [id=91abd76b-a522-4324-b622-2a3d35d6dbe7]
cloudstack_role_permission.test: Refreshing state... [id=91abd76b-a522-4324-b622-2a3d35d6dbe7]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  - destroy

Terraform will perform the following actions:

  # cloudstack_role.test will be destroyed
  - resource "cloudstack_role" "test" {
      - id          = "91abd76b-a522-4324-b622-2a3d35d6dbe7" -> null
      - is_public   = true -> null
      - name        = "tf-pr340-role" -> null
      - type        = "User" -> null
        # (1 unchanged attribute hidden)
    }

  # cloudstack_role_permission.test will be destroyed
  - resource "cloudstack_role_permission" "test" {
      - authoritative = false -> null
      - id            = "91abd76b-a522-4324-b622-2a3d35d6dbe7" -> null
      - role_id       = "91abd76b-a522-4324-b622-2a3d35d6dbe7" -> null

      - permission {
          - id          = "d2ba48c6-cd39-4d71-bff7-7b33303b01cc" -> null
          - permission  = "allow" -> null
          - rule        = "listZones" -> null
            # (1 unchanged attribute hidden)
        }
      - permission {
          - id          = "3c28c4c0-ebb9-442f-b9af-3220f889f27d" -> null
          - permission  = "allow" -> null
          - rule        = "listVirtualMachines" -> null
            # (1 unchanged attribute hidden)
        }
      - permission {
          - id          = "d6a5be98-873e-4f29-b046-3cba878a5487" -> null
          - permission  = "deny" -> null
          - rule        = "*" -> null
            # (1 unchanged attribute hidden)
        }
    }

Plan: 0 to add, 0 to change, 2 to destroy.

Do you really want to destroy all resources?
  Terraform will destroy all your managed infrastructure, as shown above.
  There is no undo. Only 'yes' will be accepted to confirm.

  Enter a value: yes

cloudstack_role_permission.test: Destroying... [id=91abd76b-a522-4324-b622-2a3d35d6dbe7]
cloudstack_role_permission.

test: Destruction complete after 1s
cloudstack_role.test: Destroying... [id=91abd76b-a522-4324-b622-2a3d35d6dbe7]
cloudstack_role.test: Destruction complete after 0s



Destroy complete! Resources: 2 destroyed.

[vishesh92]

Fixed some issues in this PR here: #350
Once this is merged, I will rebase my PR.

[sureshanaparti]

clgtm