Cluster integration for `role_store` and related structures. #16926

oleiman · 2024-03-07T00:39:05Z

This PR introduces a new record batch type for replicated management of the in-memory role_store

model::record_batch_type::role_management_cmd governs four new controller commands:

create_role_cmd
delete_role_cmd
update_role_cmd
rename_role_cmd

Includes integration of the role_store itself into cluster::controller, cluster::security_manager as a sharded service, handlers for the commands, and cluster::security_frontend API.

Also includes a feature gate for the new commands (feature::role_based_access_control) and a fixture test for the whole kit.

Closes https://github.com/redpanda-data/core-internal/issues/1099

Backports Required

Release Notes

Introduce feature::role_based_access_control

oleiman · 2024-03-07T00:42:57Z

/dt

src/v/cluster/security_manager.cc

src/v/cluster/security_frontend.cc

vbotbuildovich · 2024-03-07T20:11:03Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45807#018e1a40-fa2f-4866-9b43-a2d52be47444

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45953#018e2b00-fa98-45ef-bd06-d0957ad979ce

oleiman · 2024-03-07T20:59:23Z

force push for review comments

rockwotj

LGTM, a couple of things I noticed on the second pass

src/v/cluster/security_manager.h

rockwotj · 2024-03-07T21:03:39Z

src/v/cluster/security_frontend.h

    ss::future<std::error_code> update_user(
      security::credential_user,
      security::scram_credential,
      model::timeout_clock::time_point);

+    // Should be called ONLY on the controller leader


nit: it would be nice to add proper documentation to this class and these methods.

Also, by leader can we specify this needs to be the leader node, this way it's more clear that it can be called from any shard, not just shard0 on the leader.

fair points

updated existing comments, but I'm going hold off full documentation of these methods for a follow-on PR. I don't want to clutter this diff up too much.

oleiman · 2024-03-07T22:05:43Z

force push CR cleanups

dotnwat · 2024-03-08T03:26:57Z

src/v/cluster/commands.h

+using create_role_cmd = controller_command<
+  security::role_name,
+  security::role,


I think it is generally a good idea to completely ignore either the key or the value and create a versioned structure for the payload of each command to future proof yourself against needing to add additional context to the command. the rename command is a good example. sure, using key=from-name and value=to-name works now, but what if you need some extra state later? you'd probably have to add a replacement command.

@mmaslankaprv is there a new/better way to handle this?

TBH we don't use the key for anything AFAIK, so I would rather just have a value and that would mean people would make their own structs for each commands, so you'd get the serde benefits. There are also things like this serde_only tag which should be the default.

I also don't love this special batch type pattern, I would rather just have the key or a header specify who owns a controller command

serde_only is the default...noticed that yesterday and meant to remove the param in this PR.

create a versioned structure for the payload

that makes good sense, thanks. I see some examples of that pattern in nearby code now.

@dotnwat - this is done if you're interested in taking another look

src/v/features/feature_table.h

oleiman · 2024-03-08T06:44:36Z

force push contents:

fix required version on rbac feature spec
- move some bits around in the test (was testing my own bug before 😛)
add version payload structs for role_management_cmds

oleiman · 2024-03-08T06:47:05Z

src/v/cluster/commands.h

+using create_role_cmd = controller_command<
+  int8_t, // unused
+  upsert_role_cmd_data,
+  create_role_cmd_type,
+  model::record_batch_type::role_management_cmd>;
+
+using delete_role_cmd = controller_command<
+  int8_t, // unused
+  security::role_name,
+  delete_role_cmd_type,
+  model::record_batch_type::role_management_cmd>;
+
+using update_role_cmd = controller_command<
+  int8_t, // unused
+  upsert_role_cmd_data,
+  update_role_cmd_type,
+  model::record_batch_type::role_management_cmd>;


considered collapsing create and update into one, but that would complicate the handler (and payload) somewhat. is there any reason to minimize the number of controller command types?

I cant think of any reasons

vbotbuildovich · 2024-03-08T09:01:34Z

