> On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/pom.xml, line 56 > > <https://reviews.apache.org/r/29238/diff/1/?file=797085#file797085line56> > > > > Why can this no longer just be in test scope?
Yeah, I will fix it > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, > > line 17 > > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line17> > > > > This doesn't seem like the correct location; all the other > > ProviderBackends are under /sentry-provider I agree with your opinion. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, > > line 76 > > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line76> > > > > We don't use the active role set here at all? I am sorry about making a mistake here. I will fix it. Thanks your feedback. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SearchDBProviderBackend.java, > > line 40 > > <https://reviews.apache.org/r/29238/diff/1/?file=797086#file797086line40> > > > > final? Thanks for your comments, I will fix it. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, > > line 65 > > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line65> > > > > do we care that this client is completely synchronous? Performance > > problem? Yeah, this client is completely synchronous, if the keyword synchronized removed, the thift MetaException(out of sequence response) was throwed. For example, when one request has been send to service, the client was waiting for response froms service, because the client is not synchronous, another thread using the client send a request when the previous request hasn't finished, then the out of sequence exception will occurred. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java, > > line 37 > > <https://reviews.apache.org/r/29238/diff/1/?file=797089#file797089line37> > > > > isn't this the service name, not the server name? The property of sentry.solr.server was used to distinct itself from multiple solr clusters. For example, there are two solr cluster server1 and server2 authorized through sentry, it must set the value of sentry.solr.server=server1 or server2 to communicate with sentry service for authorization > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, > > line 230 > > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line230> > > > > I still don't get why this is called requestorUserName. I want to get > > the roles for a certain user, whether that is the requestor or not seems > > orthogonal and something that should be handled by the authenticaiton layer. May be there is a mistake I have. Changing the requestorUserName to user is better and can understand easier. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java, > > line 55 > > <https://reviews.apache.org/r/29238/diff/1/?file=797091#file797091line55> > > > > Can we add some documentation here? What is the requestor param for? Hi chanan. I guess you have a confussion about this requestor param. I want to explain it. At first, the sentry service exposes a thrift API list_sentry_roles_by_group(string requestor, string groupName, ActiveRoleSet roleSet). The logic of this function as followings: if (groupName == null){ //list all role in the sentry, The requestor must be belongs to adminGroup Set<string> requestorGroups = getGroups(requestor); if (requestorGroups has intersection with adminGroups) { roles = store.getAllRoles(); return roles }else { throw denied exception } } else if (groupName == "ALL") { //list roles belongs to requestor Set<string> requestorGroups = getGroups(requestor); roles = store.getRoles(requestorGroups) return roles } else { //list roles according to the groupName, the requestor must be belongs to adminGroup or belong to the request groupName Set<string> requestorGroups = getGroups(requestor); if ((requestorGroups has intersection with adminGroups) or (requestorGroups.contain(groupName))){ roles = store.getRoles(groupName); return roles; } else { throw denied exception } } That's the procedure of list_roles_by_groups in the sentry service side. The Hive authorization with Sentry also using this thrift API to support commands like "show roles", the requestor will be gotten from UGI. When the solr integration with DB store, it must be communicate with snetry service. when solr want to get roles by group, it must transfer the requestor to sentry serivce side. the requestor can be gotten from SolrHadoopAuthenticationFilter. So the requestor parameter must be add. > On 十二月 22, 2014, 10:19 p.m., Gregory Chanan wrote: > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java, > > line 71 > > <https://reviews.apache.org/r/29238/diff/1/?file=797088#file797088line71> > > > > this isn't accureate, we don't renew the client if it's already been > > renewed. Currently Solr integrates authorization with sentry using the singleton class SentryIndexAuthorizationSingleton. The SentryIndexAuthorizationSingleton will be intialized when it was loaded, so the SolrAuthzBinding in the SentryIndexAuthorizationSingleton also has been intialized when the SentryIndexAuthorizationSingleton was loaded. When the solr integrated with db store, the SearchProviderBackend was created in the SolrAuthzBinding and using the SolrSentryRetryClient to communicate with sentry service. Because the SolrAuthzBinding has no chance to intialized again, the SolrSentryRetryClient in the SearchProviderBackend will not reconnect to the sentry serivce. There is a problem. when the Sentry service restart, the client will throw a thrift connection exception when sending requests to Sentry service because it has already restarted, the client must has a ability to reconnect to the service. - shen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29238/#review65823 ----------------------------------------------------------- On 十二月 19, 2014, 5:54 a.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29238/ > ----------------------------------------------------------- > > (Updated 十二月 19, 2014, 5:54 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/SearchDBProviderBackend.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrAuthzBinding.java > faf862f > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/authz/SolrSentryRetryClient.java > PRE-CREATION > > sentry-binding/sentry-binding-solr/src/main/java/org/apache/sentry/binding/solr/conf/SolrAuthzConf.java > 227f75e > > 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-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java > fa5ab69 > > Diff: https://reviews.apache.org/r/29238/diff/ > > > Testing > ------- > > > Thanks, > > shen guoquan > >
