hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HIVE-2616) Passing user identity from metastore client to server in non-secure mode
Date Sat, 17 Dec 2011 02:42:31 GMT

    [ https://issues.apache.org/jira/browse/HIVE-2616?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171421#comment-13171421
] 

jiraposter@reviews.apache.org commented on HIVE-2616:
-----------------------------------------------------



bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 237
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line237>
bq.  >
bq.  >     All properties that appear in HiveConf also need to appear in conf/hive-default.xml.template
along with a description.
bq.  >

Done.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 238
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line238>
bq.  >
bq.  >     
bq.  >     I think it would make sense to change the name to 'hive.metastore.client.enable.setugi'.
Also, I think this feature should be disabled by default.
bq.  >     
bq.  >

Done. False by default.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 239
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61777#file61777line239>
bq.  >
bq.  >     Please add a new property hive.metastore.server.enable.setugi that allows this
RPC to be disabled on the server side, and set the default value to false.

I reused same config hive.metastore.execute.setugi at both client and server which is off
by default.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/if/hive_metastore.thrift, line 438
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61778#file61778line438>
bq.  >
bq.  >     Please add a comment explaining what this call does.

Added comment.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java,
line 145
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61782#file61782line145>
bq.  >
bq.  >     When I apply your changes and run the thriftif ant target I see a small diff
in this file. Did you use Thrift 0.7.0 to generate these files?

I am not sure how that happened. I reran ant thriftif again. So, those should go away now.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line
3589
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3589>
bq.  >
bq.  >     Indentation.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line
3743
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3743>
bq.  >
bq.  >     So instead of checking the hive.metastore.sasl.enabled property we now just
check to see if we're using a security enabled shim, and if so assume that the user wants
to enable security? I don't think this is correct behavior since the fact that we're using
a secure version of Hadoop does not necessarily imply that we actually have security enabled.
bq.  >     
bq.  >     Also, it looks like this change deprecates the hive.metastore.sasl.enabled configuration
property. In line with my comment above I think it makes sense to leave this property in,
but if you do remove it then you need to release note the change and remove this property
from HiveConf and conf/hive-default.xml.template.

Reverted back to use old config variables to avoid the issues outlined. 


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line
3751
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3751>
bq.  >
bq.  >     Indentation.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line
3756
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3756>
bq.  >
bq.  >     We're initializing SASL even if isSecure=false?

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line
3758
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61787#file61787line3758>
bq.  >
bq.  >     Formatting.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java,
line 263
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line263>
bq.  >
bq.  >     Formatting: please add spaces.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java,
line 280
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line280>
bq.  >
bq.  >     Should this be "Failed to login to the MetaStore Server..."?

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java,
line 264
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61788#file61788line264>
bq.  >
bq.  >     "new client talking to old metastore" seems to imply that we're able to determine
whether or not we're talking to an old server, which isn't true. In reality, the onus is on
the admin to ensure that both sides support this feature. What happens if the client calls
set_ugi(), but the server doesn't support it? Is the error message helpful?

If client calls set_ugi() and server doesn't support it then exception will be thrown to client
which client will ignore and continue. So, effectively you will end up with old behavior.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java,
line 32
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line32>
bq.  >
bq.  >     I think it's more accurate to say that the processor "*checks* to see if the
first call is to set_ugi()..." instead of saying that it *expects* the first call to be to
set_ugi().

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java,
line 50
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line50>
bq.  >
bq.  >     +1 to referencing the THRIFT JIRA. I think the class comment should call out
that this is a temporary workaround cite a TODO.

Done.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java,
line 92
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line92>
bq.  >
bq.  >     Formatting: '} else {'

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java,
line 127
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61789#file61789line127>
bq.  >
bq.  >     There's a TAB here. Please remove.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java, line
519
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61791#file61791line519>
bq.  >
bq.  >     Spacing.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, line
501
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61792#file61792line501>
bq.  >
bq.  >     Please change the name of this method to isSecurityEnabled(), or create a new
method with that name instead of modifying this method. The current name is misleading and
will cause confusion (e.g. my comments in HiveMetaStore).

Reverted this change as no longer required.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java,
line 378
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61793#file61793line378>
bq.  >
bq.  >     Spacing.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java,
line 1
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line1>
bq.  >
bq.  >     Missing ASF header.

