CORE-15456 Support Group ACLs in Schema Registry#29476
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables Group principal support for Schema Registry ACLs, allowing enterprise customers with the group_based_authorization feature to use Group-based ACLs. The change includes a bug fix in the C++ ACL parsing logic and comprehensive test coverage.
Changes:
- Fixed
from_string_view<principal_type>to correctly parse "group" principal type - Added C++ unit tests for Group principal parsing, authorization, and DENY precedence
- Added Python integration tests covering Group ACL creation, querying, deletion, validation, mixed User/Group scenarios, and scale testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/v/security/acl.cc | Added missing "group" case to principal type parser |
| src/v/security/tests/authorizer_test.cc | Added comprehensive C++ unit tests for Group principal functionality |
| tests/rptest/tests/schema_registry_test.py | Added Python integration tests for Group ACL operations in Schema Registry |
| src/go/rpk/go.mod | Updated dependency versions for rpadmin and rpsr libraries |
8344852 to
25943fe
Compare
|
Force push:
|
25943fe to
22403ec
Compare
|
Force push:
|
| wildcard_group_acl = [self._create_test_acl(principal="Group:*")] | ||
| response = self.sr_client.post_security_acls(wildcard_group_acl) |
There was a problem hiding this comment.
Variable name wildcard_group_acl should be plural wildcard_group_acls since it contains a list of ACLs, for consistency with other test methods like group_acls, mixed_acls, and acls.
| wildcard_group_acl = [self._create_test_acl(principal="Group:*")] | |
| response = self.sr_client.post_security_acls(wildcard_group_acl) | |
| wildcard_group_acls = [self._create_test_acl(principal="Group:*")] | |
| response = self.sr_client.post_security_acls(wildcard_group_acls) |
| empty_group_acl = [self._create_test_acl(principal="Group:")] | ||
| response = self.sr_client.post_security_acls(empty_group_acl) |
There was a problem hiding this comment.
Variable name empty_group_acl should be plural empty_group_acls since it contains a list of ACLs, for consistency with other test methods.
| empty_group_acl = [self._create_test_acl(principal="Group:")] | |
| response = self.sr_client.post_security_acls(empty_group_acl) | |
| empty_group_acls = [self._create_test_acl(principal="Group:")] | |
| response = self.sr_client.post_security_acls(empty_group_acls) |
CI test resultstest results on build#79910
test results on build#79953 |
pgellert
left a comment
There was a problem hiding this comment.
lgtm, the main question I left is if we should move the scale test away from per-PR CI runs
|
|
||
| @cluster(num_nodes=3) | ||
| @matrix(scale=[1, 100]) | ||
| def test_group_acl_scale(self, scale): |
There was a problem hiding this comment.
How quick is this scale test? I'm wondering if it should go into the schema_registry_scale_test.py, which doesn't run on every CI run, but only in the nightly CDT I think.
There was a problem hiding this comment.
This runs very quickly
- Fix missing group mapping in from_string_view<principal_type> - Add C++ tests for group principal parsing and authorization - Add Python integration tests for Schema Registry group ACL operations This ensures group-based ACLs work correctly for enterprise customers with the group_based_authorization feature enabled. Signed-off-by: Michael Boquard <michael@redpanda.com>
Update rpsr to v0.1.3 which permits Group principal ACL creation for Schema Registry ACLs. Signed-off-by: Michael Boquard <michael@redpanda.com>
22403ec to
859e768
Compare
|
Force push:
|
| assert acl["principal"].startswith("Group:team_"), ( | ||
| f"Unexpected principal: {acl['principal']}" |
There was a problem hiding this comment.
Use self.assert_true() instead of assert for consistency with the rest of the test suite and to ensure proper test framework error reporting.
| assert acl["principal"].startswith("Group:team_"), ( | |
| f"Unexpected principal: {acl['principal']}" | |
| self.assert_true( | |
| acl["principal"].startswith("Group:team_"), | |
| f"Unexpected principal: {acl['principal']}", |
| EXPECT_THROW(acl_principal::from_string("Group:*"), acl_conversion_error); | ||
|
|
||
| // Test empty group name throws | ||
| EXPECT_THROW(acl_principal::from_string("Group:"), std::exception); |
There was a problem hiding this comment.
Consider using a more specific exception type (e.g., acl_conversion_error) instead of the generic std::exception for consistency with the wildcard test on line 3253.
| EXPECT_THROW(acl_principal::from_string("Group:"), std::exception); | |
| EXPECT_THROW(acl_principal::from_string("Group:"), acl_conversion_error); |
|
There's something wrong with the license checks here (https://redpandadata.slack.com/archives/C02PGU02EMR/p1767675745170899). Another PR (#29153) also had this problem, but I guess we can bypass it and it shouldn't block future PRs. |
Enable Group principal support for Schema Registry ACLs with comprehensive testing.
This PR ensures that Group-based ACLs work correctly for enterprise customers with the
group_based_authorizationfeature enabled. It includes a bug fix, extensive C++ unit tests, and Python integration tests for end-to-end validation.Backports Required
Release Notes