Skip to content

security/acl: move get_resource_type body to acl.cc#30220

Merged
dotnwat merged 2 commits intoredpanda-data:devfrom
pgellert:sr-types-acl
Apr 20, 2026
Merged

security/acl: move get_resource_type body to acl.cc#30220
dotnwat merged 2 commits intoredpanda-data:devfrom
pgellert:sr-types-acl

Conversation

@pgellert
Copy link
Copy Markdown
Contributor

@pgellert pgellert commented Apr 17, 2026

acl.h's consteval get_resource_type() referenced
pandaproxy::schema_registry and kafka types inside its body, forcing
every consumer of security/acl.h to transitively pull both modules
through security's public API.

Move the body into acl.cc with explicit instantiations for the six
known types, matching the existing get_allowed_operations() pattern
in the same file. Drop consteval since all call sites use the function
in runtime contexts. Move schema_registry:types and kafka/protocol
from security's deps to implementation_deps.

Rebuild fan-out on a types.h content-hash bump of
//src/v/pandaproxy/schema_registry/...:

  before: 347 CppCompile actions, 382.6s wall
  after:   71 CppCompile actions,  88.9s wall
  delta:  -276 actions (-79.5%), -294s (-76.8%)

TODO

  • Run microbench on the PR

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

  • none

@pgellert pgellert requested a review from a team April 17, 2026 17:14
@pgellert pgellert self-assigned this Apr 17, 2026
@pgellert pgellert requested review from Copilot and nguyen-andrew and removed request for a team April 17, 2026 17:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Moves security::get_resource_type<T>()’s type-dependent logic out of the public header to reduce transitive dependencies (notably Kafka protocol + Schema Registry types) and improve build fan-out.

Changes:

  • Replaced the header-defined get_resource_type<T>() body in security/acl.h with a declaration, and implemented it in security/acl.cc with explicit instantiations for the supported types.
  • Updated Bazel deps: moved Kafka protocol + Schema Registry types from //src/v/security public deps to implementation_deps.
  • Fixed downstream compilation fallout by adding explicit includes/deps where those types are referenced directly (security tests, cloud_topics frontend).

Reviewed changes

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

Show a summary per file
File Description
src/v/security/acl.h Removes transitive Kafka/SR includes and leaves get_resource_type<T>() as a declaration.
src/v/security/acl.cc Adds the moved get_resource_type<T>() implementation + explicit instantiations.
src/v/security/BUILD Shifts Kafka/SR dependencies to implementation_deps to stop leaking through the public API.
src/v/security/tests/authorizer_test.cc Adds direct include for Kafka protocol types now that acl.h no longer brings them in.
src/v/security/tests/BUILD Adds the needed //src/v/kafka/protocol test dependency.
src/v/cloud_topics/frontend/frontend.cc Adds direct include for Kafka protocol errors now required explicitly.
src/v/cloud_topics/frontend/BUILD Adds //src/v/kafka/protocol dep to match the new include.

acl.h's consteval get_resource_type<T>() referenced
pandaproxy::schema_registry and kafka types inside its body, forcing
every consumer of security/acl.h to transitively pull both modules
through security's public API.

Move the body into acl.cc with explicit instantiations for the six
known types, matching the existing get_allowed_operations<T>() pattern
in the same file. Drop consteval since all call sites use the function
in runtime contexts. Move schema_registry:types and kafka/protocol
from security's deps to implementation_deps.

Rebuild fan-out on a types.h content-hash bump of
//src/v/pandaproxy/schema_registry/...:
  before: 347 CppCompile actions, 382.6s wall
  after:   71 CppCompile actions,  88.9s wall
  delta:  -276 actions (-79.5%), -294s (-76.8%)
@pgellert
Copy link
Copy Markdown
Contributor Author

force-push:

  • first: fix clang tidy failure
  • second: rebase to dev to fix merge conflicts

@vbotbuildovich
Copy link
Copy Markdown
Collaborator

CI test results

