----------------------------------------------------------- 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 > >
