You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Andras Katona via Review Board <no...@reviews.apache.org> on 2022/02/11 08:38:26 UTC
Re: Review Request 73344: RANGER-3231 Ranger should use kafka Authorizer from KIP-504
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73344/#review224038
-----------------------------------------------------------
plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java
Lines 151 (patched)
<https://reviews.apache.org/r/73344/#comment313021>
this whole authorization for only one action could be extracted to a private method which would return a single result.
imho just would be nicer
```
private AuthorizationResult authorize(AuthorizableRequestContext requestContext, Action action) {
```
- Andras Katona
On June 7, 2021, 8:08 a.m., Chia-Ping Tsai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73344/
> -----------------------------------------------------------
>
> (Updated June 7, 2021, 8:08 a.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-3231
> https://issues.apache.org/jira/browse/RANGER-3231
>
>
> Repository: ranger
>
>
> Description
> -------
>
> As described in the KIP, `org.apache.kafka.server.authorizer.Authorizer` is an improvement over `kafka.security.auth.Authorizer` and it's a pure Java interface (instead of Scala).
> `kafka.security.auth.Authorizer` has been deprecated since December 2019 and it will be removed in Apache Kafka 3.0 (roughly planned for July/August).
> See the KIP for more details:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-504+-+Add+new+Java+Authorizer+Interface
>
>
> Diffs
> -----
>
> plugin-kafka/pom.xml 010707d99
> plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 2a1b812e0
> ranger-kafka-plugin-shim/pom.xml fd1dc3cde
> ranger-kafka-plugin-shim/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 9d72ae0c8
>
>
> Diff: https://reviews.apache.org/r/73344/diff/3/
>
>
> Testing
> -------
>
> run `mvn clean test` and all pass on my local.
>
>
> File Attachments
> ----------------
>
> RANGER-3231.v1.patch
> https://reviews.apache.org/media/uploaded/files/2021/05/18/4e2f190f-c871-4115-b554-0e6041a5a5a6__RANGER-3231.v1.patch
>
>
> Thanks,
>
> Chia-Ping Tsai
>
>
Re: Review Request 73344: RANGER-3231 Ranger should use kafka Authorizer from KIP-504
Posted by Andras Katona via Review Board <no...@reviews.apache.org>.
> On Feb. 11, 2022, 8:38 a.m., Andras Katona wrote:
> > plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java
> > Lines 151 (patched)
> > <https://reviews.apache.org/r/73344/diff/3/?file=2250664#file2250664line168>
> >
> > this whole authorization for only one action could be extracted to a private method which would return a single result.
> > imho just would be nicer
> > ```
> > private AuthorizationResult authorize(AuthorizableRequestContext requestContext, Action action) {
> > ```
Sorry, placed the comment at the wrong place, basically the authorize method would just look like this:
```
@Override
public List<AuthorizationResult> authorize(AuthorizableRequestContext requestContext, List<Action> actions) {
return actions.stream()
.map(action -> authorize(requestContext, action))
.collect(Collectors.toList());
}
```
So not just the final block could be extracted but the whole current authorize method content could be made to handle one action only and the mentioned above would make the collection of the results.
- Andras
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73344/#review224038
-----------------------------------------------------------
On June 7, 2021, 8:08 a.m., Chia-Ping Tsai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73344/
> -----------------------------------------------------------
>
> (Updated June 7, 2021, 8:08 a.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-3231
> https://issues.apache.org/jira/browse/RANGER-3231
>
>
> Repository: ranger
>
>
> Description
> -------
>
> As described in the KIP, `org.apache.kafka.server.authorizer.Authorizer` is an improvement over `kafka.security.auth.Authorizer` and it's a pure Java interface (instead of Scala).
> `kafka.security.auth.Authorizer` has been deprecated since December 2019 and it will be removed in Apache Kafka 3.0 (roughly planned for July/August).
> See the KIP for more details:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-504+-+Add+new+Java+Authorizer+Interface
>
>
> Diffs
> -----
>
> plugin-kafka/pom.xml 010707d99
> plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 2a1b812e0
> ranger-kafka-plugin-shim/pom.xml fd1dc3cde
> ranger-kafka-plugin-shim/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 9d72ae0c8
>
>
> Diff: https://reviews.apache.org/r/73344/diff/3/
>
>
> Testing
> -------
>
> run `mvn clean test` and all pass on my local.
>
>
> File Attachments
> ----------------
>
> RANGER-3231.v1.patch
> https://reviews.apache.org/media/uploaded/files/2021/05/18/4e2f190f-c871-4115-b554-0e6041a5a5a6__RANGER-3231.v1.patch
>
>
> Thanks,
>
> Chia-Ping Tsai
>
>
Re: Review Request 73344: RANGER-3231 Ranger should use kafka Authorizer from KIP-504
Posted by Andras Katona via Review Board <no...@reviews.apache.org>.
> On Feb. 11, 2022, 8:38 a.m., Andras Katona wrote:
> > plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java
> > Lines 151 (patched)
> > <https://reviews.apache.org/r/73344/diff/3/?file=2250664#file2250664line168>
> >
> > this whole authorization for only one action could be extracted to a private method which would return a single result.
> > imho just would be nicer
> > ```
> > private AuthorizationResult authorize(AuthorizableRequestContext requestContext, Action action) {
> > ```
>
> Andras Katona wrote:
> Sorry, placed the comment at the wrong place, basically the authorize method would just look like this:
> ```
> @Override
> public List<AuthorizationResult> authorize(AuthorizableRequestContext requestContext, List<Action> actions) {
> return actions.stream()
> .map(action -> authorize(requestContext, action))
> .collect(Collectors.toList());
> }
> ```
> So not just the final block could be extracted but the whole current authorize method content could be made to handle one action only and the mentioned above would make the collection of the results.
I just realized that the (common) rangerPlugin has an isAccessAllowed method which accepts lists of requests, it would be nice to call that and refactor the code
- Andras
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73344/#review224038
-----------------------------------------------------------
On June 7, 2021, 8:08 a.m., Chia-Ping Tsai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73344/
> -----------------------------------------------------------
>
> (Updated June 7, 2021, 8:08 a.m.)
>
>
> Review request for ranger.
>
>
> Bugs: RANGER-3231
> https://issues.apache.org/jira/browse/RANGER-3231
>
>
> Repository: ranger
>
>
> Description
> -------
>
> As described in the KIP, `org.apache.kafka.server.authorizer.Authorizer` is an improvement over `kafka.security.auth.Authorizer` and it's a pure Java interface (instead of Scala).
> `kafka.security.auth.Authorizer` has been deprecated since December 2019 and it will be removed in Apache Kafka 3.0 (roughly planned for July/August).
> See the KIP for more details:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-504+-+Add+new+Java+Authorizer+Interface
>
>
> Diffs
> -----
>
> plugin-kafka/pom.xml 010707d99
> plugin-kafka/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 2a1b812e0
> ranger-kafka-plugin-shim/pom.xml fd1dc3cde
> ranger-kafka-plugin-shim/src/main/java/org/apache/ranger/authorization/kafka/authorizer/RangerKafkaAuthorizer.java 9d72ae0c8
>
>
> Diff: https://reviews.apache.org/r/73344/diff/3/
>
>
> Testing
> -------
>
> run `mvn clean test` and all pass on my local.
>
>
> File Attachments
> ----------------
>
> RANGER-3231.v1.patch
> https://reviews.apache.org/media/uploaded/files/2021/05/18/4e2f190f-c871-4115-b554-0e6041a5a5a6__RANGER-3231.v1.patch
>
>
> Thanks,
>
> Chia-Ping Tsai
>
>