----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31339/#review75354 -----------------------------------------------------------
flume-ng-auth/pom.xml <https://reviews.apache.org/r/31339/#comment122340> Dependencies only used in tests should be in the test scope rather than using optional. flume-ng-auth/src/main/java/org/apache/flume/auth/FlumeAuthenticationUtil.java <https://reviews.apache.org/r/31339/#comment122333> These should be separate Preconditions so that the correct error message is thrown. flume-ng-auth/src/main/java/org/apache/flume/auth/FlumeAuthenticationUtil.java <https://reviews.apache.org/r/31339/#comment122334> Will a failed authentication leave the authenticator in an unusable state? Why is it not set as the authenticator until the auth method succeeds? flume-ng-auth/src/main/java/org/apache/flume/auth/FlumeAuthenticator.java <https://reviews.apache.org/r/31339/#comment122335> I think this name could be more clear. What about `isAuthenticated`? flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122344> This is a use of slf4j, so I don't think the comments that it is only required for tests in the POM are correct. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122342> This duplicates what is done in `ProxyExecutor`, but adds the call to relogin. 1. Shouldn't relogin happen even if you're using a proxy user? 2. This duplicates the code in `ProxyExecutor`, when you could keep the right executor around and delegate to it. I think that would be a better option to ensure exception handling and other tasks are consistent as this is maintained over time. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122349> The `doAs` method also throws `IOException` and the user's action could as well. The try/catch should be separated so that the relogin exceptions are handled without mixing them up with the `doAs` exceptions. While you're doing that, you could put the relogin code in a private method, like `ensureValidAuth`, that is used by both execute methods. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122343> The `Exception` case doesn't need to be here. The checked exception is thrown by user code, not by auth framework code, so it is preferable for the method to pass the checked exception along. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122351> Minor: would be better to use Preconditions. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122357> This debug statement isn't true because `this.ugi` is never set to `curUser`. I think the logic in the last part of `KerberosUtil` is a bit cleaner, you might have a look at it to see if it could simplify this code. Using preconditions would help here, too. Also, please add a test for this case because I don't think it would be passing. flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java <https://reviews.apache.org/r/31339/#comment122345> All of the messages in this method should be combined into one log call. flume-ng-auth/src/main/java/org/apache/flume/auth/PrivilegedExecutor.java <https://reviews.apache.org/r/31339/#comment122341> The javadoc here should be filled in. flume-ng-auth/src/main/java/org/apache/flume/auth/ProxyExecutor.java <https://reviews.apache.org/r/31339/#comment122346> While used for Proxy users, there's nothing that requires this to be a proxy. Maybe UGIExecutor would be a better name. flume-ng-sinks/flume-dataset-sink/src/main/java/org/apache/flume/sink/kite/DatasetSink.java <https://reviews.apache.org/r/31339/#comment122337> Minor: Avoid wildcard imports and unnecessary import changes. flume-ng-sinks/flume-dataset-sink/src/main/java/org/apache/flume/sink/kite/DatasetSink.java <https://reviews.apache.org/r/31339/#comment122336> This should replace the login on line 74. - Ryan Blue On March 4, 2015, 8:58 p.m., Johny Rufus John wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31339/ > ----------------------------------------------------------- > > (Updated March 4, 2015, 8:58 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/FlumeAuthenticationUtil.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/auth/FlumeAuthenticator.java > PRE-CREATION > > flume-ng-auth/src/main/java/org/apache/flume/auth/KerberosAuthenticator.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/auth/PrivilegedExecutor.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/auth/ProxyExecutor.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/auth/SecurityException.java > PRE-CREATION > flume-ng-auth/src/main/java/org/apache/flume/auth/SimpleAuthenticator.java > PRE-CREATION > > flume-ng-auth/src/test/java/org/apache/flume/auth/TestFlumeAuthenticator.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/BucketWriter.java > 62f4eee > > 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 > >
