[CORE-13170] - Support SR ACLs in DescribeACLs #27295

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.

Sign up for GitHub

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

Jump to bottom

Open

michael-redpanda wants to merge 2 commits into redpanda-data:dev from michael-redpanda:dr/acl/support-sr-acls

+241 −7

Contributor

michael-redpanda commented Aug 19, 2025

This PR adds support for describe SR resource based ACLs via the DescribeACLs API. This adds a new tagged field to the describe_acls_request and describe_acls_response messages that is used to indicate whether or not the request is for describing SR ACLs or Kafka ACLs.

Backports Required

Release Notes

None

michael-redpanda requested a review from Copilot

August 19, 2025 19:09

michael-redpanda self-assigned this

github-actions bot added area/build area/redpanda labels

michael-redpanda requested review from BenPope, oleiman and mmaslankaprv

August 19, 2025 19:09

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull Request Overview

This PR adds support for describing Schema Registry (SR) ACLs through the DescribeACLs Kafka API by introducing a new tagged boolean field describe_registry_acls to differentiate between Kafka and SR ACL requests.

Key changes:

Added a new tagged field to describe_acls_request and describe_acls_response protocol messages
Modified ACL handling logic to route requests to appropriate resource type converters based on the new field
Added comprehensive test coverage for the new functionality

Reviewed Changes

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

Show a summary per file

File	Description
`src/v/kafka/protocol/schemata/describe_acls_request.json`	Added `DescribeRegistryAcls` tagged boolean field to request schema
`src/v/kafka/protocol/schemata/describe_acls_response.json`	Added `DescribeRegistryAcls` tagged boolean field to response schema
`src/v/kafka/server/handlers/details/security.h`	Added `to_registry_resource_type` conversion function and updated filtering logic
`src/v/kafka/server/handlers/describe_acls.cc`	Modified response handling to use appropriate resource type converter
`src/v/kafka/server/tests/describe_acls_test.cc`	Added comprehensive test cases for SR ACL describe functionality
`src/v/kafka/server/tests/BUILD`	Added build configuration for new test file

src/v/kafka/server/tests/describe_acls_test.cc

+                  kafka::describe_acls_request req;
+                  req.data.describe_registry_acls = false;
+                  req.data.resource_type = 1;

Preview

Copilot AI Aug 19, 2025

Magic number 1 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.resource_type = 1;
          
                req.data.resource_type = kafka::resource_type::topic;

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  kafka::describe_acls_request req;
+                  req.data.describe_registry_acls = false;
+                  req.data.resource_type = 1;
+                  req.data.resource_pattern_type = 1;

Preview

Copilot AI Aug 19, 2025

Magic number 1 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.resource_pattern_type = 1;
          
                req.data.resource_pattern_type = kafka::resource_pattern_type::literal;

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  req.data.describe_registry_acls = false;
+                  req.data.resource_type = 1;
+                  req.data.resource_pattern_type = 1;
+                  req.data.operation = 1;

Preview

Copilot AI Aug 19, 2025

Magic number 1 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.operation = 1;
          
                req.data.operation = kafka::acl_operation::read;

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  req.data.resource_type = 1;
+                  req.data.resource_pattern_type = 1;
+                  req.data.operation = 1;
+                  req.data.permission_type = 1;

Preview

Copilot AI Aug 19, 2025

Magic number 1 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.permission_type = 1;
          
                req.data.permission_type = kafka::acl_permission_type::allow;

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  BOOST_REQUIRE(!resp.data.describe_registry_acls);
+                  BOOST_REQUIRE_EQUAL(resp.data.error_code, kafka::error_code::none);
+                  BOOST_REQUIRE_EQUAL(resp.data.resources.size(), 1);
+                  BOOST_REQUIRE_EQUAL(resp.data.resources[0].type, 2);

Preview

Copilot AI Aug 19, 2025

Magic number 2 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                BOOST_REQUIRE_EQUAL(resp.data.resources[0].type, 2);
          
                BOOST_REQUIRE_EQUAL(
          
                  resp.data.resources[0].type,
          
                  static_cast<int8_t>(security::resource_type::topic));

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  req.data.describe_registry_acls = false;
+                  // Request SR_SUBJECT resource type, which is invalid if
+                  // describe_registry_acls is false
+                  req.data.resource_type = 100;

Preview

Copilot AI Aug 19, 2025

Magic number 100 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.resource_type = 100;
          
                req.data.resource_type = static_cast<int8_t>(security::resource_type::sr_subject);

Copilot uses AI. Check for mistakes.

src/v/kafka/server/tests/describe_acls_test.cc

+                  req.data.describe_registry_acls = true;
+                  // Request TOPIC resource type, which is invalid if describe_registry_acls
+                  // is true
+                  req.data.resource_type = 2;

Preview

Copilot AI Aug 19, 2025

Magic number 2 should be replaced with a named constant or enum value to improve code readability and maintainability.

Suggested change

      
                req.data.resource_type = 2;
          
                req.data.resource_type = static_cast<int8_t>(kafka::resource_type::topic);

Copilot uses AI. Check for mistakes.

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 6add5fc to c813ff9 Compare

August 19, 2025 19:13

Contributor Author

michael-redpanda commented Aug 19, 2025

Force push:

Fixed lint issues

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from c813ff9 to 7997f68 Compare

August 19, 2025 19:21

Contributor Author