Added.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java,
line 16
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line16>
bq.  >
bq.  >     If this is client-specific code then maybe the package should be o.a.h.hive.thrift.client.

Moved.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java,
line 44
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61794#file61794line44>
bq.  >
bq.  >     What's the point of these assert statements? Remove?

Removed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java,
line 78
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61796#file61796line78>
bq.  >
bq.  >     Spacing.

Added.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java,
line 83
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61796#file61796line83>
bq.  >
bq.  >     Spacing.

Fixed.


bq.  On 2011-12-16 03:06:59, Carl Steinbach wrote:
bq.  > trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TFilterTransport.java,
line 1
bq.  > <https://reviews.apache.org/r/2975/diff/2/?file=61797#file61797line1>
bq.  >
bq.  >     ASF header.

Added.


- Ashutosh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2975/#review3939
-----------------------------------------------------------


On 2011-12-03 00:07:25, Ashutosh Chauhan wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/2975/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-12-03 00:07:25)
bq.  
bq.  
bq.  Review request for hive.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Pass user identity in metastore connection in unsecure mode
bq.  
bq.  
bq.  This addresses bug HIVE-2616.
bq.      https://issues.apache.org/jira/browse/HIVE-2616
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 1209772 
bq.    trunk/metastore/if/hive_metastore.thrift 1209772 
bq.    trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.h 1209772 
bq.    trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore.cpp 1209772 
bq.    trunk/metastore/src/gen/thrift/gen-cpp/ThriftHiveMetastore_server.skeleton.cpp 1209772

bq.    trunk/metastore/src/gen/thrift/gen-javabean/org/apache/hadoop/hive/metastore/api/ThriftHiveMetastore.java
1209772 
bq.    trunk/metastore/src/gen/thrift/gen-php/hive_metastore/ThriftHiveMetastore.php 1209772

bq.    trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore-remote 1209772

bq.    trunk/metastore/src/gen/thrift/gen-py/hive_metastore/ThriftHiveMetastore.py 1209772

bq.    trunk/metastore/src/gen/thrift/gen-rb/thrift_hive_metastore.rb 1209772 
bq.    trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1209772

bq.    trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
1209772 
bq.    trunk/metastore/src/java/org/apache/hadoop/hive/metastore/TUGIBasedProcessor.java PRE-CREATION

bq.    trunk/shims/ivy.xml 1209772 
bq.    trunk/shims/src/0.20/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java 1209772 
bq.    trunk/shims/src/0.20S/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 1209772

bq.    trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge20S.java
1209772 
bq.    trunk/shims/src/0.20S/java/org/apache/hadoop/hive/thrift/TUGIAssumingTransport.java
PRE-CREATION 
bq.    trunk/shims/src/common/java/org/apache/hadoop/hive/shims/HadoopShims.java 1209772 
bq.    trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/HadoopThriftAuthBridge.java
1209772 
bq.    trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TFilterTransport.java PRE-CREATION

bq.    trunk/shims/src/common/java/org/apache/hadoop/hive/thrift/TUGIContainingTransport.java
PRE-CREATION 
bq.  
bq.  Diff: https://reviews.apache.org/r/2975/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  All the tests in metastore dir passes. Manually tested that file on hdfs is owned by
user running the client and not by user running metastore server.
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Ashutosh
bq.  
bq.


                
> Passing user identity from metastore client to server in non-secure mode
> ------------------------------------------------------------------------
>
>                 Key: HIVE-2616
>                 URL: https://issues.apache.org/jira/browse/HIVE-2616
>             Project: Hive
>          Issue Type: Bug
>          Components: Metastore
>            Reporter: Ashutosh Chauhan
>            Assignee: Ashutosh Chauhan
>         Attachments: hive-2616.patch, hive-2616_1.patch, hive-2616_3.patch
>
>
> Currently in unsecure mode client don't pass on user identity. As a result hdfs and other
operations done by server gets executed by user running metastore process instead of being
done in context of client. This results in problem as reported here: 
> http://mail-archives.apache.org/mod_mbox/hive-user/201111.mbox/%3CCAK0mCrRC3aPqtRHDe2J25Rm0JX6TS1KXxd7KPjqJjoqBjg=APA@mail.gmail.com%3E

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message