-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RBAC: Add ability to delete role ACLs when deleting the role itself in the Admin API #17437
RBAC: Add ability to delete role ACLs when deleting the role itself in the Admin API #17437
Conversation
String to bool, irrespective of character case
- type: bool - required: false - default: false - desc: if true, delete ACLs bound to the deleted role
ed26643
to
61b2ae0
Compare
src/v/redpanda/admin/security.cc
Outdated
{std::move(role_binding_filter)}, 5s); | ||
|
||
for (const auto& r : results) { | ||
co_await throw_on_error(*req, r.error, model::controller_ntp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be preferable to fail silently here. Idk. Any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably log it... or maybe include it in the response but still reutrn a 2XX response code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably log it
As in "log it and don't fail" or "log it and fail"? I'm kind of leaning toward just a log line and success, since any error here is functionally non-retryable (b/c the role is already gone).
include it in the response...
The success code here is 204 (No Content) so not much room for "extra" context. Might be confusing to have two different success modes.
61b2ae0
to
039fa86
Compare
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46920#018e818f-c6de-4aea-a983-e7a18619e4f3 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46920#018e818f-c6e1-48f0-8718-1a9e350c4038 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46941#018e82a3-a8e5-457c-8342-63e49247869f |
While deleting a role, also delete ACLs if the `delete_acls` query param is present and alpha-convertible to bool{true}. We can do this by submitting a single delete_acls command to the cluster controller with a "catch-all" filter for any acl entry bound to the corresponding role principal. This is NOT atomic with respect to the corresponding role management command. It is possible that role deletion could succeed while some or all ACL deletions return an error. In this case, the delete_role operation will not be undone, but ACLs may be deleted manually on the client side as needed.
Default "User", prepended to the `--allow-principal` value. This is useful for testing "delete everything" behavior on the DELETE role endpoint.
039fa86
to
4b53c22
Compare
force push to log failed ACL deletions instead of failing the request outright |
new failures in https://buildkite.com/redpanda/redpanda/builds/46941#018e8291-c049-483c-8af1-8fce00235b49:
new failures in https://buildkite.com/redpanda/redpanda/builds/46941#018e8291-c04e-47bc-bcf7-d98c435bd2c8:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
CI Failures:
|
Closes https://github.com/redpanda-data/core-internal/issues/1203
Backports Required
Release Notes