michael-redpanda commented Aug 19, 2025

Force push:

fixed bazel lint issue

Collaborator

vbotbuildovich commented Aug 19, 2025 •

edited

Loading

CI test results

test results on build#71012

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason
DataMigrationsApiTest	test_migrated_topic_data_integrity	{"include_groups": true, "params": {"cancellation": null, "use_alias": true}, "transfer_leadership": true}	integration	https://buildkite.com/redpanda/redpanda/builds/71012#0198c3f2-b0ee-4fea-af12-c1d6b5f581ec	FLAKY	19/20
PrefixTruncateRecoveryTest	test_prefix_truncate_recovery	{"acks": -1, "start_empty": false}	integration	https://buildkite.com/redpanda/redpanda/builds/71012#0198c3f2-b0f3-49c4-9839-682cd031ef88	FLAKY	20/21	upstream reliability is '99.71098265895954'. current run reliability is '95.23809523809523'. drift is 4.47289 and the allowed drift is set to 50. The test should PASS
WriteCachingFailureInjectionTest	test_unavoidable_data_loss	null	integration	https://buildkite.com/redpanda/redpanda/builds/71012#0198c3ef-9021-405d-9a18-8804794a70b6	FLAKY	19/21	upstream reliability is '99.72527472527473'. current run reliability is '90.47619047619048'. drift is 9.24908 and the allowed drift is set to 50. The test should PASS

test results on build#71110

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason
DataMigrationsApiTest	test_mount_inexistent	null	integration	https://buildkite.com/redpanda/redpanda/builds/71110#0198c9d4-b881-44fb-bbe5-c42c894f9c8a	FLAKY	20/21	upstream reliability is '100.0'. current run reliability is '95.23809523809523'. drift is 4.7619 and the allowed drift is set to 50. The test should PASS
ProducersAdminAPITest	test_producers_state_api_during_load	null	integration	https://buildkite.com/redpanda/redpanda/builds/71110#0198c9d6-60d8-410d-8756-b587186dfb8a	FAIL	0/1

test results on build#71401

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason
RandomNodeOperationsTest	test_node_operations	{"cloud_storage_type": 1, "compaction_mode": "sliding_window", "enable_failures": true, "mixed_versions": true, "with_iceberg": false}	integration	https://buildkite.com/redpanda/redpanda/builds/71401#0198e747-6cd7-4a6e-984f-cda43d10b9fc	FLAKY	19/21	upstream reliability is '100.0'. current run reliability is '90.47619047619048'. drift is 9.52381 and the allowed drift is set to 50. The test should PASS

dotnwat reviewed

View reviewed changes

src/v/kafka/server/handlers/details/security.h Outdated Show resolved Hide resolved

src/v/kafka/server/handlers/details/security.h Outdated Show resolved Hide resolved

src/v/kafka/server/handlers/details/security.h Outdated Show resolved Hide resolved

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 7997f68 to 2224ade Compare

August 20, 2025 20:47

michael-redpanda requested a review from dotnwat

August 20, 2025 20:47

Contributor Author

michael-redpanda commented Aug 20, 2025

Force push:

Defined constants for the wire value of SR_SUBJECT and SR_REGISTRY resources


          k/s/p: Added DescribeRegistryAcls tagged field

f31b881

Field used to indicate if DescribeAclsRequest or DescribeAclsResponse
contains only Schema Registry resources.

Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 2224ade to 817bac5 Compare

August 20, 2025 22:41

Contributor Author

michael-redpanda commented Aug 20, 2025

Force push:

Moved tagged field from base response to each individual resource response to indicate that the resource is a registry resource

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 817bac5 to 47e9d18 Compare

August 20, 2025 22:53

Contributor Author

michael-redpanda commented Aug 20, 2025

Force push:

Fixed lint error

oleiman previously approved these changes

View reviewed changes

Member

oleiman left a comment

lgtm

src/v/kafka/server/handlers/details/security.h Outdated

Comment on lines 271 to 275

		default:
		vassert(false, "Request only for Schema Registry resources");

Member

oleiman Aug 25, 2025

nitpick(pedantic): prefer exhaustive switch cases

BenPope reviewed

View reviewed changes

Member

BenPope left a comment

This looks good to me.

I agree with exhaustive switch and use of constants.

michael-redpanda dismissed oleiman’s stale review via

4baa4bb

August 26, 2025 16:00

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 47e9d18 to 4baa4bb Compare

August 26, 2025 16:00

michael-redpanda requested review from oleiman and BenPope

August 26, 2025 16:00

Contributor Author

michael-redpanda commented Aug 26, 2025

Force push:

Exhaustive switch


          k/s: Add support for describe_registry_acls tagged field

If this new tagged field is true, then return Schema Registry resources
in the DescribeAcls response.  Will also accept SR resource types if the
new tagged field is true.

Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda force-pushed the dr/acl/support-sr-acls branch from 4baa4bb to 3692072 Compare

August 26, 2025 16:02

Contributor Author

michael-redpanda commented Aug 26, 2025

Force push:

Fixed lint

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

Reviewers

Copilot code review Copilot Copilot left review comments

mmaslankaprv Awaiting requested review from mmaslankaprv

dotnwat Awaiting requested review from dotnwat

oleiman Awaiting requested review from oleiman

BenPope Awaiting requested review from BenPope

At least 1 approving review is required to merge this pull request.

Labels

area/build area/redpanda