-
-
Notifications
You must be signed in to change notification settings - Fork 647
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.
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
enhancement: support for acls-filter-regexp #1281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I really appreciate your work!
(I am new to AKHQ and you can ignore me but I left a couple of comments and questions.)
public List<AccessControl> findAll(String clusterId, Optional<String> search) throws ExecutionException, InterruptedException { | ||
return toGroupedAcl(kafkaWrapper | ||
.describeAcls(clusterId, AclBindingFilter.ANY) | ||
.stream() | ||
.filter(aclBinding -> isSearchMatch(search, aclBinding.entry().principal())) | ||
.filter(aclBinding -> isMatchRegex(getAclFilterRegex(),aclBinding.entry().principal())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space should be here?
.filter(aclBinding -> isMatchRegex(getAclFilterRegex(),aclBinding.entry().principal())) | |
.filter(aclBinding -> isMatchRegex(getAclFilterRegex(), aclBinding.entry().principal())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LittleWat - good catch :) Must have been my fingers.
var searchResult = aclRepository.findByPrincipal(KafkaTestCluster.CLUSTER_ID, AccessControl.encodePrincipal("test:toto"), Optional.empty()); | ||
assertEquals("test:toto", searchResult.getPrincipal()); | ||
assertEquals(2, searchResult.getAcls().size()); | ||
assertEquals(2, searchResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether this test case is necessary or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LittleWat - I added to make sure few more principal patterns other than one starting with user* still works after my changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I just thought both the result(searchResult.getAcls().size()
and searchResult.stream().filter(acl -> acl.getOperation().getPermissionType() == AclPermissionType.ALLOW).count())
) are 2
, so it might be okay to remove the latter one. This is just a thought so that you can ignore it. 🙇
@@ -114,6 +117,8 @@ akhq: | |||
attributes: | |||
topics-filter-regexp: | |||
- "test.*" | |||
acls-filter-regexp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is necessary because this group does not have acls/read
role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LittleWat - Same comment as one below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -131,6 +136,9 @@ akhq: | |||
- topic/insert | |||
- topic/delete | |||
- registry/version/delete | |||
attributes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LittleWat - I added these as part of trying to locally test the changes for non-default groups. No harm in leaving it i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm in leaving it i think
Thank you for your reply! I think so too but I just felt a bit strange that no-filter
group has the filter(acls-filter-regexp
).
@@ -104,6 +104,9 @@ akhq: | |||
- connect/update | |||
- connect/delete | |||
- connect/state/update | |||
attributes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this might be a stupid question. If we remove this, void findAllByUser()
in AccessControlRepositoryTest.java
will fail, right...? ( I mean that if we remove it, assertEquals(5, searchResult.getAcls().size());
would become assertEquals(7, searchResult.getAcls().size());
)
Amazing ! |
Hello, I change akhq version 0.16 to the last 0.24 but i think may be the structure change for the mapping of groups and the permissions filters.
But in the last version 0.24 i see this change [1][2] for declare the default-group:
What's the good use for define my default-group the example 1 or the example 2 for the last version 0.24? Thank you for your help in advance. [1] https://akhq.io/docs/configuration/authentifications/external.html [2] https://github.com/tchiotludo/akhq/blob/0.24.0/application.example.yml |
This PR contains changes to support Regex for filtering security groups based on principal names added to the acl bindings