acl_store: Return prefix matches as a view #17152

oleiman · 2024-03-17T20:34:40Z

This PR refactors acl_store slightly to return prefix ACL matches as a view. This saves a vector allocation on the authZ path.

Also breaks some header dependencies in an effort to isolate view templates to the specific compilation units where they are needed.

Backports Required

Release Notes

none

oleiman · 2024-03-18T18:27:19Z

/dt

vbotbuildovich · 2024-03-18T20:42:05Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46392#018e5303-a45f-4eeb-a347-a21a6f959733

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46392#018e5303-a454-4703-b894-229c25082dc5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46392#018e53c4-6f4e-406c-8b8d-fb68583a21db

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46392#018e53c4-6f59-46c1-934a-68f27b541722

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46577#018e6239-67f3-4841-beb4-6da20c4f7679

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46577#018e624b-6761-4629-b60f-54b95b80a883

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46595#018e639c-c303-4eaa-8034-42e186084ef2

oleiman · 2024-03-18T21:52:32Z

Builds on #17025

This PR is for: https://github.com/redpanda-data/redpanda/compare/d2ad0dd32e21d4e2a637c6195af6a52f00f8b408..cc8fd027d6373ca0b0f523c2c4a8805bfea66741

dotnwat · 2024-03-21T17:46:40Z

src/v/security/acl_store.h

+    template<typename RefT>
+    static auto get_prefix_view(


Do we need to warn users of this interface about any invalidation foot-guns since presumably they are not only hanging on to references into entries in the container, but now also iterators?

Ugh, yeah that's a great point. This usage should be safe -- call chain has exactly one consumer (authorizer::authorized) and no yield points -- but stern comments here and on acl_store::find definitely a good idea.

i figured this was probably the case most of the acl/security code is in non-futurized code paths. but holding these across co_awaits is a constant footgun we encounter.

Indeed. Also I had btree_map erroneously filed away as iter stable. Thankfully reminded that's not the case in core panda the other day!

src/v/security/acl_store.h

dotnwat · 2024-03-21T17:58:11Z

src/v/security/role.cc

+                         return _members_store;
+                     });
+               })
+               | std::views::filter(std::forward<decltype(pred)>(pred))


confused about this one. it looks like you want std::move(pred) here without the forwarding?

yeah definitely.

dotnwat · 2024-03-21T18:02:00Z

src/v/security/authorizer.cc

+template auth_result authorizer::authorized(
+  const kafka::transactional_id&,
+  acl_operation,
+  const acl_principal&,
+  const acl_host&) const;


header ❤️

This shaves a whole allocation off of each call to `authorizer::authorized` both with and without roles. The major consequence of this is that `acl_store.h` is included (transitively via `authorizer.h`) in many places, so we're bleeding range view templates throughout the source tree. Subsequent commits seek to isolate those templates by breaking header dependencies. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

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

And explicitly instantiate, since the set of resource types is bounded and small. This breaks a rather large dependency on `role_store.h` throughout the source tree. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

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

oleiman · 2024-03-21T23:23:32Z

force push some comments and correct a perfect forward to a move in role_store

dotnwat · 2024-03-21T23:52:13Z

changes look good

vbotbuildovich · 2024-03-22T02:03:13Z

new failures in https://buildkite.com/redpanda/redpanda/builds/46595#018e639c-c2fe-44eb-a5cf-f9966cc97ba7:

"rptest.tests.read_replica_e2e_test.TestReadReplicaService.test_identical_lwms_after_delete_records.partition_count=5.cloud_storage_type=CloudStorageType.S3"

oleiman · 2024-03-22T17:22:05Z

CI Failures:

BenPope

Looks great

BenPope · 2024-03-28T09:53:34Z

src/v/security/acl_store.h

@@ -188,8 +142,103 @@ class acl_store {
        }
    };

-    absl::btree_map<resource_pattern, acl_entry_set, resource_pattern_compare>
-      _acls;
+    using container_type = absl::


nitpick: This commit would have been easier to review it was split into two steps:

Pure refactor (code movement)

Behaviour change

Yeah, I have several regrets about the structure of this PR. Sorry for time suck 😕

BenPope · 2024-03-28T10:07:19Z

src/v/security/CMakeLists.txt

@@ -28,6 +28,7 @@ v_cc_library(
    krb5_configurator.cc
    gssapi_principal_mapper.cc
    role.cc
+    authorizer.cc


nitpick: I usually try to keep this list sorted.

BenPope · 2024-03-28T10:10:08Z

src/v/security/fwd.h

@@ -18,6 +18,9 @@ class authorizer;
 class credential_store;
 class ephemeral_credential_store;
 class role_store;
+class role;


nitpick: I usually try to keep this list sorted

BenPope · 2024-03-28T10:26:17Z

src/v/security/acl_store.h

+};
+
+/*
+ * A lightweight container of references to ACL entries. An instance of this


nitpick: * A lightweight view of ACL entries.

BenPope · 2024-03-28T10:29:17Z

src/v/security/role_store.h

-          role_name_view, /* role_name  */
-          std::function<const members_store_type&(void)>>& e,
-        const security::role_member& member) {
+      [](const role_accessor& e, const RoleMember auto& member) {
          const auto [name, get_ms] = e;


nitpick: copy not required (I changed the std::function to an ss::noncopyable_function)

Suggested change

const auto [name, get_ms] = e;

const auto& [name, get_ms] = e;

BenPope · 2024-03-28T10:32:14Z

src/v/security/authorizer.h

@@ -16,9 +16,9 @@
 #include "model/fundamental.h"
 #include "security/acl.h"
 #include "security/acl_store.h"
+#include "security/fwd.h"


nitpick: These don't seem to be required:

#include "kafka/types.h" #include "model/fundamental.h" #include <assert.h>

BenPope · 2024-03-28T10:35:33Z

src/v/security/authorizer.h

-        return auth_result::opt_acl_match(
-          principal, host, operation, resource_name, std::nullopt);
-    }
+      const acl_host& host) const;


