Conditional supporting RBAC creation by Zerpet · Pull Request #2128 · rabbitmq/cluster-operator

Zerpet · 2026-04-30T10:35:37Z

Summary Of Changes

Conditionally create RBAC for RabbitMQ StatefulSet, based on rabbit version
Add delete RBAC for Role and RoleBinding to the Operator ClusterRole

Additional Context

Related to #1865. Starting in RabbitMQ 4.1, the k8s peer discovery plugin does not require any RBAC in Kubernetes. Earlier versions of this plugin queried k8s API to determine the addresses of its peers. That's not required anymore since 4.1. This PR is an optimisation that removes unnecessary RBACs (SA, Role/Binding).

The annotation based approach has a caveat: since the Operator does not know before hand the version of RabbitMQ, it creates the RBACs, and when it detects that rabbit is >= 4.1.0, it removes them. This causes a rolling restart, which slightly delays initial rabbit deployment. The same applies when rabbit is upgraded from 3.13 to 4.2. Once rabbit is upgraded to 4.2, and the annotation is updated, RBACs are deleted and the STS is rolled. This happens because the Pod Spec changes, to remove the SA reference.

Another downside is that the Operator ClusterRole now requires delete on Role, RoleBinding and ServiceAccount. This broadens the permissions of the operator with a justified reason, but it feels like the operator is effectively a super-admin-user.

I'm ok to not merge this PR, if the ClusterRole change is deemed unacceptable.

Local Testing

Unit and integration tests.

make unit-tests integration-tests

Tested locally the following scenarios:

Deploy 3.13 -> Verify RBAC is created -> Upgrade 4.2 -> Verify RBAC is deleted
Deploy 4.1 -> Verify RBAC is eventually deleted
Deploy 4.2 -> Verify RBAC is eventually deleted

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Zerpet · 2026-05-05T11:39:37Z

@copilot apply changes based on the comments in this thread

Copilot · 2026-05-05T11:58:53Z

@copilot apply changes based on the comments in this thread

Applied the following changes in commit abfca8a:

ServiceAccount always created/kept — Moved ServiceAccount builder out of the conditional block so it is always included, regardless of RabbitMQ version. The deletion block in the controller no longer removes the ServiceAccount (only Role/RoleBinding are deleted for >= 4.1).
ServiceAccountName always set in StatefulSet — Removed the ShouldCreateRBAC guard around ServiceAccountName and AutomountServiceAccountToken; the pod spec always references the operator-managed ServiceAccount.
Semver constraint at package scope — Created peerDiscoveryRBACConstraint as a package-level variable (using a mustNewConstraint helper) to avoid rebuilding it on every call.
Renamed ShouldCreateRBAC → ShouldCreatePeerDiscoveryRBAC — The new name clarifies that the function only gates creation of the peer-discovery Role/RoleBinding, not the ServiceAccount.
Removed delete from ServiceAccount RBAC marker — Since the operator no longer explicitly deletes ServiceAccounts, the delete verb was removed from the serviceaccounts RBAC permission, reducing the operator's footprint back to the original level.

MirahImage

Given that we still need the serviceaccount for vault integration, how complicated the logic ends up being, and the fact that we trigger additional rolling upgrades based on version, I'm not sure this is really worthwhile.

Also, did copilot actually implement the changes it claims? I'm not sure it did?

Zerpet · 2026-05-05T14:31:17Z

I think it won't require a rolling restart anymore, since we are keeping the service account. The STS modification (to remove the SA reference) is what was causing the rolling restart. I'll test this in a bit.

MirahImage · 2026-05-05T15:02:23Z

I think it won't require a rolling restart anymore, since we are keeping the service account. The STS modification (to remove the SA reference) is what was causing the rolling restart. I'll test this in a bit.

Assuming it doesn't cause the rolling restart, and that we get the rbac right after the code generation, I'm good to go with this. It's still a bit fragile, but in a way that it takes the approach that works if it doesn't know.

Add a helper function to verify if RBAC resources (ServiceAccount, Role, and RoleBinding) should be created based on the RabbitMQ version annotation. The RBAC resources are no longer required for RabbitMQ >= 4.1.0. - Skip adding RBAC resource builders for clusters >= 4.1.0 - Ensure any existing RBAC resources are deleted during reconciliation if the cluster is upgraded to a version that doesn't need them - Conditionally set ServiceAccountName and AutomountServiceAccountToken on the StatefulSet depending on the version - Include unit and integration tests to verify the new behavior Made-with: Cursor

Update newly added tests to use BeTrueBecause and BeFalseBecause for more descriptive error messages when assertions fail. Made-with: Cursor