new failures in https://buildkite.com/redpanda/redpanda/builds/45852#018e1d12-b6b0-4991-a079-f2a2df1caebb:

"rptest.tests.data_transforms_test.DataTransformsLoggingTest.test_logs_volume"

new failures in https://buildkite.com/redpanda/redpanda/builds/45951#018e2a36-0ec4-4da2-b568-4723fb9dfc8a:

"rptest.tests.write_caching_test.WriteCachingPropertiesTest.test_properties"

oleiman · 2024-03-08T17:42:57Z

CI Failure:

CI Failure (timeout waiting for transform workload) in DataTransformsLoggingTest.data_transforms_test #16961

src/v/cluster/commands.h

oleiman · 2024-03-08T18:18:38Z

force push to add versioned delete_role_cmd_data

oleiman · 2024-03-10T21:11:12Z

/ci-repeat 1
skip-redpanda-build
release
skip-units

oleiman · 2024-03-10T23:14:19Z

/ci-repeat 1

pgellert

lgtm

Batch type (NEW): model::record_batch_type::role_management_cmd payload types: - upsert_role_cmd_data - (role_name, role) - rename_role_cmd_data - (role_name, role_name) - delete_role_cmd_data - (role_name) controller_commands: - create_role_cmd - (int8_t /*unused*/, upsert_role_cmd_data) - delete_role_cmd - (int8_t /*unused*/, delete_role_cmd_data) - update_role_cmd - (int8_t /*unused*/, upsert_role_cmd_data) - rename_role_cmd - (int8_t /*unused*/, rename_role_cmd_data) Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