suggestion: You could also move auth_result::operator<<

oleiman · 2024-03-28T16:07:48Z

@BenPope - thanks for suggestions. I'm going to defer those to a fast follow to clear the log jam here.

dotnwat · 2024-03-28T17:28:26Z

src/v/security/acl_store.h

+        for (auto& binding : bindings) {
+            auto& entries = _acls[binding.pattern()];
+            entries.insert(binding.entry());
+            entries.rehash();


rehash after the loop? seems like rehashing after each insert could be an expensive thing?

Sure. fwiw, I didn't introduce this, I think it just shows up in the diff because I ripped acl_entry_set and git is...git. But I have a stack of minor changes coming and can stick that on top.

yeh nbd, just drive by noticing things. maybe i had added this long ago?

Ya, I assume it was from the OG implementation. I looked at it again and I think we'd need a container or something to keep track of which entries matched, so maybe not much to be gained anyway

oleiman self-assigned this Mar 17, 2024

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

oleiman mentioned this pull request Mar 17, 2024

Role based authorization #17025

Merged

6 tasks

oleiman force-pushed the security/acl-store-alloc branch 3 times, most recently from 6703782 to cc8fd02 Compare March 18, 2024 18:26

oleiman requested a review from BenPope March 18, 2024 18:26

oleiman mentioned this pull request Mar 18, 2024

rbac: Add Telemetry and Licence nag #17163

Merged

6 tasks

oleiman force-pushed the security/acl-store-alloc branch 3 times, most recently from 22b394a to 4b6ce35 Compare March 21, 2024 17:10

oleiman marked this pull request as ready for review March 21, 2024 17:22

dotnwat reviewed Mar 21, 2024

View reviewed changes

oleiman added 4 commits March 21, 2024 16:21

security/role: Move some implementations to role.cc

095f10b

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

security/authorizer: Move template authorized to .cc

022c261

And explicitly instantiate, since the set of resource types is bounded and small. This breaks a rather large dependency on `role_store.h` throughout the source tree. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>

security/acl_store: Breaking header dependencies

9f7777a

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

oleiman force-pushed the security/acl-store-alloc branch from 4b6ce35 to 9f7777a Compare March 21, 2024 23:22

BenPope reviewed Mar 28, 2024

View reviewed changes

BenPope approved these changes Mar 28, 2024

View reviewed changes

BenPope mentioned this pull request Mar 28, 2024

security/rbac: Add metrics #17463

Merged

6 tasks

oleiman merged commit bc8a617 into redpanda-data:dev Mar 28, 2024
17 checks passed

dotnwat reviewed Mar 28, 2024

View reviewed changes

oleiman mentioned this pull request Mar 28, 2024

security: Various minor cleanups #17467

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acl_store: Return prefix matches as a view #17152

acl_store: Return prefix matches as a view #17152

oleiman commented Mar 17, 2024

oleiman commented Mar 18, 2024

vbotbuildovich commented Mar 18, 2024 •

edited

oleiman commented Mar 18, 2024

dotnwat Mar 21, 2024

oleiman Mar 21, 2024

dotnwat Mar 21, 2024

oleiman Mar 22, 2024

dotnwat Mar 21, 2024

oleiman Mar 21, 2024

dotnwat Mar 21, 2024

oleiman commented Mar 21, 2024

dotnwat commented Mar 21, 2024

vbotbuildovich commented Mar 22, 2024

oleiman commented Mar 22, 2024

BenPope left a comment

BenPope Mar 28, 2024

oleiman Mar 28, 2024

BenPope Mar 28, 2024

BenPope Mar 28, 2024

BenPope Mar 28, 2024

BenPope Mar 28, 2024

BenPope Mar 28, 2024

BenPope Mar 28, 2024

oleiman commented Mar 28, 2024

dotnwat Mar 28, 2024

oleiman Mar 28, 2024

dotnwat Apr 2, 2024

oleiman Apr 2, 2024

	const auto [name, get_ms] = e;
	const auto& [name, get_ms] = e;

acl_store: Return prefix matches as a view #17152

acl_store: Return prefix matches as a view #17152

Conversation

oleiman commented Mar 17, 2024

Backports Required

Release Notes

oleiman commented Mar 18, 2024

vbotbuildovich commented Mar 18, 2024 • edited

oleiman commented Mar 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiman commented Mar 21, 2024

dotnwat commented Mar 21, 2024

vbotbuildovich commented Mar 22, 2024

oleiman commented Mar 22, 2024

BenPope 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

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 28, 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

vbotbuildovich commented Mar 18, 2024 •

edited