You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2014/06/07 11:03:49 UTC
Review Request 22340: SENTRY-259: Implement Hive metastore plugin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/
-----------------------------------------------------------
Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
Bugs: SENTRY-259
https://issues.apache.org/jira/browse/SENTRY-259
Repository: sentry
Description
-------
- Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
- Test framework support for starting a metastore server
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
Diff: https://reviews.apache.org/r/22340/diff/
Testing
-------
Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
Thanks,
Prasad Mujumdar
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45105
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
<https://reviews.apache.org/r/22340/#comment79773>
I had locally fixed the same issue. will removed the extra code.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
<https://reviews.apache.org/r/22340/#comment79775>
Removed
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
<https://reviews.apache.org/r/22340/#comment79774>
Removed
- Prasad Mujumdar
On June 9, 2014, 2:13 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 2:13 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 5d7428a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45092
-----------------------------------------------------------
Thanks for the update Prasad! The patch looks good! But, looks like there are some by products of rebase (comments below)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
<https://reviews.apache.org/r/22340/#comment79758>
start has been made async as part of SENTRY-276, so we probably do not need this?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java
<https://reviews.apache.org/r/22340/#comment79764>
As per above comment, we can remove this change.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java
<https://reviews.apache.org/r/22340/#comment79765>
here as well.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
<https://reviews.apache.org/r/22340/#comment79766>
This section has been moved to setup() as part of SENTRY-261, so we should get rid of this?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
<https://reviews.apache.org/r/22340/#comment79767>
same as above
- Sravya Tirukkovalur
On June 9, 2014, 2:13 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 2:13 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 5d7428a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45077
-----------------------------------------------------------
Love this change
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
<https://reviews.apache.org/r/22340/#comment79753>
Can we give this exception a msg?
- Brock Noland
On June 9, 2014, 2:13 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 2:13 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 5d7428a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45142
-----------------------------------------------------------
Ship it!
Ship It!
- Sravya Tirukkovalur
On June 9, 2014, 7:12 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 9, 2014, 7:12 p.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/
-----------------------------------------------------------
(Updated June 9, 2014, 7:12 p.m.)
Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
Changes
-------
Fixed the merge issues per review feedback.
Bugs: SENTRY-259
https://issues.apache.org/jira/browse/SENTRY-259
Repository: sentry
Description
-------
- Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
- Test framework support for starting a metastore server
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
Diff: https://reviews.apache.org/r/22340/diff/
Testing
-------
Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
Thanks,
Prasad Mujumdar
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/
-----------------------------------------------------------
(Updated June 9, 2014, 2:13 a.m.)
Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
Changes
-------
Updates per review feedback -
- Fixed Minor typos, comments etc.
- Moved the metastore specific test APIs to a new base class. The new e2e test is now extending this metastore specific class
Bugs: SENTRY-259
https://issues.apache.org/jira/browse/SENTRY-259
Repository: sentry
Description
-------
- Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
- Test framework support for starting a metastore server
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java f9928df
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java 66d6eef
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java 5d7428a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 8e8db72
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java a8ce2a2
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java d8f5256
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
Diff: https://reviews.apache.org/r/22340/diff/
Testing
-------
Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
Thanks,
Prasad Mujumdar
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java, line 66
> > <https://reviews.apache.org/r/22340/diff/2/?file=605385#file605385line66>
> >
> > Use same name for the variable as "SENTRY.METASTORE.SERVICE.USERS"?
Updated.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java, line 144
> > <https://reviews.apache.org/r/22340/diff/2/?file=605386#file605386line144>
> >
> > Now that we will have security at HMS level, we should probably restrict multiple Hive Server2's having different values set for this property but talking to same HMS. In other words always handle server name at HMS level?
Make sense. The overall server namespace is bit flaky.
will log a followup ticket to keep the server name in sync.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java, lines 191-262
> > <https://reviews.apache.org/r/22340/diff/2/?file=605386#file605386line191>
> >
> > Would be good to have same code paths for resolving (operation) => (required input/output privileges) as SemanticAnalyzerhook unless there is a reason not to?
> >
> > Here is how we resolve it based on operation scope of the operation in SemanticAnalyzerHook: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java#L357
The entry point of Sentry for Hive is the compiler callback. This is a common method for each query that gets the syntax tree and input/out entities (ie. list of objects referred by the statement). Hence we have a common code that examines the operation and generate the input and output hierarchies for Sentry authorization.
In case of metastore, the entry point of Sentry is the pre-envent callback. This gets a different context objects for each operation which makes us easy to identify the operation type. Hence we have a different handler method for each operation which hand constructs the input/out hierarchies for that specific operation.
Thus we don't need the equivalent of hive binding hook for metastore operations.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java, line 166
> > <https://reviews.apache.org/r/22340/diff/2/?file=605389#file605389line166>
> >
> > Test extending from AbstractTestWithStaticConfiguration are designed to be run with a static context, for example an external HiveServer2, and that is the reason it was designed to not accept any custom parameters, and static reflection usage. It would be best for HiveMetastore tests to extend AbstractTestHiveMetaStore
ah ok. will update the patch. Thanks!
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java, lines 284-317
> > <https://reviews.apache.org/r/22340/diff/2/?file=605390#file605390line284>
> >
> > I would put all of the metastore client related additions into a separate class like AbstractTestWithHiveMetastore
will do.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java, line 40
> > <https://reviews.apache.org/r/22340/diff/2/?file=605392#file605392line40>
> >
> > I do not think I understand this, why would we want to return currently logged in user as the group names for all users?
The metastore uses impersonation mode. When we are connecting as dummy test users, eg admin1, user2_1 etc, the impersonation runs into error since these are not real OS users. This is a mock group mapping to overcome that issue. It maps every test user to it's own group and the logged in user's group.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java, lines 63-65
> > <https://reviews.apache.org/r/22340/diff/2/?file=605392#file605392line63>
> >
> > Not sure why these are needed?
Again to handle the file metastore impersonation.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java, lines 109-113
> > <https://reviews.apache.org/r/22340/diff/2/?file=605393#file605393line109>
> >
> > Not sure why these are needed?
I guess we can remove this since the MiniDFS is handling the permissions now.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java, line 157
> > <https://reviews.apache.org/r/22340/diff/2/?file=605393#file605393line157>
> >
> > Just curious, what is the behavior is this is not set?
The secure metastore always impersonate connecting user. The non-secure metastore is configurable. If this is not set, then the file operations would be executed as the user that start the metastore. Also the metastore won't be able to figure out the identity of the connecting user.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java, line 216
> > <https://reviews.apache.org/r/22340/diff/2/?file=605393#file605393line216>
> >
> > We should add an UnmanagedMetaStore in a follow up jira to be able to test it out against a real deployment.
will do.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java, line 46
> > <https://reviews.apache.org/r/22340/diff/2/?file=605395#file605395line46>
> >
> > Should extend something like AbstractTestWithHiveMetastore.
will do
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java, line 73
> > <https://reviews.apache.org/r/22340/diff/2/?file=605395#file605395line73>
> >
> > "read_db1" -> db_read_role
Done
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java, line 210
> > <https://reviews.apache.org/r/22340/diff/2/?file=605395#file605395line210>
> >
> > Fix the comment, "alter table" -> "add partition" ?
Done
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45037
-----------------------------------------------------------
On June 8, 2014, 2:30 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 2:30 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java, lines 191-262
> > <https://reviews.apache.org/r/22340/diff/2/?file=605386#file605386line191>
> >
> > Would be good to have same code paths for resolving (operation) => (required input/output privileges) as SemanticAnalyzerhook unless there is a reason not to?
> >
> > Here is how we resolve it based on operation scope of the operation in SemanticAnalyzerHook: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java#L357
>
> Prasad Mujumdar wrote:
> The entry point of Sentry for Hive is the compiler callback. This is a common method for each query that gets the syntax tree and input/out entities (ie. list of objects referred by the statement). Hence we have a common code that examines the operation and generate the input and output hierarchies for Sentry authorization.
> In case of metastore, the entry point of Sentry is the pre-envent callback. This gets a different context objects for each operation which makes us easy to identify the operation type. Hence we have a different handler method for each operation which hand constructs the input/out hierarchies for that specific operation.
> Thus we don't need the equivalent of hive binding hook for metastore operations.
>
Thanks for the info Prasad! I agree that contexts are different for these two code paths and operation type is easily available in the pre event listener context. But I am still wondering this implementation means we have two different sources of truth on which operation translates to which required privilege scope. For example: Alter table - add part requires all on DB, which is deducible from here: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java#L126
But also from the above authorizeAddPartition. So there is scope for divergence in future.
- Sravya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45037
-----------------------------------------------------------
On June 8, 2014, 2:30 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 2:30 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On June 8, 2014, 6:57 p.m., Sravya Tirukkovalur wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java, lines 191-262
> > <https://reviews.apache.org/r/22340/diff/2/?file=605386#file605386line191>
> >
> > Would be good to have same code paths for resolving (operation) => (required input/output privileges) as SemanticAnalyzerhook unless there is a reason not to?
> >
> > Here is how we resolve it based on operation scope of the operation in SemanticAnalyzerHook: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java#L357
>
> Prasad Mujumdar wrote:
> The entry point of Sentry for Hive is the compiler callback. This is a common method for each query that gets the syntax tree and input/out entities (ie. list of objects referred by the statement). Hence we have a common code that examines the operation and generate the input and output hierarchies for Sentry authorization.
> In case of metastore, the entry point of Sentry is the pre-envent callback. This gets a different context objects for each operation which makes us easy to identify the operation type. Hence we have a different handler method for each operation which hand constructs the input/out hierarchies for that specific operation.
> Thus we don't need the equivalent of hive binding hook for metastore operations.
>
>
> Sravya Tirukkovalur wrote:
> Thanks for the info Prasad! I agree that contexts are different for these two code paths and operation type is easily available in the pre event listener context. But I am still wondering this implementation means we have two different sources of truth on which operation translates to which required privilege scope. For example: Alter table - add part requires all on DB, which is deducible from here: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java#L126
>
> But also from the above authorizeAddPartition. So there is scope for divergence in future.
The required privileges are not different, they are picked up from the same hiveAuthzStmtPrivMap by calling HiveAuthzPrivilegesMap.getHiveAuthzPrivileges(hiveOp). It's only the requested privileges which are extracted differently. Does that answers the question or I misunderstood the comment.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45037
-----------------------------------------------------------
On June 8, 2014, 2:30 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 2:30 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/#review45037
-----------------------------------------------------------
Thanks for the patch Prasad! Some comments below.
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
<https://reviews.apache.org/r/22340/#comment79702>
Use same name for the variable as "SENTRY.METASTORE.SERVICE.USERS"?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
<https://reviews.apache.org/r/22340/#comment79703>
typo: listeners?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
<https://reviews.apache.org/r/22340/#comment79704>
Now that we will have security at HMS level, we should probably restrict multiple Hive Server2's having different values set for this property but talking to same HMS. In other words always handle server name at HMS level?
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
<https://reviews.apache.org/r/22340/#comment79705>
typo: Listener
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
<https://reviews.apache.org/r/22340/#comment79707>
Would be good to have same code paths for resolving (operation) => (required input/output privileges) as SemanticAnalyzerhook unless there is a reason not to?
Here is how we resolve it based on operation scope of the operation in SemanticAnalyzerHook: https://github.com/apache/incubator-sentry/blob/master/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java#L357
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
<https://reviews.apache.org/r/22340/#comment79708>
Test extending from AbstractTestWithStaticConfiguration are designed to be run with a static context, for example an external HiveServer2, and that is the reason it was designed to not accept any custom parameters, and static reflection usage. It would be best for HiveMetastore tests to extend AbstractTestHiveMetaStore
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java
<https://reviews.apache.org/r/22340/#comment79713>
I would put all of the metastore client related additions into a separate class like AbstractTestWithHiveMetastore
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java
<https://reviews.apache.org/r/22340/#comment79712>
I do not think I understand this, why would we want to return currently logged in user as the group names for all users?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java
<https://reviews.apache.org/r/22340/#comment79720>
Not sure why these are needed?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
<https://reviews.apache.org/r/22340/#comment79711>
Not sure why these are needed?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
<https://reviews.apache.org/r/22340/#comment79715>
Just curious, what is the behavior is this is not set?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
<https://reviews.apache.org/r/22340/#comment79716>
We should add an UnmanagedMetaStore in a follow up jira to be able to test it out against a real deployment.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
<https://reviews.apache.org/r/22340/#comment79717>
Should extend something like AbstractTestWithHiveMetastore.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
<https://reviews.apache.org/r/22340/#comment79718>
"read_db1" -> db_read_role
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
<https://reviews.apache.org/r/22340/#comment79719>
Fix the comment, "alter table" -> "add partition" ?
- Sravya Tirukkovalur
On June 8, 2014, 2:30 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22340/
> -----------------------------------------------------------
>
> (Updated June 8, 2014, 2:30 a.m.)
>
>
> Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-259
> https://issues.apache.org/jira/browse/SENTRY-259
>
>
> Repository: sentry
>
>
> Description
> -------
>
> - Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
> - Test framework support for starting a metastore server
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22340/diff/
>
>
> Testing
> -------
>
> Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 22340: SENTRY-259: Implement Hive metastore plugin
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22340/
-----------------------------------------------------------
(Updated June 8, 2014, 2:30 a.m.)
Review request for sentry, Brock Noland, Jarek Cecho, and Sravya Tirukkovalur.
Changes
-------
Additional test cases to cover partitions and URI privileges. Fixed issues found with testing.
Bugs: SENTRY-259
https://issues.apache.org/jira/browse/SENTRY-259
Repository: sentry
Description
-------
- Basic metastore binding via pre-even listener hooks. Uses the same privilege model as hive. Required authorizable lists are created and the hive binding is invoked with the corresponding hive operation type. The rest of the auth is then handled by the Sentry.
- Test framework support for starting a metastore server
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java 63484a8
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 7b7bf8e
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/PolicyProviderForTest.java 79ca387
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java 8beedd7
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java b6bb09c
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/Context.java 39d411e
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestSentryOnFailureHookLoading.java cae270b
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/fs/MiniDFS.java 184c066
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java 19ff6cf
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/InternalMetastoreServer.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java PRE-CREATION
sentry-tests/sentry-tests-hive/src/test/resources/core-site.xml PRE-CREATION
Diff: https://reviews.apache.org/r/22340/diff/
Testing
-------
Added new test with various DB and Table opeartions. Additional testing effort is tracked by a different jira.
Thanks,
Prasad Mujumdar