You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Arun Suresh <ar...@gmail.com> on 2014/11/04 12:01:52 UTC
Re: Review Request 26900: Synchronization of HDFS permissions with
Sentry permissions
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, line 53
> > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line53>
> >
> > Should'nt this be always same as "sentry.service.security.mode". If so, why do not we reuse the same property?
The ServiceConstants class is use both in the client ans server. I did not want the Sentry HDFS client code to depend on any of the core Sentry packages, so I created a separate ServiceConstants class.
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, line 56
> > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line56>
> >
> > Can we add a comment that this is property for ugi is only for testing purposes, for JAAS based tests if we happen to add them in future? Outside of tests this will never be used isnt it?
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, line 57
> > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line57>
> >
> > Same as above, if this always has to be sentry service's principal, why not reuse the same property? So that users do not have two parameters to set?
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java, line 59
> > <https://reviews.apache.org/r/26900/diff/4/?file=728607#file728607line59>
> >
> > Same as above.
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 99
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line99>
> >
> > Consider extending from AbstractTestWithStaticConfiguration (see example AbstractMetastoreTestWithStaticConfiguration), so that starting up services logic and other utilities can be reused.
I initially tried that but I wanted to do stuff like start and stop the sentry service in various permutations during dev-testing.. also.. I think AbstractTestWithStaticConfiguration starts and stops everything using @BeforeClass and @AfterClass.. I initially wanted to it to stop and start before every testCase
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 102
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line102>
> >
> > Looks like you are not using this class, but instead using MiniDFS.PseudoGroupMappingService ? In which case, can we remove this class here?
Will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 291
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line291>
> >
> > Rename fn name to startDFS, as we are not starting Yarn here?
Starting YARN for some reason results in the test going OOM... thought i would look into it later..
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, lines 308-314
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line308>
> >
> > Consider using service constants rather than harcoding the parameter value, so that it would be easier to navigate the usages as well as refactor if need to.
I actually prefer putting the actual property name in tests as much as possible.. That way I dont have to follow the IDE link to know the actual property name..
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 315
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line315>
> >
> > Nit: You may want to add a comment, that is can be removed when moved to hadoop 2.6, as minidfs defaults to this?
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 322
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line322>
> >
> > Make sure mkdirs succeded before proceeding?
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 483
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line483>
> >
> > Hmm, why does meta store throw an exception the first time?
Ive sinced fixed this in the Hive / Metastore Binding.. by adding a retry.. Earlier if Sentry went down and came back up, neither netastore or HS2 was every able to connect to Sentry again..
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, lines 497-499
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line497>
> >
> > Is this repeated on purpose to test idempotency?
I have since added more tests..
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 529
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line529>
> >
> > It is not clear from the fn name that it also grants select permissions to p1_admin which is visisble outside this function.
will do
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 547
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line547>
> >
> > Why are we not running MR here?
The MiniMR cluster was going OOM when I start it... will look into it later (did not think it was a priority since HDFS read write is working)
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java, line 552
> > <https://reviews.apache.org/r/26900/diff/4/?file=728649#file728649line552>
> >
> > select corresponds to READ_EXECUTE? Not just READ?
So POSIX directories apparantly need an execute flag to list (ls) it.. since all tables/dbs are ultimately mapped to directories.. it doesnt make sense to grant READ and not EXECUTE..
Also.. considering we don't technically 'execute' a file in HDFS, we felt it was safe to give all objects an EXECUTE flag as well..
> On Oct. 29, 2014, 3:02 a.m., Sravya Tirukkovalur wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java, lines 108-143
> > <https://reviews.apache.org/r/26900/diff/4/?file=728652#file728652line108>
> >
> > Why was this test commented out?
Had added/removed it to test some stuff earlier (before making everything a plugin).. will remove..
- Arun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26900/#review58943
-----------------------------------------------------------
On Oct. 22, 2014, 6:20 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26900/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2014, 6:20 a.m.)
>
>
> Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-432 : Synchronization of HDFS permissions with Sentry permissions
>
>
> Diffs
> -----
>
> pom.xml e172e92
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/SentryHiveAuthorizationTaskFactoryImpl.java f38ee91
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java 4d2a625
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java 38bf8b2
> sentry-dist/pom.xml cd7126b
> sentry-dist/src/main/assembly/bin.xml 258e63c
> sentry-dist/src/main/assembly/sentry-hdfs.xml PRE-CREATION
> sentry-hdfs/pom.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/.gitignore PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/pom.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPaths.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPathsDumper.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/AuthzPermissions.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPathsDumper.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/MetastoreClient.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPathsFullDump.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-common/src/test/resources/hdfs-sentry.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-dist/pom.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-dist/src/main/assembly/all-jar.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/.gitignore PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/pom.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationConstants.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/MockSentryAuthorizationProvider.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/resources/hdfs-sentry.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/.gitignore PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/pom.xml PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/ExtendedMetastoreClient.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java PRE-CREATION
> sentry-provider/sentry-provider-db/pom.xml b4167e4
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryMetastoreListenerPlugin.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java b66037a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 017cf08
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java 65905f5
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java b54e12e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 40e8a0e
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyStoreProcessor.java 46f8fb8
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerWithoutKerberos.java e5238a6
> sentry-tests/sentry-tests-hive/pom.xml 10415fc
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java PRE-CREATION
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/StaticUserGroup.java 66f088f
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java 45d24f9
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java 8ce78bc
>
> Diff: https://reviews.apache.org/r/26900/diff/
>
>
> Testing
> -------
>
> Manual testing, Basic e2e integration testing, unit tests
>
>
> Thanks,
>
> Arun Suresh
>
>