Review Request 19801: review request for HIVE-6738, HiveServer2 secure Thrift/HTTP needs to accept doAs parameter from proxying intermediary

2014-03-28 Thread dilli dorai

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

Review request for hive, Thejas Nair and Vaibhav Gumashta.


Bugs: HIVE-6738
https://issues.apache.org/jira/browse/HIVE-6738


Repository: hive-git


Description
---

see the jira HIVE-6738


Diffs
-

  service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
7f6687e 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 
58f3e3b 
  service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java 
c579db5 

Diff: https://reviews.apache.org/r/19801/diff/


Testing
---

see the attachment to Jira HIVE-6738
https://issues.apache.org/jira/secure/attachment/12637059/hive-6738-req-impl-verify.md


Thanks,

dilli dorai



Re: Review Request 19801: review request for HIVE-6738, HiveServer2 secure Thrift/HTTP needs to accept doAs parameter from proxying intermediary

2014-03-28 Thread Vaibhav Gumashta

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



service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java
https://reviews.apache.org/r/19801/#comment71348

Can we use proxyUserNameParam for doAsQueryParam? doAs in hive world is 
different than proxy user like we discussed offline. 



service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java
https://reviews.apache.org/r/19801/#comment71349

Similar to the previous comment: I think it'll be good to change the method 
name to getProxyUserNameParam from getDoAsQueryParam.


- Vaibhav Gumashta


On March 28, 2014, 9:08 p.m., dilli dorai wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/19801/
 ---
 
 (Updated March 28, 2014, 9:08 p.m.)
 
 
 Review request for hive, Thejas Nair and Vaibhav Gumashta.
 
 
 Bugs: HIVE-6738
 https://issues.apache.org/jira/browse/HIVE-6738
 
 
 Repository: hive-git
 
 
 Description
 ---
 
 see the jira HIVE-6738
 
 
 Diffs
 -
 
   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
 7f6687e 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 
 58f3e3b 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java 
 c579db5 
 
 Diff: https://reviews.apache.org/r/19801/diff/
 
 
 Testing
 ---
 
 see the attachment to Jira HIVE-6738
 https://issues.apache.org/jira/secure/attachment/12637059/hive-6738-req-impl-verify.md
 
 
 Thanks,
 
 dilli dorai
 




Re: Review Request 19801: review request for HIVE-6738, HiveServer2 secure Thrift/HTTP needs to accept doAs parameter from proxying intermediary

2014-03-28 Thread Vaibhav Gumashta

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



service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
https://reviews.apache.org/r/19801/#comment71355

I think we should set proxyUser from the threadlocal only when HS2 is 
running in http mode - this will avoid any future confusion while development. 
This is the block you can use:

if 
(hiveConf.getVar(HiveConf.ConfVars.HIVE_SERVER2_TRANSPORT_MODE).equalsIgnoreCase(http))
 {
}


- Vaibhav Gumashta


On March 28, 2014, 9:08 p.m., dilli dorai wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/19801/
 ---
 
 (Updated March 28, 2014, 9:08 p.m.)
 
 
 Review request for hive, Thejas Nair and Vaibhav Gumashta.
 
 
 Bugs: HIVE-6738
 https://issues.apache.org/jira/browse/HIVE-6738
 
 
 Repository: hive-git
 
 
 Description
 ---
 
 see the jira HIVE-6738
 
 
 Diffs
 -
 
   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
 7f6687e 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 
 58f3e3b 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java 
 c579db5 
 
 Diff: https://reviews.apache.org/r/19801/diff/
 
 
 Testing
 ---
 
 see the attachment to Jira HIVE-6738
 https://issues.apache.org/jira/secure/attachment/12637059/hive-6738-req-impl-verify.md
 
 
 Thanks,
 
 dilli dorai
 




Re: Review Request 19801: review request for HIVE-6738, HiveServer2 secure Thrift/HTTP needs to accept doAs parameter from proxying intermediary

2014-03-28 Thread dilli dorai


 On March 28, 2014, 11:43 p.m., Vaibhav Gumashta wrote:
  service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java, 
  line 82
  https://reviews.apache.org/r/19801/diff/1/?file=539920#file539920line82
 
  Can we use proxyUserNameParam for doAsQueryParam? doAs in hive world is 
  different than proxy user like we discussed offline.

First, thanks for the review.
I think the query parameter name has to be doAs, case does not matter.
That is the interface used by other HTTP services of Hadoop echo system.


 On March 28, 2014, 11:43 p.m., Vaibhav Gumashta wrote:
  service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java, 
  line 309
  https://reviews.apache.org/r/19801/diff/1/?file=539920#file539920line309
 
  Similar to the previous comment: I think it'll be good to change the 
  method name to getProxyUserNameParam from getDoAsQueryParam.

See the comment above.


- dilli


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


On March 28, 2014, 9:08 p.m., dilli dorai wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/19801/
 ---
 
 (Updated March 28, 2014, 9:08 p.m.)
 
 
 Review request for hive, Thejas Nair and Vaibhav Gumashta.
 
 
 Bugs: HIVE-6738
 https://issues.apache.org/jira/browse/HIVE-6738
 
 
 Repository: hive-git
 
 
 Description
 ---
 
 see the jira HIVE-6738
 
 
 Diffs
 -
 
   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
 7f6687e 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 
 58f3e3b 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java 
 c579db5 
 
 Diff: https://reviews.apache.org/r/19801/diff/
 
 
 Testing
 ---
 
 see the attachment to Jira HIVE-6738
 https://issues.apache.org/jira/secure/attachment/12637059/hive-6738-req-impl-verify.md
 
 
 Thanks,
 
 dilli dorai
 




Re: Review Request 19801: review request for HIVE-6738, HiveServer2 secure Thrift/HTTP needs to accept doAs parameter from proxying intermediary

2014-03-28 Thread dilli dorai


 On March 29, 2014, 12:08 a.m., Vaibhav Gumashta wrote:
  service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java, 
  line 543
  https://reviews.apache.org/r/19801/diff/1/?file=539919#file539919line543
 
  I think we should set proxyUser from the threadlocal only when HS2 is 
  running in http mode - this will avoid any future confusion while 
  development. This is the block you can use:
  
  if 
  (hiveConf.getVar(HiveConf.ConfVars.HIVE_SERVER2_TRANSPORT_MODE).equalsIgnoreCase(http))
   {
  }

The value is set at the higher layer only if the request was over http and we 
got a query parameter.
So, I think this check would be superfluous.

If Hive code later adds a different code path to set the value without http 
mode, that should be an intentional design choice at that time.


- dilli


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


On March 28, 2014, 9:08 p.m., dilli dorai wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/19801/
 ---
 
 (Updated March 28, 2014, 9:08 p.m.)
 
 
 Review request for hive, Thejas Nair and Vaibhav Gumashta.
 
 
 Bugs: HIVE-6738
 https://issues.apache.org/jira/browse/HIVE-6738
 
 
 Repository: hive-git
 
 
 Description
 ---
 
 see the jira HIVE-6738
 
 
 Diffs
 -
 
   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
 7f6687e 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java 
 58f3e3b 
   service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java 
 c579db5 
 
 Diff: https://reviews.apache.org/r/19801/diff/
 
 
 Testing
 ---
 
 see the attachment to Jira HIVE-6738
 https://issues.apache.org/jira/secure/attachment/12637059/hive-6738-req-impl-verify.md
 
 
 Thanks,
 
 dilli dorai