> On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote: > > Mostly look fine. A couple of questions - > > - Does the sentry config file need to be in classpath ? can it be specified > > via another argument ? > > - I guess it will need some work for secure connection. The DB client needs > > the actions to be wrapped in security context. In case of Hive or other > > service, they already have secure UGI object created. In case of the shell > > we'll need to handle it. > > - We should definately log a followup ticket for generic privileges. > > > > Few minor comments/suggestions below
Thanks for the comments, 1. The sentry config file should be spcified via the argument -conf <filepath> or --sentry_conf <filepath> 2. For the security feature, we can trace this feature by another ticket. 3. For generic model, we also need fire a new ticket. > On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, > > line 238 > > <https://reviews.apache.org/r/34925/diff/1/?file=976351#file976351line238> > > > > Isn't this already verified on the server side. In that case, we don't > > have duplicate the validation logic in shell again. For this privilege hierarchy, there is no validation logic on the server side. In the previous, maybe the privilege was generated from hive, so such validation is not neccessary, in this feature, the privilege is generated from the shell, to avoid the mistype, I think this validation should be kept. > On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote: > > bin/sentryShell, line 1 > > <https://reviews.apache.org/r/34925/diff/1/?file=976349#file976349line1> > > > > It would be good to have an option to invoke this from sentry script > > itself. > > Perhaps you can log a followup ticket to address that in future. Do you mean the shell should be called like the following example: sentry --simpleshell -t hive -gpr -r testrole -p server=server1->db=db1->action=select -conf /path/to/configfile ? - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34925/#review88171 ----------------------------------------------------------- On June 2, 2015, 5:09 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34925/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 5:09 a.m.) > > > Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar. > > > Repository: sentry > > > Description > ------- > > Create a simpler shell for sentry that handles command line arguments, eg > sentryShell --grant_role_privilege --role analyst --privilege > server=server1->database=db2->table=tab1->action=select > > > Diffs > ----- > > bin/sentryShell PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34925/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
