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