test results on build#83376
test_status test_class test_method test_arguments test_kind job_url passed reason test_history
FLAKY(PASS) ShadowLinkingReplicationTests test_with_restart null integration https://buildkite.com/redpanda/redpanda/builds/83376#019daa49-fc06-4790-9bed-fcd971ab4a73 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=ShadowLinkingReplicationTests&test_method=test_with_restart
FLAKY(PASS) EndToEndHydrationTimeoutTest test_hydration_completes_when_consumer_killed null integration https://buildkite.com/redpanda/redpanda/builds/83376#019daa49-fc05-4d55-b703-1070599c07e6 10/11 Test PASSES after retries.No significant increase in flaky rate(baseline=0.0139, 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=EndToEndHydrationTimeoutTest&test_method=test_hydration_completes_when_consumer_killed
FLAKY(PASS) WriteCachingFailureInjectionE2ETest test_crash_all {"use_transactions": false} integration https://buildkite.com/redpanda/redpanda/builds/83376#019daa4e-4803-4cca-906a-317ca64bbe43 18/21 Test PASSES after retries.No significant increase in flaky rate(baseline=0.0697, p0=0.4110, reject_threshold=0.0100. adj_baseline=0.1949, p1=0.2223, trust_threshold=0.5000) https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=WriteCachingFailureInjectionE2ETest&test_method=test_crash_all

@pgellert
Copy link
Copy Markdown
Contributor Author

/microbench

@vbotbuildovich
Copy link
Copy Markdown
Collaborator

Performance change detected in https://buildkite.com/redpanda/redpanda/builds/83385#019daabc-2c66-4874-ad48-2d3c552cfbde:

Performance changes detected in 23 tests
api_rpbench.sr_bench_fixture.post_x20_20: allocs -> -0.08pct
kafka_fetch_plan_rpbench.fetch_plan.t1p1_no_auth: inst -> +0.15pct
kafka_fetch_plan_rpbench.fetch_plan.t1p1_yes_auth: inst -> +0.16pct
role_store_bench_rpbench.role_store_bench.get_member_roles: inst -> -0.07pct
role_store_bench_rpbench.role_store_bench.get_member_roles_bare_query: inst -> -0.07pct
role_store_bench_rpbench.role_store_bench.role_authz_128_roles_1Ki_members: inst -> +0.03pct
role_store_bench_rpbench.role_store_bench.role_authz_128_roles_1Ki_members_mixed: inst -> +0.06pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members: inst -> +0.05pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members_16_extra_bindings_mixed: inst -> +0.08pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members_4_extra_bindings: inst -> +0.06pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members_4_extra_bindings_mixed: inst -> +0.09pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members_8_extra_bindings_mixed: inst -> +0.10pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_1Ki_members_mixed: inst -> +0.08pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_512_members: inst -> +0.05pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_512_members_deny: inst -> +0.07pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_512_members_deny_mixed: inst -> +0.15pct
role_store_bench_rpbench.role_store_bench.role_authz_64_roles_512_members_mixed: inst -> +0.10pct
role_store_bench_rpbench.role_store_bench.role_authz_8_roles_1Ki_members: inst -> +0.21pct
role_store_bench_rpbench.role_store_bench.role_authz_8_roles_1Ki_members_mixed: inst -> +0.33pct
role_store_bench_rpbench.role_store_bench.role_authz_empty_store: inst -> +0.52pct
role_store_bench_rpbench.role_store_bench.user_range_query: inst -> -0.06pct
role_store_bench_rpbench.role_store_bench.user_range_query_bare_query: inst -> -0.06pct
leader_balancer_bench_rpbench.lb.full_simulation_random: inst -> +2.38pct

See https://redpandadata.atlassian.net/wiki/x/LQAqLg for docs

@pgellert
Copy link
Copy Markdown
Contributor Author

The role_store_bench changes are all minor and reasonable for this change. I think the incremental rebuild time improvement is well worth the slight performance drop here. The rest of the changes all seem like noise/perhaps random binary layout changes.

Copy link
Copy Markdown
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

excellent

@dotnwat dotnwat merged commit 5d71815 into redpanda-data:dev Apr 20, 2026
21 checks passed
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.

4 participants