-
Notifications
You must be signed in to change notification settings - Fork 553
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 Telemetry and Licence nag #17163
Conversation
a129b79
to
3196882
Compare
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46364#018e523b-1a87-44c9-8ba4-a7b7df23d0a2 ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46408#018e5442-73e0-465a-b573-d1c9d42ebf03 |
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. One question about ranges relative to another change I have pending.
@@ -252,6 +255,9 @@ metrics_reporter::build_metrics_snapshot() { | |||
snapshot.has_oidc = config::oidc_is_enabled_kafka() | |||
|| config::oidc_is_enabled_http(); | |||
|
|||
snapshot.has_rbac | |||
= !_role_store.local().range([](auto const&) { return true; }).empty(); |
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 have a change on this pr that takes the std::ranges
out of the role_store
interface.
So in this case we would wind up constructing a container of all role names to satisfy this query. For this usage, should we just add a role_store::empty
method instead?
Other alternative is to leave the view in the interface, which might be preferable. That same PR breaks the near tree-wide dependency on role_store.h
fwiw.
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.
So in this case we would wind up constructing a container of all role names to satisfy this query. For this usage, should we just add a
role_store::empty
method instead?
Sufficient.
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
Signed-off-by: Ben Pope <ben@redpanda.com>
Fixes redpanda-data/core-internal#1137 Signed-off-by: Ben Pope <ben@redpanda.com>
Fixes redpanda-data/core-internal#1136 Signed-off-by: Ben Pope <ben@redpanda.com>
3196882
to
1e1105b
Compare
Changes in force-push :
|
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
First 2 commits are from #17089
Backports Required
Release Notes