They are no longer required by the peer discovery plugin in 4.1+ Keeping the Service Account because it is still required for Vault integration.

Agent-Logs-Url: https://github.com/rabbitmq/cluster-operator/sessions/99f20b88-4e9d-460d-8e59-c385360b7b8e Co-authored-by: Zerpet <1515757+Zerpet@users.noreply.github.com>

Because sometimes system tests flake attempting to verify that rabbit has started. From the logs looks like `kubectl` command does not return. By bumping the timeout, we should avoid hammering the CI runner with a pile of pending `kubectl` commands.

Zerpet · 2026-05-05T16:02:43Z

Confirmed the latest additions by Copilot don't cause a rolling restart. The operator initially creates the Role/Binding, because it doesn't know the version of RabbitMQ, but then it deletes them quietly. I also reviewed the Vault example. Indeed we require the SA for correct Vault integration.

Zerpet · 2026-05-05T16:04:54Z

The diff might look weird. This is because I rebased and squashed some commits, so the only RBAC change required is adding delete to Role/Binding.

Zerpet requested review from MirahImage and mkuratczyk April 30, 2026 10:35

Zerpet self-assigned this Apr 30, 2026

Zerpet added this to the v2.21.0 milestone Apr 30, 2026

Zerpet requested a review from Copilot April 30, 2026 10:36

Copilot started reviewing on behalf of Zerpet April 30, 2026 10:37 View session

Zerpet mentioned this pull request Apr 30, 2026

Drop dedicated ServiceAccount for CR instances #2072

Open

Copilot AI reviewed Apr 30, 2026

Zerpet requested a review from Copilot May 5, 2026 10:29

Copilot started reviewing on behalf of Zerpet May 5, 2026 10:29 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread internal/resource/statefulset.go Outdated

Comment thread internal/resource/rabbitmq_resource_builder.go Outdated

Comment thread internal/controller/rabbitmqcluster_controller.go Outdated

Comment thread internal/resource/rabbitmq_resource_builder.go Outdated

Copilot started work on behalf of Zerpet May 5, 2026 11:39 View session

Copilot finished work on behalf of Zerpet May 5, 2026 12:00

MirahImage reviewed May 5, 2026

View reviewed changes

Comment thread config/rbac/role.yaml Outdated

Zerpet and others added 6 commits May 5, 2026 16:54

Refactor BeTrue/BeFalse assertions to use BeTrueBecause/BeFalseBecause

5f70c0f

Update newly added tests to use BeTrueBecause and BeFalseBecause for more descriptive error messages when assertions fail. Made-with: Cursor

Delete peer-discovery Role/Binding for RMQ >= 4.1

9cea2eb

They are no longer required by the peer discovery plugin in 4.1+ Keeping the Service Account because it is still required for Vault integration.

Conditional peer-discovery RBAC creation based on RabbitMQ version

f1ec4a0

Agent-Logs-Url: https://github.com/rabbitmq/cluster-operator/sessions/99f20b88-4e9d-460d-8e59-c385360b7b8e Co-authored-by: Zerpet <1515757+Zerpet@users.noreply.github.com>

Bump system test interval check

a39416e

Because sometimes system tests flake attempting to verify that rabbit has started. From the logs looks like `kubectl` command does not return. By bumping the timeout, we should avoid hammering the CI runner with a pile of pending `kubectl` commands.

Re-generate RBAC role

bf74c71

Zerpet force-pushed the conditional-rbac-creation branch from e02ccda to bf74c71 Compare May 5, 2026 15:58

MirahImage approved these changes May 6, 2026

View reviewed changes

Zerpet merged commit d28d547 into main May 7, 2026
115 of 117 checks passed

Zerpet deleted the conditional-rbac-creation branch May 7, 2026 15:15

pvillaverde mentioned this pull request Jun 2, 2026

[rabbitmq-cluster-operator ] Adds missing role/rolebinding delete permission to clusterRole CloudPirates-io/helm-charts#1354

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Conditional supporting RBAC creation#2128

Conditional supporting RBAC creation#2128
Zerpet merged 6 commits into
mainfrom
conditional-rbac-creation

Zerpet commented Apr 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Copilot AI commented May 5, 2026

Uh oh!

MirahImage left a comment

Uh oh!

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

MirahImage commented May 5, 2026

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Zerpet commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary Of Changes

Additional Context

Local Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Copilot AI commented May 5, 2026

Uh oh!

MirahImage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

MirahImage commented May 5, 2026

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Zerpet commented May 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Zerpet commented Apr 30, 2026 •

edited

Loading