Re: Review Request 66081: Kerberos authentication in lens

[Ankit Kailaswar]

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

(Updated April 2, 2018, 12:58 p.m.)


Review request for lens, Amareshwari Sriramadasu, Rajat Khandelwal, and Puneet 
Gupta.


Repository: lens


Description
---

https://issues.apache.org/jira/browse/LENS-1506

This patch contains code changes to enable kerberos authentication for 
1. lens to hive 
2. lens to metastore
3. lens to hdfs

code changes are as follows,
1. new http thrift client for hive driver to support sasl transport for 
kerberozied hive server.
2. cron to update KDC ticket before it expires.


Diffs (updated)
-

  lens-driver-hive/src/main/java/org/apache/lens/driver/hive/HiveDriver.java 
2eb94aa706c648b50dce1bbb5360ddd22a242295 
  
lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RemoteThriftConnection.java
 54885f77643d5723ab7735f5b666ce6bfa75a782 
  
lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java
 PRE-CREATION 
  
lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java 
d5273be8b1d0c6c1e63da049c1cba712a5817903 
  lens-server/src/main/java/org/apache/lens/server/BaseLensService.java 
c30a2d72d27acdbab4c175e6e723f6369d934de9 
  
lens-server/src/main/java/org/apache/lens/server/session/HiveSessionService.java
 31ac358d0a1747b1dbfe1d6dacaf0fb82556475d 
  lens-server/src/main/resources/lensserver-default.xml 
28b1db29a8be7dc9762a2ce413a988d3b14b8543 
  lens-server/src/test/resources/lens-site.xml 
7e5f522ea581e83dfd85210f27f10473e93c0097 
  tools/scripts/lens-ctl 304b4f5a8b82a5d313cdf31852cf502a214ba83b 


Diff: https://reviews.apache.org/r/66081/diff/3/

Changes: https://reviews.apache.org/r/66081/diff/2-3/


Testing
---

unit testing


Thanks,

Ankit Kailaswar

Re: Review Request 66081: Kerberos authentication in lens

[Ankit Kailaswar]

> On March 20, 2018, 6:37 a.m., Rajat Khandelwal wrote:
> > lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java
> > Lines 125 (patched)
> > 
> >
> > This class is also redundant. Can we reuse 
> > `org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient.InvocationResult`?

it is protected inner class, usage are scoped within package only. Wont be a 
good idea to follow package struture for single class in lens. cant even extend 
outside package, no default ctor available for InvocationResult in 
RetryingThriftCLIServiceClient.


> On March 20, 2018, 6:37 a.m., Rajat Khandelwal wrote:
> > lens-driver-hive/src/main/java/org/apache/lens/driver/hive/RetryingThriftCLIServiceClientSasl.java
> > Lines 137-182 (patched)
> > 
> >
> > Both these methods can be easily avoided by simply extending 
> > `RetryingThriftCLIServiceClient`.

it can't be extended form RetryingThriftCLIServiceClient please read above 
reply to puneet's comment.


On March 20, 2018, 6:37 a.m., Ankit Kailaswar wrote:
> > Current design:
> > ```java
> > HiveConf conf;
> > if (conf.auth == Kerberose) {
> > org.apache.lens.driver.hive.RetryingThriftCLIServiceClientSasl.newClient
> > } else {
> > 
> > org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient.newClient
> > }
> > 
> > 
> > class RetryingThriftCLIServiceClientSasl {
> > // some reuse from RetryingThriftCLIServiceClient
> > // some code copied from RetryingThriftCLIServiceClient
> > }
> > 
> > ```
> > 
> > Proposal: 
> > 
> > 
> > ```java
> > HiveConf conf;
> > 
> > org.apache.lens.driver.hive.client.thrift.RetryingThriftCLIServiceClient.newClient
> > 
> > class RetryingThriftCLIServiceClient extends 
> > org.apache.hive.service.cli.thrift.RetryingThriftCLIServiceClient {
> > @Override
> > protected synchronized TTransport connect(HiveConf conf) throws 
> > HiveSQLException, TTransportException {
> > if (this.transport != null && this.transport.isOpen()) {
> > this.transport.close();
> > }
> > 
> > String host = conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_BIND_HOST);
> > int port = conf.getIntVar(ConfVars.HIVE_SERVER2_THRIFT_PORT);
> > LOG.info("Connecting to " + host + ":" + port);
> > this.transport = new TSocket(host, port);
> > 
> > ((TSocket)this.transport).setTimeout((int)conf.getTimeVar(ConfVars.SERVER_READ_SOCKET_TIMEOUT,
> >  TimeUnit.SECONDS) * 1000);
> > 
> > try {
> > 
> > ((TSocket)this.transport).getSocket().setKeepAlive(conf.getBoolVar(ConfVars.SERVER_TCP_KEEP_ALIVE));
> > } catch (SocketException var8) {
> > LOG.error("Error setting keep alive to " + 
> > conf.getBoolVar(ConfVars.SERVER_TCP_KEEP_ALIVE), var8);
> > }
> > /* Move the following code to new method getTransport below in 
> > the else part: 
> > String userName = 
> > conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_CLIENT_USER);
> > String passwd = 
> > conf.getVar(ConfVars.HIVE_SERVER2_THRIFT_CLIENT_PASSWORD);
> > 
> > try {
> > this.transport = PlainSaslHelper.getPlainTransport(userName, 
> > passwd, this.transport);
> > } catch (SaslException var7) {
> > LOG.error("Error creating plain SASL transport", var7);
> > }
> > */
> > this.transport = getTransport(conf);
> > TProtocol protocol = new TBinaryProtocol(this.transport);
> > this.transport.open();
> > this.base = new ThriftCLIServiceClient(new Client(protocol));
> > LOG.info("Connected!");
> > return this.transport;
> > }
> > protected TTransport getTransport(HiveConf conf) {
> > if (conf.auth == Kerberose) {
> > // use KerberoseSaslHelper
> > } else {
> > // use PlainSaslHelper
> > }
> > }
> > }
> > 
> > ```

member variables of base class cant be accesses from derived class.


- Ankit


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


On March 20, 2018, 8:43 p.m., Ankit Kailaswar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66081/
> ---
> 
> (Updated March 20, 2018, 8:43 p.m.)
> 
> 
> Review request for lens, Amareshwari Sriramadasu, Rajat Khandelwal, and 
> Puneet Gupta.
> 
> 
> Repository: lens
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/LENS-1506
> 
> This patch contains code changes to enable kerberos