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/03/08 01:23:32 UTC
Re: Review Request 43977: SENTRY-1014: Add E2E tests for Kafka plugin.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43977/
-----------------------------------------------------------
(Updated March 8, 2016, 12:23 a.m.)
Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
Changes
-------
Rebased.
Bugs: SENTRY-1014
https://issues.apache.org/jira/browse/SENTRY-1014
Repository: sentry
Description
-------
SENTRY-1014: Add E2E tests for Kafka plugin.
Diffs (updated)
-----
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
Diff: https://reviews.apache.org/r/43977/diff/
Testing
-------
Added E2E tests.
Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
Thanks,
Ashish Singh
Re: Review Request 43977: SENTRY-1014: Add E2E tests for Kafka plugin.
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43977/#review122783
-----------------------------------------------------------
Ship it!
- Hao Hao
On March 9, 2016, 7:25 p.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43977/
> -----------------------------------------------------------
>
> (Updated March 9, 2016, 7:25 p.m.)
>
>
> Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
>
>
> Bugs: SENTRY-1014
> https://issues.apache.org/jira/browse/SENTRY-1014
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1014: Add E2E tests for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
> sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43977/diff/
>
>
> Testing
> -------
>
> Added E2E tests.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 43977: SENTRY-1014: Add E2E tests 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/43977/#review122854
-----------------------------------------------------------
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java (line 52)
<https://reviews.apache.org/r/43977/#comment184945>
Thank Ashish for your contribution, I didn't find AbstractKafkaSentryTestBase, is it missing? https://github.com/apache/incubator-sentry/tree/kafka/sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka
Thank Ashish for your contribution, I have only one comment.
- Dapeng Sun
On 三月 10, 2016, 3:25 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43977/
> -----------------------------------------------------------
>
> (Updated 三月 10, 2016, 3:25 a.m.)
>
>
> Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
>
>
> Bugs: SENTRY-1014
> https://issues.apache.org/jira/browse/SENTRY-1014
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1014: Add E2E tests for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
> sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43977/diff/
>
>
> Testing
> -------
>
> Added E2E tests.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 43977: SENTRY-1014: Add E2E tests 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/43977/
-----------------------------------------------------------
(Updated March 9, 2016, 7:25 p.m.)
Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
Changes
-------
Update error messages.
Bugs: SENTRY-1014
https://issues.apache.org/jira/browse/SENTRY-1014
Repository: sentry
Description
-------
SENTRY-1014: Add E2E tests for Kafka plugin.
Diffs (updated)
-----
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
Diff: https://reviews.apache.org/r/43977/diff/
Testing
-------
Added E2E tests.
Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
Thanks,
Ashish Singh
Re: Review Request 43977: SENTRY-1014: Add E2E tests 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/43977/
-----------------------------------------------------------
(Updated March 9, 2016, 5:32 p.m.)
Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
Changes
-------
Address review comments.
Bugs: SENTRY-1014
https://issues.apache.org/jira/browse/SENTRY-1014
Repository: sentry
Description
-------
SENTRY-1014: Add E2E tests for Kafka plugin.
Diffs (updated)
-----
sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
Diff: https://reviews.apache.org/r/43977/diff/
Testing
-------
Added E2E tests.
Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
Thanks,
Ashish Singh
Re: Review Request 43977: SENTRY-1014: Add E2E tests for Kafka plugin.
Posted by Ashish Singh <as...@cloudera.com>.
> On March 8, 2016, 2 a.m., Hao Hao wrote:
> > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java, line 58
> > <https://reviews.apache.org/r/43977/diff/3/?file=1287043#file1287043line58>
> >
> > Should it be 'admin' instead of 'test' to be more accurate?
This will require changing the generated keystore and truststore files to just rename it. I have used a variable `final String SuperuserName = "test";` too allevaite your concern. Hope it is OK with you.
> On March 8, 2016, 2 a.m., Hao Hao wrote:
> > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java, line 102
> > <https://reviews.apache.org/r/43977/diff/3/?file=1287043#file1287043line102>
> >
> > "create topic" instead of "describe topic"?
Added more info.
> On March 8, 2016, 2 a.m., Hao Hao wrote:
> > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java, line 111
> > <https://reviews.apache.org/r/43977/diff/3/?file=1287043#file1287043line111>
> >
> > A little confused here. Isn't addPermission grant the needed permission there?
Here user is trying to produce/write to a topic. So, the user should have write/produce permissions on the topic. Did I answer your question?
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43977/#review122434
-----------------------------------------------------------
On March 8, 2016, 12:23 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43977/
> -----------------------------------------------------------
>
> (Updated March 8, 2016, 12:23 a.m.)
>
>
> Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
>
>
> Bugs: SENTRY-1014
> https://issues.apache.org/jira/browse/SENTRY-1014
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1014: Add E2E tests for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
> sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43977/diff/
>
>
> Testing
> -------
>
> Added E2E tests.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>
Re: Review Request 43977: SENTRY-1014: Add E2E tests for Kafka plugin.
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43977/#review122434
-----------------------------------------------------------
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java (line 58)
<https://reviews.apache.org/r/43977/#comment184431>
Should it be 'admin' instead of 'test' to be more accurate?
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java (line 102)
<https://reviews.apache.org/r/43977/#comment184446>
"create topic" instead of "describe topic"?
sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java (line 111)
<https://reviews.apache.org/r/43977/#comment184452>
A little confused here. Isn't addPermission grant the needed permission there?
- Hao Hao
On March 8, 2016, 12:23 a.m., Ashish Singh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43977/
> -----------------------------------------------------------
>
> (Updated March 8, 2016, 12:23 a.m.)
>
>
> Review request for sentry, Anne Yu, Dapeng Sun, and Hao Hao.
>
>
> Bugs: SENTRY-1014
> https://issues.apache.org/jira/browse/SENTRY-1014
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1014: Add E2E tests for Kafka plugin.
>
>
> Diffs
> -----
>
> sentry-tests/sentry-tests-kafka/src/main/java/org/apache/sentry/tests/e2e/kafka/KafkaTestServer.java 129191ae6f9fa4cc6794117fddb693a3cd1ab815
> sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/TestAuthorize.java 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.keystore.jks PRE-CREATION
> sentry-tests/sentry-tests-kafka/src/test/resources/user2.truststore.jks PRE-CREATION
>
> Diff: https://reviews.apache.org/r/43977/diff/
>
>
> Testing
> -------
>
> Added E2E tests.
>
> Note that this contains changes from SENTRY-1098, SENTRY-1056, SENTRY-1030 and SENTRY-1057, and will have to be rebased once they get it.
>
>
> Thanks,
>
> Ashish Singh
>
>