cluster::errc { ... role_exists, // Cmd failed because the target role exists role_does_not_exist, // Cmd failed because the target role does not exist } Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Fixup for clang-tidy (and general preferred practice). No functional changes.

Adds a sharded<security::role_store> to cluster::controller, which shared it down into cluster::security_manager (alongside credential_store and authorizer). The sharded role_store service is started and stopped by the controller in the usual way. Also adds controller::get_role_store. The meat of the change is in security_manager, where high level semantics and error handling for the various role_managment_cmds are applied. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

create, delete, update, rename All requests are subject to the role_based_access_control feature gate. This interface should be consumed ONLY on the controller leader. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

New file: security_frontend_test.cc Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman · 2024-03-11T21:29:09Z

force_push rebase on dev to fix feature_table merge conflict

oleiman self-assigned this Mar 7, 2024

github-actions bot added the area/redpanda label Mar 7, 2024

oleiman force-pushed the rbac/controller-cmd branch from a89fa17 to 071cfb9 Compare March 7, 2024 00:42

oleiman requested a review from pgellert March 7, 2024 16:52

oleiman force-pushed the rbac/controller-cmd branch 2 times, most recently from b4e06d3 to 948bec4 Compare March 7, 2024 17:42

oleiman marked this pull request as ready for review March 7, 2024 17:56

oleiman requested review from michael-redpanda, rockwotj, graphcareful and BenPope March 7, 2024 18:31

rockwotj reviewed Mar 7, 2024

View reviewed changes

src/v/cluster/security_manager.cc Outdated Show resolved Hide resolved

src/v/cluster/security_manager.cc Outdated Show resolved Hide resolved

src/v/cluster/security_frontend.cc Show resolved Hide resolved

oleiman force-pushed the rbac/controller-cmd branch from 948bec4 to 2260cd1 Compare March 7, 2024 20:58

oleiman requested a review from rockwotj March 7, 2024 20:59

rockwotj previously approved these changes Mar 7, 2024

View reviewed changes

oleiman dismissed rockwotj’s stale review via dbcfe8f March 7, 2024 22:03

oleiman force-pushed the rbac/controller-cmd branch from 2260cd1 to dbcfe8f Compare March 7, 2024 22:03

rockwotj previously approved these changes Mar 7, 2024

View reviewed changes

dotnwat reviewed Mar 8, 2024

View reviewed changes

oleiman dismissed rockwotj’s stale review via f4b050a March 8, 2024 06:40

oleiman force-pushed the rbac/controller-cmd branch from dbcfe8f to f4b050a Compare March 8, 2024 06:40

oleiman commented Mar 8, 2024

View reviewed changes

oleiman requested review from dotnwat and rockwotj March 8, 2024 17:38

rockwotj previously approved these changes Mar 8, 2024

View reviewed changes

src/v/cluster/commands.h Outdated Show resolved Hide resolved

oleiman dismissed rockwotj’s stale review via e482955 March 8, 2024 18:17

oleiman force-pushed the rbac/controller-cmd branch from f4b050a to e482955 Compare March 8, 2024 18:17

oleiman requested a review from rockwotj March 8, 2024 18:18

rockwotj previously approved these changes Mar 8, 2024

View reviewed changes

pgellert previously approved these changes Mar 11, 2024

View reviewed changes

oleiman added 7 commits March 11, 2024 14:12

cluster/errc: Role management error codes

7b41d09

cluster::errc { ... role_exists, // Cmd failed because the target role exists role_does_not_exist, // Cmd failed because the target role does not exist } Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

cluster/security_manager: Move do_apply overloads to anon namespace

28c814d

Fixup for clang-tidy (and general preferred practice). No functional changes.

features: Add feature::role_based_access_control

b589e7b

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

cluster/security_frontend: Role management interface

63614d9

create, delete, update, rename All requests are subject to the role_based_access_control feature gate. This interface should be consumed ONLY on the controller leader. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

cluster/tests: Fixture test for role management frontend

b437374

New file: security_frontend_test.cc Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

oleiman dismissed stale reviews from pgellert and rockwotj via b437374 March 11, 2024 21:28

oleiman force-pushed the rbac/controller-cmd branch from e482955 to b437374 Compare March 11, 2024 21:28

oleiman requested review from rockwotj and pgellert March 11, 2024 21:29

rockwotj approved these changes Mar 12, 2024

View reviewed changes

oleiman merged commit 8471282 into redpanda-data:dev Mar 12, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster integration for `role_store` and related structures. #16926

Cluster integration for `role_store` and related structures. #16926

oleiman commented Mar 7, 2024 •

edited

oleiman commented Mar 7, 2024

vbotbuildovich commented Mar 7, 2024 •

edited

oleiman commented Mar 7, 2024

rockwotj left a comment

rockwotj Mar 7, 2024

oleiman Mar 7, 2024

oleiman Mar 7, 2024

oleiman commented Mar 7, 2024

dotnwat Mar 8, 2024

dotnwat Mar 8, 2024

rockwotj Mar 8, 2024

oleiman Mar 8, 2024

oleiman Mar 11, 2024

oleiman commented Mar 8, 2024

oleiman Mar 8, 2024

rockwotj Mar 8, 2024

vbotbuildovich commented Mar 8, 2024 •

edited

oleiman commented Mar 8, 2024 •

edited

oleiman commented Mar 8, 2024

oleiman commented Mar 10, 2024

oleiman commented Mar 10, 2024

pgellert left a comment

oleiman commented Mar 11, 2024

Cluster integration for role_store and related structures. #16926

Cluster integration for role_store and related structures. #16926

Conversation

oleiman commented Mar 7, 2024 • edited

Backports Required

Release Notes

oleiman commented Mar 7, 2024

vbotbuildovich commented Mar 7, 2024 • edited

oleiman commented Mar 7, 2024

rockwotj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Mar 8, 2024 • edited

oleiman commented Mar 8, 2024 • edited

oleiman commented Mar 8, 2024

oleiman commented Mar 10, 2024

oleiman commented Mar 10, 2024

pgellert left a comment

Choose a reason for hiding this comment

oleiman commented Mar 11, 2024

Cluster integration for `role_store` and related structures. #16926

Cluster integration for `role_store` and related structures. #16926

oleiman commented Mar 7, 2024 •

edited

vbotbuildovich commented Mar 7, 2024 •

edited

vbotbuildovich commented Mar 8, 2024 •

edited

oleiman commented Mar 8, 2024 •

edited