You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "Claudenw (via GitHub)" <gi...@apache.org> on 2024/08/02 14:38:22 UTC
[PR] KIP-1042: Standard authorizer refactor [kafka]
Claudenw opened a new pull request, #16779:
URL: https://github.com/apache/kafka/pull/16779
Refactor of the Standard Authorizer, associated classes and tests to make it easier to create new AuthorizerData implementations. This is a precursor to the Trie implementation described in KIP-1042.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2278274815
Also can you please create a JIRA for this PR I know it is explained in KIP-1042 but this KIP still not accepted last time I checked and my understanding this is precursor so I believe it need its own jira to track and also to assign it to releases clearly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2278264446
> 2. I copied the code for authorizeByResourceType from the Authorizer interface into the StandardAuthroizerData class and modified StandardAuthorizer to override authorizeByResourceType by calling the implementation in StandardAuthroizerData. Now the checkstyle complains about cyclometric compexity. I don't know why this would be the case and I will have to dig into it.
The Authorizer interface is exempt from these 2 checks you might need to do the same for your class by adding it to suppressions.xml
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2286348905
> @OmniaGM I run the failed tests on my local box and they pass. If I run the whole suite I get different failures at different times. Is this just flaky build or have I broken something?
Running the whole suit usually get different results as we have some flaky tests in Kafka I would recommend run the test with ` -PmaxParallelForks`, `-PmaxTestRetries` and `-PmaxTestRetryFailures=2` this usually eliminate some of the flaky ones as they most likely to pass on the retry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2286341573
@OmniaGM I run the failed tests on my local box and they pass. If I run the whole suite I get different failures at different times. Is this just flaky build or have I broken something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2277497882
@OmniaGM I made some fixes but I have 2 issues that I am not sure how to address:
1. When I run `./gradlew spotlessCheck` locally everything passes. When I try to run `./gradlew :metadata:spotlessCheck` or `./gradlew :metadata:spotlessApply` I get a `Cannot locate tasks that match ':metadata:spotlessApply' as task 'spotlessApply' not found in project ':metadata'` message.
2. I copied the code for `authorizeByResourceType` from the Authorizer interface into the `StandardAuthroizerData` class and modified `StandardAuthorizer` to override `authorizeByResourceType` by calling the implementation in `StandardAuthroizerData`. Now the checkstyle complains about cyclometric compexity. I don't know why this would be the case and I will have to dig into it.
If you have any suggestions for how to address either of these issues I would appreciate hearing them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2317454494
@OmniaGM I think the latest tests are flaky tests and this request could be merged. Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2278221261
Thanks for the fixes @Claudenw,
> 1. When I run `./gradlew spotlessCheck` locally everything passes. When I try to run `./gradlew :metadata:spotlessCheck` or `./gradlew :metadata:spotlessApply` I get a `Cannot locate tasks that match ':metadata:spotlessApply' as task 'spotlessApply' not found in project ':metadata'` message.
Which JDK version do you have locally? spotless doesn't work with JDK 21 so please make sure your JDK is either 11 or 17
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2283891520
@OmniaGM I think I have the issues fixed and the Jira opened.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2286431067
Same with `AuthorizerTest` is failing with your patch but passes without it locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2278270069
You can find the list of exemptions for `CyclomaticComplexity` here
https://github.com/apache/kafka/blob/0a4a12fbc4df0ac4b141a869d3880b15b66a7759/checkstyle/suppressions.xml#L100
and for `NPathComplexity` here
https://github.com/apache/kafka/blob/0a4a12fbc4df0ac4b141a869d3880b15b66a7759/checkstyle/suppressions.xml#L106
Unfornatly it's not documented on the interfaces/classes that they are part of the suppressions.xml file or even which checks are suppressed or why so you end up with this confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2309780705
@OmniaGM can you tell me how I can figure out what tests are failing? The builds all seem to be correct but the CI is failing. How can I determine what the issue is?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2344112120
@cmccabe
You asked about why the StandardAuthorizerData was easier to test than the StandardAuthorizer.
It is not that the StandardAuthorizerData is easier to test, it is that the interface for StandardAuthorizerData is different than the StandardAuthorizer. The StandardAuthorizer does some work and then passed requests on to the StandardAuthorizerData to handle. So the parameters are different for some methods. This give us the opportunity to test the StandardAuthorizerData interface separate from the StandardAuthorizer. In many cases it is probably possible to implement the StandardAuthorizerData than to start at the StandardAuthorizer level. The Trie implementation is simply a replacement for the StandardAuthorizerData.
The tests will now verify edge cases and corner cases for the StandardAuthorizerData that otherwise were not discovered until integration testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2620655271
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2286418959
@Claudenw I ran the `AuthorizerIntegrationTest` from trunk and it pass however it fails with your pr and it fails with same error https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-16779/4/testReport/junit/kafka.api/AuthorizerIntegrationTest/Build___JDK_21_and_Scala_2_13____testIdempotentProducerNoIdempotentWriteAclInInitProducerId_String__quorum_kraft_/
I am guessing it's due to your changes on `metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizer.java` as this is used in the setup of `AbstractAuthorizerIntegrationTest` which is extended by `AuthorizerIntegrationTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KIP-1042: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2275831283
The build because of violation of spotless checks can you fix these please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2310267021
> @OmniaGM can you tell me how I can figure out what tests are failing? The builds all seem to be correct but the CI is failing. How can I determine what the issue is?
Here is all failed ones https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-16779/7/testReport/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #16779: KAFKA-17316: Standard authorizer refactor
URL: https://github.com/apache/kafka/pull/16779
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17316: Standard authorizer refactor [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16779:
URL: https://github.com/apache/kafka/pull/16779#issuecomment-2689641095
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org