You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Ashish Singh <as...@cloudera.com> on 2016/02/25 03:00:23 UTC
Review Request 43979: SENTRY-1057: Add CRUD support for ACLs,
roles and privileges for Kafka plugin.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43979/
-----------------------------------------------------------
Review request for sentry, Dapeng Sun and Hao Hao.
Bugs: SENTRY-1057
https://issues.apache.org/jira/browse/SENTRY-1057
Repository: sentry
Description
-------
SENTRY-1057: Add CRUD support for ACLs, roles and privileges for Kafka plugin.
Diffs
-----
sentry-binding/sentry-binding-kafka/pom.xml bd24c20edcbffb5caee69aedddb93cb5a6c8c112
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java 9ffb971d8da51b716ece3ff5a5bbeebcc7f74e8d
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java 9e72d7890e0db3710ab0971ccbb71a32d3d36e87
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBindingSingleton.java 92e50e6452a89e676eb559a3657b17ba563bfddc
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java e75ec7eddaa1a48b731d551736859a94c08af593
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizerTest.java eafe0f0ee5482fadee43ddec99bdc5da3f42e30f
sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java bb30b1b78d4dccaf8ea5fe22eeed496b608cfcb1
sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java 20d5e8e518cb91b32fad930e9d39400764f40b55
sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java f1ed0001883e5d4b362389c258f4f72697a1e5a4
sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java 513c2719424f994e8eb773e72073401b465b7d58
sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java bcc119860860d928d092ed3ddc0a5a28e841c9aa
Diff: https://reviews.apache.org/r/43979/diff/
Testing
-------
Tested with E2E tests added as part of SENTRY-1014.
Note that this contains changes from SENTRY-1098, SENTRY-1056 and SENTRY-1030, and will have to be rebased once they get it.
Thanks,
Ashish Singh
Re: Review Request 43979: SENTRY-1057: Add CRUD support for ACLs,
roles and privileges for Kafka plugin.
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43979/
-----------------------------------------------------------
(Updated Feb. 29, 2016, 8:11 p.m.)
Review request for sentry, Dapeng Sun and Hao Hao.
Changes
-------
Add e2e tests for Kafka Acls CRUD operations. Address review comments.
Bugs: SENTRY-1057
https://issues.apache.org/jira/browse/SENTRY-1057
Repository: sentry
Description
-------
SENTRY-1057: Add CRUD support for ACLs, roles and privileges for Kafka plugin.
Diffs (updated)
-----
.gitignore a89bad852812015695f373d18f8d9f72a3acce0e
pom.xml ca2c92a26e3e42fe036c974235db47255d1465de
sentry-binding/sentry-binding-kafka/pom.xml bd24c20edcbffb5caee69aedddb93cb5a6c8c112
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java 9ffb971d8da51b716ece3ff5a5bbeebcc7f74e8d
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java 9e72d7890e0db3710ab0971ccbb71a32d3d36e87
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBindingSingleton.java 92e50e6452a89e676eb559a3657b17ba563bfddc
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java e75ec7eddaa1a48b731d551736859a94c08af593
sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizerTest.java eafe0f0ee5482fadee43ddec99bdc5da3f42e30f
sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java bb30b1b78d4dccaf8ea5fe22eeed496b608cfcb1
sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/KafkaActionFactory.java 7b8b5187e3d95a49304512e238e778da885fd27d
sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java 20d5e8e518cb91b32fad930e9d39400764f40b55
sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java f1ed0001883e5d4b362389c258f4f72697a1e5a4
sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java 513c2719424f994e8eb773e72073401b465b7d58
sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java bcc119860860d928d092ed3ddc0a5a28e841c9aa
sentry-provider/sentry-provider-db/pom.xml 7514a7cdfcc7934f2dd0386996fdaf88c0ccbb14
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/PrivilegeOperatePersistence.java c3b0be8694c746cb09797425f98578b8faef8b4a
sentry-tests/pom.xml 3294335e95fb7dbb2da4151041269392004426fb
sentry-tests/sentry-tests-kafka/pom.xml PRE-CREATION
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/CustomPrincipalBuilder.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/EmbeddedZkServer.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/TestUtils.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/StaticUserGroupRole.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAclsCrud.java PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/log4j.properties PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/test.crt PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/test.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/test.truststore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user1.crt PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user1.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user1.truststore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.crt PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
Diff: https://reviews.apache.org/r/43979/diff/
Testing
-------
Tested with E2E tests added as part of SENTRY-1014.
Note that this contains changes from SENTRY-1098, SENTRY-1056 and SENTRY-1030, and will have to be rebased once they get it.
Thanks,
Ashish Singh
Re: Review Request 43979: SENTRY-1057: Add CRUD support for ACLs,
roles and privileges for Kafka plugin.
Posted by Ashish Singh <as...@cloudera.com>.
> On Feb. 29, 2016, 8:31 a.m., Dapeng Sun wrote:
> > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java, line 175
> > <https://reviews.apache.org/r/43979/diff/1/?file=1270547#file1270547line175>
> >
> > Is there any authorization at Kafka ACL commands? If not, it seems any user can modify the ACLs via Kafka commands.
Dapeng, thanks for the review. Kafka will authenticate users before passing user's request to Sentry to perform ACLs CRUD. Sentry can assume that users requests coming to it for performing ACLs CRUD are authenticated and authorized.
> On Feb. 29, 2016, 8:31 a.m., Dapeng Sun wrote:
> > sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java, line 67
> > <https://reviews.apache.org/r/43979/diff/1/?file=1270546#file1270546line67>
> >
> > Better to add some Unit Tests for ACL operations.
I found it a bit complicated to be able to add Acls CRUD unit tests. I have added e2e tests for the same.
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43979/#review121205
-----------------------------------------------------------
On Feb. 25, 2016, 2 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43979/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 2 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Hao Hao.
>
>
> Bugs: SENTRY-1057
> https://issues.apache.org/jira/browse/SENTRY-1057
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1057: Add CRUD support for ACLs, roles and privileges for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-kafka/pom.xml bd24c20edcbffb5caee69aedddb93cb5a6c8c112
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java 9ffb971d8da51b716ece3ff5a5bbeebcc7f74e8d
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java 9e72d7890e0db3710ab0971ccbb71a32d3d36e87
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBindingSingleton.java 92e50e6452a89e676eb559a3657b17ba563bfddc
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java e75ec7eddaa1a48b731d551736859a94c08af593
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizerTest.java eafe0f0ee5482fadee43ddec99bdc5da3f42e30f
> sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java bb30b1b78d4dccaf8ea5fe22eeed496b608cfcb1
> sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java 20d5e8e518cb91b32fad930e9d39400764f40b55
> sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java f1ed0001883e5d4b362389c258f4f72697a1e5a4
> sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java 513c2719424f994e8eb773e72073401b465b7d58
> sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java bcc119860860d928d092ed3ddc0a5a28e841c9aa
>
> Diff: https://reviews.apache.org/r/43979/diff/
>
>
> Testing
> -------
>
> Tested with E2E tests added as part of SENTRY-1014.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056 and SENTRY-1030, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 43979: SENTRY-1057: Add CRUD support for ACLs,
roles and privileges for Kafka plugin.
Posted by Dapeng Sun <da...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43979/#review121205
-----------------------------------------------------------
Thank Ashish, I left some comments.
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java (line 41)
<https://reviews.apache.org/r/43979/#comment182872>
better to use INSTANCE_NAME?
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java (line 67)
<https://reviews.apache.org/r/43979/#comment182875>
Better to add some Unit Tests for ACL operations.
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java (line 173)
<https://reviews.apache.org/r/43979/#comment182874>
Is there any authorization at Kafka ACL commands? If not, it seems any user can modify the ACLs via Kafka commands.
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java (line 198)
<https://reviews.apache.org/r/43979/#comment182876>
What's the meaning of the return value?
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java (line 277)
<https://reviews.apache.org/r/43979/#comment182877>
Some question: what's the meaning of the return value? Can we always return "true"?
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java (line 345)
<https://reviews.apache.org/r/43979/#comment182879>
better to remove the code if it is unused.
sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java (line 402)
<https://reviews.apache.org/r/43979/#comment182880>
Can we split this method and add some utils?
- Dapeng Sun
On 二月 25, 2016, 10 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43979/
> -----------------------------------------------------------
>
> (Updated 二月 25, 2016, 10 a.m.)
>
>
> Review request for sentry, Dapeng Sun and Hao Hao.
>
>
> Bugs: SENTRY-1057
> https://issues.apache.org/jira/browse/SENTRY-1057
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1057: Add CRUD support for ACLs, roles and privileges for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-kafka/pom.xml bd24c20edcbffb5caee69aedddb93cb5a6c8c112
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizer.java 9ffb971d8da51b716ece3ff5a5bbeebcc7f74e8d
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBinding.java 9e72d7890e0db3710ab0971ccbb71a32d3d36e87
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/binding/KafkaAuthBindingSingleton.java 92e50e6452a89e676eb559a3657b17ba563bfddc
> sentry-binding/sentry-binding-kafka/src/main/java/org/apache/sentry/kafka/conf/KafkaAuthConf.java e75ec7eddaa1a48b731d551736859a94c08af593
> sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/kafka/authorizer/SentryKafkaAuthorizerTest.java eafe0f0ee5482fadee43ddec99bdc5da3f42e30f
> sentry-core/sentry-core-model-kafka/src/main/java/org/apache/sentry/core/model/kafka/Cluster.java bb30b1b78d4dccaf8ea5fe22eeed496b608cfcb1
> sentry-core/sentry-core-model-kafka/src/test/java/org/apache/sentry/core/model/kafka/TestKafkaAuthorizable.java 20d5e8e518cb91b32fad930e9d39400764f40b55
> sentry-policy/sentry-policy-kafka/src/main/java/org/apache/sentry/policy/kafka/KafkaModelAuthorizables.java f1ed0001883e5d4b362389c258f4f72697a1e5a4
> sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/TestKafkaModelAuthorizables.java 513c2719424f994e8eb773e72073401b465b7d58
> sentry-policy/sentry-policy-kafka/src/test/java/org/apache/sentry/policy/kafka/provider/TestKafkaAuthorizationProviderGeneralCases.java bcc119860860d928d092ed3ddc0a5a28e841c9aa
>
> Diff: https://reviews.apache.org/r/43979/diff/
>
>
> Testing
> -------
>
> Tested with E2E tests added as part of SENTRY-1014.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056 and SENTRY-1030, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>