hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejas Nair" <the...@hortonworks.com>
Subject Re: Review Request 26984: HIVE-8534 : sql std auth : update configuration whitelist for 0.14
Date Wed, 22 Oct 2014 05:02:28 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message