You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Lenni Kuff <ls...@cloudera.com> on 2015/02/12 19:29:10 UTC
Re: Review Request 30068: SENTRY-613:Solr synchronize collection
privileges with Sentry when metadata has been changed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30068/#review71541
-----------------------------------------------------------
Ship it!
sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
<https://reviews.apache.org/r/30068/#comment118225>
Can you expand on exactly what "sync" means? Maybe rename to "isSyncEnabled"?
sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
<https://reviews.apache.org/r/30068/#comment118232>
Add a comment on which user this is expected to be
sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
<https://reviews.apache.org/r/30068/#comment118231>
Maybe get rid of "isSync" and just return:
providerBackend instanceof SearchProviderBackend;
?
sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java
<https://reviews.apache.org/r/30068/#comment118229>
Are you sure that the only time you make it to this block is when the connection could not be obtained?
sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureRequestHandlerUtil.java
<https://reviews.apache.org/r/30068/#comment118230>
Maybe I am missing something, but I don't see any code to handle the "rename" case.
- Lenni Kuff
On Jan. 30, 2015, 5:40 a.m., shen guoquan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30068/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2015, 5:40 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Solr needs to sync collection privileges with Sentry when the metadata has changed. For example, when the collection has been deleted, the privileges related to the collection were needed to drop
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureRequestHandlerUtil.java 8b46388
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureCollectionsHandler.java 20d9626
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/sentry/SentryIndexAuthorizationSingleton.java d44b228
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrAdminOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/30068/diff/
>
>
> Testing
> -------
>
> All tests have passed through the local jenkins environment
>
>
> Thanks,
>
> shen guoquan
>
>
Re: Review Request 30068: SENTRY-613:Solr synchronize collection
privileges with Sentry when metadata has been changed
Posted by shen guoquan <gu...@intel.com>.
> On 二月 12, 2015, 6:29 p.m., Lenni Kuff wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java, line 75
> > <https://reviews.apache.org/r/30068/diff/3/?file=841296#file841296line75>
> >
> > Add a comment on which user this is expected to be
Thanks Lenni. I will fix it.
> On 二月 12, 2015, 6:29 p.m., Lenni Kuff wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java, line 226
> > <https://reviews.apache.org/r/30068/diff/3/?file=841296#file841296line226>
> >
> > Maybe get rid of "isSync" and just return:
> >
> > providerBackend instanceof SearchProviderBackend;
> > ?
Good advice. Thanks Lenni. I will change it.
> On 二月 12, 2015, 6:29 p.m., Lenni Kuff wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java, line 67
> > <https://reviews.apache.org/r/30068/diff/3/?file=841296#file841296line67>
> >
> > Can you expand on exactly what "sync" means? Maybe rename to "isSyncEnabled"?
The Solr binding may be use the policy file as the backend. If the backend is file store, It doesn't need to sync. When the solr integration the db store, it need to sync with the sentry service
> On 二月 12, 2015, 6:29 p.m., Lenni Kuff wrote:
> > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java, line 250
> > <https://reviews.apache.org/r/30068/diff/3/?file=841296#file841296line250>
> >
> > Are you sure that the only time you make it to this block is when the connection could not be obtained?
Yeah, when the client was successfully created, there is only the SentryUserException throwed. If the other exception throw, there must be that the client was created failed.
> On 二月 12, 2015, 6:29 p.m., Lenni Kuff wrote:
> > sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureRequestHandlerUtil.java, line 61
> > <https://reviews.apache.org/r/30068/diff/3/?file=841297#file841297line61>
> >
> > Maybe I am missing something, but I don't see any code to handle the "rename" case.
Hi Lenni. Thanks for your help reviewing my patch about solr integration with db store.
--> but I don't see any code to handle the "rename" case
Currently there is one operation lead to the metadata been changed when solr is running solrcloud mode. This operation is deleting a collection, and the collection API is /admin/collections?action=DELETE&name={collectionName}. when the collection has been deleted in solr, the privileges related to the collection wsa needed to drop in sentry.
According to the rename operation, I found when the solr running in the solrcloud mode, the collection name will not been changed with this Operation. For exmaple, the solrcloud has one collection called collection1, and the collection has two
cores: collection1_core_1, collection1_core_2. The rename operation just change the name of core in the collection. It will not change the collection name. So it doesn not need to sync with sentry. When the solr running in the non-solrcloud mode, the binding is not currently supported. So the there is no code related the rename operation.
- shen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30068/#review71541
-----------------------------------------------------------
On 一月 30, 2015, 5:40 a.m., shen guoquan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30068/
> -----------------------------------------------------------
>
> (Updated 一月 30, 2015, 5:40 a.m.)
>
>
> Review request for sentry, Xiaomeng Huang, Colin Ma, Dapeng Sun, Gregory Chanan, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Solr needs to sync collection privileges with Sentry when the metadata has changed. For example, when the collection has been deleted, the privileges related to the collection were needed to drop
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java faf862f
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/SecureRequestHandlerUtil.java 8b46388
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/handler/admin/SecureCollectionsHandler.java 20d9626
> sentry-solr/solr-sentry-handlers/src/main/java/org/apache/solr/sentry/SentryIndexAuthorizationSingleton.java d44b228
> sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/TestSolrAdminOperations.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/30068/diff/
>
>
> Testing
> -------
>
> All tests have passed through the local jenkins environment
>
>
> Thanks,
>
> shen guoquan
>
>