> On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, > > line 25 > > <https://reviews.apache.org/r/29238/diff/2/?file=799864#file799864line25> > > > > what's "it"? Where is the value set? Also, I don't think we should > > have solr specific terminology in a search package, it should generally > > apply to search.
-->what's "it" The property of sentry.search.cluster was used to distinct itself from multiple search clusters. For example, there are two search clusters: cluster1 and cluster2 implemented authorization via sentry, and it must set the value of sentry.search.cluster=cluster1 or cluster2 to communicate with sentry service for authorization -->I don't think we should have solr specific terminology in a search package, it should generally apply to search Thanks for your comment. I totally agree with you. I will fix it. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java, > > line 29 > > <https://reviews.apache.org/r/29238/diff/2/?file=799864#file799864line29> > > > > I saw your comment in the previous review. I don't think the word > > "server" makes much sense in this context. I think this is borrowing from > > hive terminology, where a server is actually distinct, but in solr, > > multiple "servers" may want to talk to the same sentry service and use the > > same policy, because they are logically part of the same (SolrCloud) > > service. Maybe rename to service or cluster? Since I don't think solr > > should be in the name either, maybe "sentry.search.service"? Or > > "sentry.search.cluster"? I think the "sentry.search.cluster" is more suitable. Thanks for your advice. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, > > line 99 > > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line99> > > > > Why is this SOLR specific in the search package? The solr integration depends on the sentry new feature "generic model"(SENTRY-398). The privilege stored in the generic model database has a component name column, so which component wanting to integrate into sentry must have a component identity. For example, when the solr want to integrate into sentry, it must transfer solr name to sentry service. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, > > line 120 > > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line120> > > > > In the previous review, you explained the parameter "requestor" in the > > ProviderBackend as being filled in on the service side by the > > SolrHadoopAuthenticationFilter. That makes sense. > > > > But this is on the client side, right? Why is the requestor present on > > the client side? -->Why is the requestor present on the client side The solr service uses the SolrHadoopAuthenticationFilter to authenticate and gets the username. According to sentry service, the solr service is the client side. So when solr service communicates with sentry service, it must set the requestor. but I agree with your concern about why the requestor has been transferred with a parameter, why the sentry service can't get the requestor using the SASL framework when the connection has set up. Currently the design part of sentry about getting the authenticate user is improper. I have already investigated the authentication of thrift service, I will fire a jira to fix the problem. Thanks for your precious opinions > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, > > line 236 > > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line236> > > > > Why isn't this called listRolesByGroupName? Thanks for your comments. I will fix it. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java, > > line 188 > > <https://reviews.apache.org/r/29238/diff/2/?file=799869#file799869line188> > > > > Spelling. I am sorry about this simple mistake. Thanks for pointing out. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, > > line 49 > > <https://reviews.apache.org/r/29238/diff/2/?file=799870#file799870line49> > > > > Maybe call it SearchProviderBackend here to match the new name? I am sorry about this simple mistake. Thanks for pointing out. > On 一月 9, 2015, 1:33 a.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java, > > line 72 > > <https://reviews.apache.org/r/29238/diff/2/?file=799870#file799870line72> > > > > here too. I am sorry about this simple mistake. Thanks for pointing out. - shen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29238/#review67357 ----------------------------------------------------------- On 十二月 24, 2014, 3:30 a.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29238/ > ----------------------------------------------------------- > > (Updated 十二月 24, 2014, 3:30 a.m.) > > > Review request for sentry, Gregory Chanan and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > ------- > > Currently Solr does support index-level security via sentry with file as > backend privilege store. Using file as the backend store is simple, but there > has some disadvantages as followings: > 1. Can't satisfied with the needs of dynamically add, delete and update > permissions > 2. Can't be centrally managed and difficult to maintain > According to the above disadvantages, The Solr Sentry plug-in integration > with DB store is demanded. The Hive Sentry plug-in has already integration > with DB store, but the Hive authorization model is different from the Solr > authorization model.So this new feature depends on the generic authorization > model(SENTRY-398). > > > Diffs > ----- > > sentry-binding/sentry-binding-solr/pom.xml 2dfc933 > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > faf862f > > sentry-core/sentry-core-model-search/src/main/java/org/apache/sentry/core/model/search/SearchConstants.java > 7a39b79 > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java > f428aea > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java > 4b98447 > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > ddb9cf9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > ea8eb79 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchPolicyServiceClient.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SearchProviderBackend.java > PRE-CREATION > > sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > fa5ab69 > > Diff: https://reviews.apache.org/r/29238/diff/ > > > Testing > ------- > > > Thanks, > > shen guoquan > >
