----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31339/#review74863 -----------------------------------------------------------
It looks like KerberosAuthenticator is primarily based on the code from HDFS, which makes sense. That's where the DatasetSink started to get its authentication code, KerberosUtil. But KerberosUtil was already an effort to make that code reusable and we found bugs in it, so it is concerning that this replicates that work rather than building on it. This API is also a directly copy of the private internal methods in the HDFS sink without major changes. Because this is a new API and will be used in the secure SDK, we need to be careful about the API that is exposed. For example, what is the major difference between `kerberosLogin` and `authenticate`? Why are these two separate methods rather than a single method? Why are there both `executePrivileged` and `runPrivileged` methods? flume-ng-auth/pom.xml <https://reviews.apache.org/r/31339/#comment121681> This uses Guava Preconditions and should depend on guava. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121677> This license block shouldn't be here. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121865> Could you add how this differs from the static credentials in `UserGroupInformation`? flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121862> If this method is kept in the API rather than merged with `kerberosLogin`, then it will need javadoc. Other methods will as well, but this should clarify why it is used and under what conditions it returns false or throws an Exceptino. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121854> Calls to `new Configuration()` should be avoided. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121863> I think this method should return the UGI rather than returning true and relying on static state via the `getUGI` method. Errors should be handled with exception classes and clear messages about what happened rather than logging errors and returning false. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121859> Why does this use `UserGroupInformation.getLoginUser()`? The `executePrivileged` methods use the `staticLogin` user. Like `executePrivileged`, I think this version of `getProxyUser` should be removed in favor of the version where a UGI is passed in. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121858> I think that these should be removed in favor of runPrivileged with the current UGI from `kerberosLogin` or `getProxyUser`. While this may need to keep a static copy of the current user, this isn't a class that keeps state and shouldn't be used as one. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121853> These log calls should be combined into one for the logged-in user and one for the superuser. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment121864> When should this use `staticLogin` vs. `getLoginUser`? The difference is not clear. - Ryan Blue On Feb. 27, 2015, 2:22 p.m., Johny Rufus John wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31339/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 2:22 p.m.) > > > Review request for Flume. > > > Bugs: FLUME-2631 > https://issues.apache.org/jira/browse/FLUME-2631 > > > Repository: flume-git > > > Description > ------- > > End to End authentication in Flume > > > Diffs > ----- > > flume-ng-auth/pom.xml PRE-CREATION > > flume-ng-auth/src/main/java/org/apache/flume/api/SecureRpcClientFactory.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/api/SecureThriftRpcClient.java > PRE-CREATION > > flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java > PRE-CREATION > > flume-ng-auth/src/test/java/org/apache/flume/auth/TestKerberosAuthenticator.java > PRE-CREATION > flume-ng-core/pom.xml 8992414 > flume-ng-core/src/main/java/org/apache/flume/sink/ThriftSink.java baa60d0 > flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java > 06bb604 > flume-ng-dist/pom.xml a083fe2 > flume-ng-dist/src/main/assembly/bin.xml 5aa7cc6 > flume-ng-dist/src/main/assembly/src.xml b1e79a2 > > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java > 33a2330 > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java > 4f75a2b > flume-ng-sinks/flume-dataset-sink/pom.xml e929d60 > > flume-ng-sinks/flume-dataset-sink/src/main/java/org/apache/flume/sink/kite/DatasetSink.java > 3e66532 > > flume-ng-sinks/flume-dataset-sink/src/main/java/org/apache/flume/sink/kite/KerberosUtil.java > c0dbffb > > flume-ng-sinks/flume-dataset-sink/src/test/java/org/apache/flume/sink/kite/TestKerberosUtil.java > f53ef73 > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 33f73a9 > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSink.java > 5de0bd5 > > flume-ng-sinks/flume-ng-hbase-sink/src/main/java/org/apache/flume/sink/hbase/HBaseSinkSecurityManager.java > 762fce9 > pom.xml ea7ffe3 > > Diff: https://reviews.apache.org/r/31339/diff/ > > > Testing > ------- > > Tested in kerberos cluster with combinations of Thrift Src, Sink and Hdfs Sink > > > Thanks, > > Johny Rufus John > >
