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/05/15 01:29:50 UTC
Review Request 21468: SENTRY-156: Support local privilege validation APIs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/
-----------------------------------------------------------
Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
Bugs: SENTRY-156
https://issues.apache.org/jira/browse/SENTRY-156
Repository: sentry
Description
-------
The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
Diffs
-----
sentry-provider/pom.xml 9bec058
sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
Diff: https://reviews.apache.org/r/21468/diff/
Testing
-------
Added new unit test cases for the provider.
Thanks,
Prasad Mujumdar
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On May 15, 2014, 2:13 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, line 35
> > <https://reviews.apache.org/r/21468/diff/1/?file=582069#file582069line35>
> >
> > I dont see this conf and resourcePath being used anywhere, might as well get rid of it? In which case we can also get rid of hadoop-common dependency.
>
> Prasad Mujumdar wrote:
> make sense. will update the patch.
Actually the constructor for providerBackend needs to have that the configuration argument. The authorization provider looks for that specific method signature using reflection.
Also if we can remove hadoop-common dependency in cache provider, the other layers still need configuration object. Hence we can't remove it from Hive or Impala altogether.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43110
-----------------------------------------------------------
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On May 15, 2014, 2:13 p.m., Sravya Tirukkovalur wrote:
> > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java, line 35
> > <https://reviews.apache.org/r/21468/diff/1/?file=582069#file582069line35>
> >
> > I dont see this conf and resourcePath being used anywhere, might as well get rid of it? In which case we can also get rid of hadoop-common dependency.
make sense. will update the patch.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43110
-----------------------------------------------------------
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43110
-----------------------------------------------------------
Mostly looks good to me. A minor comment below.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
<https://reviews.apache.org/r/21468/#comment77124>
I dont see this conf and resourcePath being used anywhere, might as well get rid of it? In which case we can also get rid of hadoop-common dependency.
- Sravya Tirukkovalur
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43221
-----------------------------------------------------------
Ship it!
Ship It!
- Sravya Tirukkovalur
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43149
-----------------------------------------------------------
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
<https://reviews.apache.org/r/21468/#comment77184>
make sense. will update the patch.
- Prasad Mujumdar
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request 21468: SENTRY-156: Support local privilege validation APIs
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21468/#review43212
-----------------------------------------------------------
Ship it!
sentry-provider/sentry-provider-cache/pom.xml
<https://reviews.apache.org/r/21468/#comment77294>
why does this need a dependency on shiro?
sentry-provider/sentry-provider-cache/pom.xml
<https://reviews.apache.org/r/21468/#comment77292>
Why does the provider have a dependency on model-db?
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java
<https://reviews.apache.org/r/21468/#comment77331>
It seems the only use case currently for close is to support your test case. Instead of baking this into the interface, how about just making that part of the test cleanup? In any event this needs a comment on what it should do and a close() without an "open" don't make sense.
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
<https://reviews.apache.org/r/21468/#comment77295>
nit: assert initialized()? Or even better, throw an exception here explaining that the binding handle was null so the caller knows what to fix.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java
<https://reviews.apache.org/r/21468/#comment77332>
Comment on public function.
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java
<https://reviews.apache.org/r/21468/#comment77333>
Instead of an "Object" should the bindingHandle be a ProviderBackend?
- Lenni Kuff
On May 14, 2014, 11:29 p.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21468/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:29 p.m.)
>
>
> Review request for sentry, Lenni Kuff and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-156
> https://issues.apache.org/jira/browse/SENTRY-156
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The patch implements a simple cache based policy provider that reads the privilege metadata from a cache interface.
>
>
> Diffs
> -----
>
> sentry-provider/pom.xml 9bec058
> sentry-provider/sentry-provider-cache/pom.xml PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestCacheProvider.java PRE-CREATION
> sentry-provider/sentry-provider-cache/src/test/resources/test-authz-provider-local-group-mapping.ini PRE-CREATION
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackendContext.java f45d23d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreSchemaInfo.java 22f1b08
>
> Diff: https://reviews.apache.org/r/21468/diff/
>
>
> Testing
> -------
>
> Added new unit test cases for the provider.
>
>
> Thanks,
>
> Prasad Mujumdar
>
>