Skip to content

security: store ACLs in a non-contiguous chunked set - #30967

Merged
WillemKauf merged 1 commit into
redpanda-data:devfrom
WillemKauf:trie-chunked-vector
Jun 30, 2026
Merged

security: store ACLs in a non-contiguous chunked set#30967
WillemKauf merged 1 commit into
redpanda-data:devfrom
WillemKauf:trie-chunked-vector

Conversation

@WillemKauf

@WillemKauf WillemKauf commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

acl_store::_acls was an absl::node_hash_map, chosen for reference stability. Unfortunately this means allocations are contiguous, which can lead to oversized allocations and OOMs.

Rewrite it using chunked_hash_set; because reference stability across insertions/erasures is still required, we use a second layer of indirection via std::unique_ptr<> to acl_nodes.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v26.1.x
  • v25.3.x
  • v25.2.x

Release Notes

Improvements

  • Prevent oversized allocations/OOMs when using a large number of ACLs in redpanda

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the security::acl_store internal ACL map to avoid large contiguous allocations that can lead to OOMs when many ACLs are present, while preserving the reference-stability requirements used by authorization lookups and the prefix index.

Changes:

  • Replace _acls storage from absl::node_hash_map to chunked_hash_map and introduce a std::unique_ptr-owned node to keep referenced data address-stable.
  • Update ACL lookup, iteration, removal, and reset paths to dereference the new node layout (pattern/entries).
  • Add Bazel dependency for chunked_hash_map.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/v/security/BUILD Adds //src/v/container:chunked_hash_map dependency for the new container usage in acl_store.
src/v/security/acl.cc Updates lookups/iteration/removal to use _acls values as heap-stable nodes (unique_ptr indirection).
src/v/security/acl_store.h Replaces _acls type with chunked_hash_map + heap node indirection to preserve reference stability without large contiguous allocations.
Comments suppressed due to low confidence (1)

src/v/security/acl_store.h:112

  • chunked_hash_map’s default hashers (see container/chunked_hash_map.h) return uint64_t and are noexcept. This custom hasher returns size_t and isn’t marked noexcept; matching the 64-bit/noexcept pattern would keep hashing consistent with other chunked_hash_map users and avoids any potential truncation on non-64-bit platforms.
    struct resource_pattern_hash {
        using is_transparent = void;
        // absl::HashOf already mixes well; tell unordered_dense not to re-mix.
        using is_avalanching = void;
        size_t operator()(const auto& p) const {
            return absl::HashOf(pattern_key(p));
        }
    };

Comment thread src/v/security/acl_store.h Outdated
`acl_store::_acls` was an `absl::node_hash_map`, chosen for reference
stability. Unfortunately this means allocations are contiguous, which can
lead to oversized allocations and OOMs.

Rewrite it using `chunked_hash_set`; because reference stability across
insertions/erasures is still required, we use a second layer of indirection
via `std::unique_ptr<>` to `acl_node`s.
@WillemKauf
WillemKauf force-pushed the trie-chunked-vector branch from 746dfd4 to 635cd75 Compare June 30, 2026 18:36
@WillemKauf WillemKauf changed the title security: store ACLs in a non-contiguous chunked map security: store ACLs in a non-contiguous chunked set Jun 30, 2026
@WillemKauf
WillemKauf requested a review from wdberkeley June 30, 2026 19:16
@vbotbuildovich

Copy link
Copy Markdown
Collaborator

CI test results

test results on build#86529
test_status test_class test_method test_arguments test_kind job_url passed reason test_history
FLAKY(PASS) EndToEndCloudTopicsMaintenanceToggleTest test_toggle_maintenance null integration https://buildkite.com/redpanda/redpanda/builds/86529#019f19e7-f7a7-4a82-bcb9-83550c5e23d2 10/11 Test PASSES after retries.No significant increase in flaky rate(baseline=0.0000, p0=1.0000, reject_threshold=0.0100. adj_baseline=0.1000, p1=0.3487, trust_threshold=0.5000) https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=EndToEndCloudTopicsMaintenanceToggleTest&test_method=test_toggle_maintenance

@oleiman oleiman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. i also had assumed we needed resource_pattern lookups on _acls.

@WillemKauf
WillemKauf merged commit 5d027da into redpanda-data:dev Jun 30, 2026
19 checks passed
* they must not move when other patterns are added or removed.
*/
struct acls_node {
resource_pattern pattern;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late comment, but should we consider making the pattern const?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants