feat(operator): implement role membership reconciliation #1192
Conversation
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
- Add ManagedPrincipals status field to track membership management - Update controller to reconcile membership changes (add/remove/clear) - Add ClearPrincipals method for transitioning to unmanaged mode - Add comprehensive unit tests for membership updates - Add acceptance tests for principals management scenarios Implements strict membership management where operator reconciles all members in spec. When spec.principals is empty/nil, operator stops managing membership (managedPrincipals=false).
sago2k8
requested review from
RafalKorepta,
andrewstucki,
chrisseto and
gene-redpanda
as code owners
sago2k8
force-pushed
the
sj/operator/manage-role-membership
branch
from
eed2ed3 to
3e8a838
Compare
The operator always takes full ownership of roles it manages, regardless of whether they pre-existed. Updated the authorization-only test to expect role deletion when the CRD is removed. Also updated controller to set managedRole=true when managing pre-existing roles to ensure proper cleanup.
sago2k8
force-pushed
the
sj/operator/manage-role-membership
branch
from
3e8a838 to
e321a0b
Compare
RafalKorepta
approved these changes
Dec 2, 2025
Comment on lines
110
to
116
| if hasRole && !shouldManageRole { | ||
| if err := rolesClient.Delete(ctx, role); err != nil { | ||
| return createPatch(err) | ||
| } | ||
| hasManagedRole = false | ||
| hasManagedPrincipals = false | ||
| } |
Contributor
There was a problem hiding this comment.
As far as I can see this is dead code as shouldManageRole will always be set to true
| require.True(t, role.ShouldManagePrincipals()) | ||
| require.True(t, role.Status.ManagedRole) | ||
| require.False(t, role.Status.ManagedACLs) | ||
| require.True(t, role.Status.ManagedPrincipals) |
Contributor
There was a problem hiding this comment.
NIT: I feel like on line 154 it is compering desire state with actual status.
Suggested change
| require.True(t, role.Status.ManagedPrincipals) | |
| require.Equal(t, role.ShouldManagePrincipals(), role.Status.ManagedPrincipals) |
| require.True(t, role.ShouldManagePrincipals()) | ||
| require.True(t, role.Status.ManagedRole) | ||
| require.True(t, role.Status.ManagedACLs) | ||
| require.True(t, role.Status.ManagedPrincipals) |
Contributor
There was a problem hiding this comment.
NIT: Ditto
Suggested change
| require.True(t, role.Status.ManagedPrincipals) | |
| require.Equal(t, role.ShouldManagePrincipals(), role.Status.ManagedPrincipals) |
| require.True(t, role.ShouldManagePrincipals()) | ||
| require.True(t, role.Status.ManagedRole) | ||
| require.False(t, role.Status.ManagedACLs) | ||
| require.True(t, role.Status.ManagedPrincipals) |
Contributor
There was a problem hiding this comment.
NIT: DITTO
Suggested change
| require.True(t, role.Status.ManagedPrincipals) | |
| require.Equal(t, role.ShouldManagePrincipals(), role.Status.ManagedPrincipals) |
| } | ||
|
|
||
| func TestRoleMembershipReconciliation(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Minute*2) |
Contributor
There was a problem hiding this comment.
Suggested change
| ctx, cancel := context.WithTimeout(context.Background(), time.Minute*2) | |
| ctx, cancel := context.WithTimeout(t.Context(), time.Minute*2) |
| require.NoError(t, environment.Factory.Get(ctx, key, role)) | ||
| require.True(t, role.Status.ManagedRole) | ||
| require.True(t, role.Status.ManagedPrincipals) | ||
| require.Equal(t, []string{"User:alice", "User:charlie"}, role.Spec.Principals) |
Contributor
There was a problem hiding this comment.
I feel like comparison with role.Spec.Principals does not test reconciler, but rather client. Shouldn't you get principals from Redpanda and compare?
Comment on lines
+697
to
+702
| t.Run("cleanup", func(t *testing.T) { | ||
| require.NoError(t, environment.Factory.Delete(ctx, role)) | ||
| _, err := environment.Reconciler.Reconcile(ctx, req) | ||
| require.NoError(t, err) | ||
| require.True(t, apierrors.IsNotFound(environment.Factory.Get(ctx, key, role))) | ||
| }) |
Contributor
There was a problem hiding this comment.
NIT: I'm not sure why cleanup is implemented in subset.
I would suggest to implement it within Cleanup function as follows:
Suggested change
| t.Run("cleanup", func(t *testing.T) { | |
| require.NoError(t, environment.Factory.Delete(ctx, role)) | |
| _, err := environment.Reconciler.Reconcile(ctx, req) | |
| require.NoError(t, err) | |
| require.True(t, apierrors.IsNotFound(environment.Factory.Get(ctx, key, role))) | |
| }) | |
| t.Cleanup(func() { | |
| require.NoError(t, environment.Factory.Delete(ctx, role)) | |
| _, err := environment.Reconciler.Reconcile(ctx, req) | |
| require.NoError(t, err) | |
| require.True(t, apierrors.IsNotFound(environment.Factory.Get(ctx, key, role))) | |
| }) |
github-actions bot
pushed a commit
that referenced
this pull request
Dec 2, 2025
* feat(operator): implement role membership reconciliation - Add ManagedPrincipals status field to track membership management - Update controller to reconcile membership changes (add/remove/clear) - Add ClearPrincipals method for transitioning to unmanaged mode - Add comprehensive unit tests for membership updates - Add acceptance tests for principals management scenarios Implements strict membership management where operator reconciles all members in spec. When spec.principals is empty/nil, operator stops managing membership (managedPrincipals=false). * chore: add changelog entry for role membership reconciliation * fix(acceptance): update test for always-manage-role pattern The operator always takes full ownership of roles it manages, regardless of whether they pre-existed. Updated the authorization-only test to expect role deletion when the CRD is removed. Also updated controller to set managedRole=true when managing pre-existing roles to ensure proper cleanup. (cherry picked from commit 7b1c1df) # Conflicts: # acceptance/features/role-crds.feature # acceptance/steps/register.go # acceptance/steps/roles.go # operator/api/applyconfiguration/redpanda/v1alpha2/rolestatus.go # operator/api/redpanda/v1alpha2/role_types.go # operator/api/redpanda/v1alpha2/testdata/crd-docs.adoc # operator/config/crd/bases/cluster.redpanda.com_redpandaroles.yaml # operator/internal/controller/redpanda/role_controller.go # operator/internal/controller/redpanda/role_controller_test.go # operator/pkg/client/roles/client.go
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
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.
Summary
Implements proper reconciliation of role membership changes by adding status tracking and ensuring the operator updates principals when the spec changes. This follows the same pattern as ACL management, where membership is strictly managed based on the spec.
Key Changes
ManagedPrincipals boolfield toRoleStatusto track whether principals are being managedManagedPrincipals = trueManagedPrincipals = falseClearPrincipals()method for transitioning from managed to unmanaged modeShouldManagePrincipals()andHasManagedPrincipals()for clean APITesting
Unit Tests
TestRoleMembershipReconciliation- New comprehensive test covering:ManagedPrincipalstrackingAcceptance Tests
Four new scenarios following PR #1146 naming conventions:
Behavior
spec.principalsis specified, operator manages ALL membershipspec.principalsis empty/nil, operator doesn't touch membershipManagedPrincipalsflag allows proper cleanup on deletionRelates to #1146
Relates to https://redpandadata.atlassian.net/browse/UX-430