> On Oct. 21, 2014, 10:36 p.m., Gunther Hagleitner wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 1410 > > <https://reviews.apache.org/r/26984/diff/2/?file=727645#file727645line1410> > > > > why is this better? seems you can do everything in the first list.
If people end up needing to add some parameter list, I expect that to be just a handful of them (maybe 1 or 2). Without this param, to do that they will need to use the full list (which is large) from authorizer and then edit that. That is hard to manage. It will also cause issues while upgrading hive, they will not be able to re-use the udpated list from new hive version, unless they clearly track what they had edited. > On Oct. 21, 2014, 10:36 p.m., Gunther Hagleitner wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2051 > > <https://reviews.apache.org/r/26984/diff/2/?file=727645#file727645line2051> > > > > is there a config var for the blacklist too? Do you have to add the > > whitelist param to the blacklist by default? Yes, there is a config var for blacklist too. But sql std authorization is relying only on the whitelist. When HS2 starts up, the whitelist is set. Since whitelist does not include the whitelist params, it can't be modified by the user. > On Oct. 21, 2014, 10:36 p.m., Gunther Hagleitner wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2053 > > <https://reviews.apache.org/r/26984/diff/2/?file=727645#file727645line2053> > > > > do we document that the blacklist overwrites the whitelist? you mean override ? config param needs to satisfy both whitelist and blacklist (they override each other!). That is not explicitly documented. I can mention that in sql std whitelist description. > On Oct. 21, 2014, 10:36 p.m., Gunther Hagleitner wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2534 > > <https://reviews.apache.org/r/26984/diff/2/?file=727645#file727645line2534> > > > > given that the pattern can be null, why do you need an extra boolean? > > even simpler: set pattern to ".*" by default and don't allow it to be null. > > (if you want to enable specific checking, write your own regex). The problem with setting whitelist parameter to ".*" by default is that it won't be clear if the value has been set by the user. ie, user wanted to effectively disable whitelist checks with sql standard authorization. Updating the patch to check for unset pattern instead of boolean. > On Oct. 21, 2014, 10:36 p.m., Gunther Hagleitner wrote: > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/SettableConfigUpdater.java, > > line 18 > > <https://reviews.apache.org/r/26984/diff/2/?file=727646#file727646line18> > > > > I'm missing something here. This seems overly complicated. You have two > > copies of the same list of parameters and the actual set method does a > > bunch of conversion (. -> \. etc). Why not just add these as the default to > > the HiveConf class? The conversions were added to try and use the old format of parameter. But on second thoughts, since this was an internal parameter, changing the format would be OK. With updated format, users will be able to specify proper java regex. I have two lists because I am using variable names for config parameters that don't conform to a regex. Using the ConfVar variables intead of the variable name string is less error prone. - Thejas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26984/#review57677 ----------------------------------------------------------- On Oct. 22, 2014, 5 a.m., Thejas Nair wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26984/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2014, 5 a.m.) > > > Review request for hive, Gunther Hagleitner and Jason Dere. > > > Bugs: HIVE-8534 > https://issues.apache.org/jira/browse/HIVE-8534 > > > Repository: hive-git > > > Description > ------- > > https://issues.apache.org/jira/browse/HIVE-8534 > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a9da7d > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/SettableConfigUpdater.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAccessController.java > 658ff76 > > ql/src/test/org/apache/hadoop/hive/ql/security/authorization/plugin/TestSQLStdHiveAccessControllerHS2.java > PRE-CREATION > > ql/src/test/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/TestSQLStdHiveAccessControllerHS2.java > f13cf7e > ql/src/test/queries/clientnegative/authorization_disallow_transform.q > 342c29a > ql/src/test/results/clientnegative/authorization_disallow_transform.q.out > 39819b6 > > Diff: https://reviews.apache.org/r/26984/diff/ > > > Testing > ------- > > Tests updated > > > Thanks, > > Thejas Nair > >
