cluster_link: shadow link roles migrator - #30946
Merged
nguyen-andrew merged 10 commits intoJun 30, 2026
Merged
Conversation
Member
Author
|
/ci-repeat 1 |
Collaborator
CI test resultstest results on build#86411
test results on build#86450
test results on build#86488 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the shadow-link RBAC role mirroring feature by adding a controller-leader singleton “roles migrator” task that periodically reconciles in-scope roles (create/update/delete) from the source cluster onto the destination, along with unit/fixture/e2e coverage and the necessary security-service plumbing to read/apply roles locally.
Changes:
- Add
roles_migratortask + factory, including source capability/permission gating, filtering, and full-mirror reconciliation logic. - Introduce
reconcile_roleshelper and add C++ unit/fixture tests for both reconcile and migrator behavior. - Extend cluster-link security dependencies to support role operations and destination role enumeration; add ducktape e2e coverage for plaintext, Kafka-source “unsupported API”, and SASL/SCRAM authz scenarios.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rptest/tests/shadow_link_role_sync_test.py | New ducktape e2e suite validating role mirroring lifecycle, pausing, scoping, Kafka-source behavior, and SASL/SCRAM authz. |
| tests/rptest/tests/cluster_linking_test_base.py | Registers the new “Roles Migrator Task” in controller-locked task expectations. |
| tests/rptest/tests/cluster_linking_e2e_test.py | Enables role sync options in an existing task-state test to cover task state transitions with the new task present. |
| src/v/kafka/client/test/cluster_mock.h | Adds remove_supported_version to simulate missing API support in tests. |
| src/v/cluster_link/tests/roles_migrator_test.cc | Fixture tests for migrator state transitions and mirroring behavior (including unsupported API and permission/RBAC gates). |
| src/v/cluster_link/tests/role_reconcile_test.cc | Unit tests for reconcile_roles diffing behavior. |
| src/v/cluster_link/tests/deps.h | Extends fake security service to support role CRUD, RBAC-active gating, and role enumeration. |
| src/v/cluster_link/tests/BUILD | Adds new gtest targets for roles migrator and reconcile logic. |
| src/v/cluster_link/service.cc | Wires in roles migrator registration and passes role_store/feature_table into cluster-link security service. |
| src/v/cluster_link/roles_migrator.h | Declares the new controller-locked roles migrator task and factory. |
| src/v/cluster_link/roles_migrator.cc | Implements periodic fetch/filter/reconcile/apply pipeline with task-state reporting. |
| src/v/cluster_link/role_reconcile.h | Declares reconcile_roles interface and role mutation plan structure. |
| src/v/cluster_link/role_reconcile.cc | Implements full-mirror diffing between filtered source/destination role sets. |
| src/v/cluster_link/deps.h | Extends security_service interface for role operations and destination role enumeration. |
| src/v/cluster_link/deps.cc | Implements the extended security_service using security_frontend + role_store + feature_table. |
| src/v/cluster_link/BUILD | Adds build targets/deps for roles_migrator and role_reconcile libraries and wires them into cluster_link. |
nguyen-andrew
force-pushed
the
sl-role-sync-task-pr2
branch
from
June 29, 2026 15:20
e7cbf40 to
a2d67d7
Compare
Member
Author
|
Force push to fix clang-tidy |
nguyen-andrew
requested review from
bartoszpiekny-redpanda,
dotnwat and
mnajda-redpanda
June 29, 2026 15:33
dotnwat
reviewed
Jun 30, 2026
dotnwat
left a comment
dotnwat
left a comment
Member
There was a problem hiding this comment.
lgtm. just a couple questions
Comment on lines
+83
to
+86
| "Insufficient permissions to replicate roles. Requires {:08x}, has " | ||
| "{:08x}", | ||
| roles_migrator::required_permissions, | ||
| cluster.get_cluster_authorized_operations()); |
Member
There was a problem hiding this comment.
does this mean insufficient permissions on the destination?
Member
Author
There was a problem hiding this comment.
This means the shadow/destination cluster's Kafka client has insufficient permissions (DESCRIBE CLUSTER) to get the roles from the source cluster. I can adjust the wording to make things more clear to distinguish between source and destination/shadow.
| : ""); | ||
| vlog(logger().warn, "{}", msg); | ||
| co_return state_transition{ | ||
| .desired_state = model::task_state::faulted, |
Member
There was a problem hiding this comment.
is there a scenario in which you'd want to treat the role fetching returned error as link_unavailable as opposed to faulted? presumably these error conditions affect things like back-off behavior?
Member
Author
There was a problem hiding this comment.
Looking at the task framework, it doesn't seem like either state changes the retry/back-off behavior - it seems like it's mostly for status reporting to the operator. That said, I do agree that link_unavailable is a better fit here - I'll adjust it.
Extend the security_service deps interface with role create/update/delete, an RBAC-active check, and a destination-role reader so the roles migrator can apply and diff roles through the existing injection seam.
nguyen-andrew
force-pushed
the
sl-role-sync-task-pr2
branch
2 times, most recently
from
June 30, 2026 03:46
cb4a7c7 to
65d2864
Compare
Member
Author
|
Force pushes to address PR comments:
Also added additional commits:
These additions expanded the review a bit, so happy to pop these off to a separate PR if it makes things easier. |
nguyen-andrew
commented
Jun 30, 2026
| } | ||
|
|
||
| ss::future<chunked_vector<role_with_members>> | ||
| role_store::all_roles_with_members() const { |
Member
Author
There was a problem hiding this comment.
For reference, this was added to match acl_store::all_bindings() and security_manager::fill_snapshot
dotnwat
reviewed
Jun 30, 2026
| co_await ss::coroutine::maybe_yield(); | ||
| } | ||
|
|
||
| for (const auto& [member, role_names] : _members_store) { |
Member
There was a problem hiding this comment.
what makes all of the loops in all_roles_with_members safe against concurrent modifications to the container, now that the iterations can be suspended?
Collaborator
Retry command for Build#86488please wait until all jobs are finished before running the slash command |
mnajda-redpanda
left a comment
mnajda-redpanda
left a comment
Contributor
There was a problem hiding this comment.
Some nit-picky comments, feel free to ignore :)
Comment on lines
+645
to
+653
| chunked_vector<security::role_with_members> out; | ||
| for (const auto& [name, role] : _roles) { | ||
| if (pred(name)) { | ||
| out.push_back( | ||
| security::role_with_members{ | ||
| .name = name, .role = security::role{role.members()}}); | ||
| } | ||
| } | ||
| return out; |
Contributor
There was a problem hiding this comment.
Nit:
I guess this could be rewritten using ranges. I find it more readable this way.
return _roles
| std::views::filter([&pred](const auto& entry) { return pred(entry.first); })
| std::views::transform([](const auto& entry) {
const auto& [name, role] = entry;
return security::role_with_members{
.name = name,
.role = security::role{role.members()}
};
});
Comment on lines
+38
to
+41
| ASSERT_EQ(changes.to_create.size(), 1); | ||
| EXPECT_EQ(changes.to_create[0].name, security::role_name{"a"}); | ||
| EXPECT_TRUE(changes.to_update.empty()); | ||
| EXPECT_TRUE(changes.to_delete.empty()); |
Contributor
There was a problem hiding this comment.
Nit:
And here you can use matcher based EXPECT_THAT.
EXPECT_THAT(changes.to_create, testing::ElementsAre(/* matcher for the element */));
EXPECT_THAT(changes.to_update, testing::IsEmpty());
EXPECT_THAT(changes.to_delete, testing::IsEmpty());
Comment on lines
+217
to
+219
| EXPECT_TRUE( | ||
| fixture().security_service().roles().contains( | ||
| security::role_name{"synced-keep"})); |
Contributor
There was a problem hiding this comment.
Nit:
I would also recommend using matchers here, for improved output on test failure (for example https://godbolt.org/z/TjP3rGrnr):
EXPECT_THAT(
fixture().security_service().roles(),
testing::Contains(security::role_name{"synced-keep"})
);
Comment on lines
+21
to
+27
| auto to_map = [](chunked_vector<security::role_with_members> roles) { | ||
| chunked_hash_map<security::role_name, security::role> m; | ||
| m.reserve(roles.size()); | ||
| for (auto& r : roles) { | ||
| m.emplace(std::move(r.name), std::move(r.role)); | ||
| } | ||
| return m; |
Contributor
There was a problem hiding this comment.
Nit:
I'd probably use ranges here as well
Suggested change
| auto to_map = [](chunked_vector<security::role_with_members> roles) { | |
| chunked_hash_map<security::role_name, security::role> m; | |
| m.reserve(roles.size()); | |
| for (auto& r : roles) { | |
| m.emplace(std::move(r.name), std::move(r.role)); | |
| } | |
| return m; | |
| auto to_map = [](chunked_vector<security::role_with_members> roles) -> chunked_hash_map<security::role_name, security::role> { | |
| roles | | |
| std::views::transform([](auto&& r) { | |
| return {std::move(r.name), std::move(r.role)} | |
| }); |
nguyen-andrew
force-pushed
the
sl-role-sync-task-pr2
branch
from
June 30, 2026 18:32
973a8c4 to
65d2864
Compare
reconcile_roles computes the full-mirror create/update/delete sets from two already-filtered role lists. Pure and unit-tested so deletion-safety is covered without a cluster fixture.
A controller-locked task that fetches all roles from the source via DescribeRedpandaRoles, selects by configured name filters, and full-mirrors them onto the destination. Fails safe: a failed or errored fetch skips the apply entirely, so no deletes happen on incomplete data.
Drives the migrator through a cluster_mock DescribeRedpandaRoles handler and a role-aware fake security_service: fetch+apply with filter scoping, deletion propagation, and fail-safe (a top-level fetch error faults the task and attempts zero deletes).
Add the role-sync e2e suite: a reusable RoleSyncTestBase holding the shared role helpers (so later SASL / Apache-Kafka-source topologies can reuse them) plus the first concrete test against a plaintext source. Two-cluster scenario: source roles created with user and group members, shadow link configured with a prefix-include role_name_filter. Asserts the destination mirrors initial state, membership additions, and deletions; excludes out-of-scope roles; and pausing/resuming role sync holds back a change made while paused, then drains it on resume.
The roles migrator is full-replace authoritative, but only over roles that match the link's name filters. Cover both halves of that boundary: - For an in-scope role, a member added directly on the destination is stripped and a destination-side delete is undone on the next reconcile, so the destination is driven back to the source. - A destination role outside the filter scope, with no source counterpart, is left untouched: the migrator never selects it for update or delete.
A link whose source is vanilla Kafka cannot negotiate DescribeRedpandaRoles, so the roles task parks LINK_UNAVAILABLE with a reason and mirrors nothing, rather than faulting. Topic syncing on the same link keeps working, since it rides standard Kafka APIs: the parked task is isolated from its siblings.
With SASL on both clusters, a synced role plus its role-bound ACL authorizes a member on the destination while a non-member is denied, proving shadow-linked RBAC actually evaluates rather than merely appearing.
A link principal without cluster DESCRIBE parks the roles task LINK_UNAVAILABLE; granting DESCRIBE on the source flips it ACTIVE and the role mirrors, exercising the real SASL client-auth path end to end.
Add the roles migrator to CONTROLLER_LOCKED_TASKS and enable role sync on the link so the existing source-outage / controller-placement assertions also exercise the controller-locked roles task.
nguyen-andrew
force-pushed
the
sl-role-sync-task-pr2
branch
from
June 30, 2026 19:00
65d2864 to
e2e4377
Compare
Member
Author
dotnwat
approved these changes
Jun 30, 2026
7 tasks
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.
Implements shadow link role sync, the follow-on to the groundwork merged in
#30932. That PR landed the role-sync configuration surface inert; this PR adds
the migrator that consumes it, so configuring role sync on a link now actually
syncs roles.
The roles migrator is a periodic, controller-leader-singleton task that
full-mirrors RBAC roles from the source cluster onto the shadow-link
destination, within the configured role-name filter scope. New roles, member
changes, and source-side deletions all propagate; in-scope destination roles
missing from the source are pruned, matching shadow-DR semantics. Source and
apply errors are surfaced through the task state.
Covered by
reconcile_rolesunit tests, C++ fixture tests for the migratortask, and a ducktape e2e suite (full mirror, in-scope authority, functional
authz over synced roles and ACLs, parking against an Apache Kafka source, and
permission-grant recovery).
Fixes CORE-16456.
Backports Required
Release Notes
Features
destination. Configure role sync on the link with a sync interval and
role-name filters to select which roles are kept in sync.