> On Jan. 13, 2015, 9:02 p.m., Christopher Tubbs wrote: > > pom.xml, line 125 > > <https://reviews.apache.org/r/29386/diff/16/?file=815035#file815035line125> > > > > Why bump this? Do we want 2.3.0 to be the default build? Are we > > dropping support for 2.2.x and require 2.3.0 and later? > > Josh Elser wrote: > MiniKDC only shows up in Hadoop 2.3.
Okay, so we need minikdc for testing, but will this impact our support for other 2.2.x at all? > On Jan. 13, 2015, 9:02 p.m., Christopher Tubbs wrote: > > pom.xml, lines 1324-1337 > > <https://reviews.apache.org/r/29386/diff/16/?file=815035#file815035line1324> > > > > If we're dropping Hadoop 1 support, that should be done in a separate > > commit. > > Josh Elser wrote: > Crap. I forgot I did this. MiniKDC only shows up in Hadoop 2.3. Do you > remember if this already on the plate for 1.7? > > Josh Elser wrote: > We did get lazy consensus on droppping Hadoop 1 > http://mail-archives.apache.org/mod_mbox/accumulo-dev/201411.mbox/%3C54694DB3.5000803%40gmail.com%3E Yeah, I just didn't expect to see it here, unless there was some reason why it was needed (like it was required to implement to the feature, which doesn't appear to be the case, although it is required for easily testing the feature). - Also, noticed you made ACCUMULO-3479 to drop Hadoop 1 support. I'm still concerned about 2.2.x support, though, if there is any impact on that. > On Jan. 13, 2015, 9:02 p.m., Christopher Tubbs wrote: > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java, > > lines 620-626 > > <https://reviews.apache.org/r/29386/diff/16/?file=815043#file815043line620> > > > > Doesn't seem like this needed to move. > > Josh Elser wrote: > There was no move. The only thing that functionally changed was the > introduction of the `_createUser` method. That moved around the try/catch > which (I'm guessing) is why RB is showing a move. Okay. Bad ReviewBoard, bad. > On Jan. 13, 2015, 9:02 p.m., Christopher Tubbs wrote: > > test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java, > > lines 185-191 > > <https://reviews.apache.org/r/29386/diff/16/?file=815075#file815075line185> > > > > I think my previous comment about coverage here still holds. This still > > results in failure, but for a different reason than what was originally > > being tested for. > > > > It now fails because there's no user with a password "fake". Before, it > > was testing that a failure would occur if instance.* configuration details > > were different. That case is now absent. > > Josh Elser wrote: > I believe you're mistaken. The original two cases being tested by this > class are still present and performing the exact same assertion as > previously. This case that you're highlighting is a new assertion that was > added to the class. > > I must not be seeing something if you still think there is something > wrong because I did (try to) address this issue already (as you remember). Okay, I see. You are correct. I misinterpreted the context around the change and was looking at the wrong section. I do see that it switched from using the SiteConfiguration, and now uses the DefaultConfiguration instead to generate the system token, in the "bad" case. It's similar enough, but the case we were testing before was when the instanceID was different. Now, it can fail if the config is different also... which is probably a good test case to cover (if not covered elsewhere), but it's not the case being tested before. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29386/#review67955 ----------------------------------------------------------- On Jan. 9, 2015, 5:29 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29386/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2015, 5:29 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2815 > https://issues.apache.org/jira/browse/ACCUMULO-2815 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2815 Initial support for Kerberos client authentication. > > Leverage SASL transport provided by Thrift which can speak GSSAPI, which > Kerberos implements. Introduced... > > * An Accumulo KerberosToken which is an AuthenticationToken to validate users. > * Custom thrift processor and invocation handler to ensure server RPCs have a > valid KRB identity and Accumulo authentication. > * A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos > identities seamlessly. > * New ClientConf variables to use SASL transport and pass Kerberos server > principal > * Updated ClientOpts and Shell opts to transparently use a KerberosToken when > SASL is enabled (no extra client work). > > I believe this is the "bare minimum" for Kerberos support. They are also > grossly lacking in unit and integration tests. I believe that I might have > somehow broken the client address string in the server (I saw log messages > with client: null, but I'm not sure if it's due to these changes or not). A > necessary limitation in the Thrift server used is that, like the SSL > transport, the SASL transport cannot presently be used with the > TFramedTransport, which means none of the [half]async thrift servers will > function with this -- we're stuck with the TThreadPoolServer. > > Performed some contrived benchmarks on my laptop (while still using it > myself) to get at big-picture view of the performance impact against "normal" > operation and Kerberos alone. Each "run" was the duration to ingest 100M > records using continuous-ingest, timed with `time`, using 'real'. > > THsHaServer (our default), 6 runs: > > Avg: 10m7.273s (607.273s) > Min: 9m43.395s > Max: 10m52.715s > > TThreadPoolServer (no SASL), 5 runs: > > Avg: 11m16.254s (676.254s) > Min: 10m30.987s > Max: 12m24.192s > > TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs: > > Avg: 13m17.187s (797.187s) > Min: 10m52.997s > Max: 16m0.975s > > The general takeway is that there's about 15% performance degredation in its > initial state which is in the realm of what I expected (~10%). > > > Diffs > ----- > > README ad6f2bf > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java eb020eb > core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java > df53645 > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java > e75bec6 > core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java > f481cc3 > core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java > fcbf9f9 > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java > 6dc846f > > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java > 5da803b > > core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/conf/Property.java ce5de85 > core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java 09bd6c4 > core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransportFactory.java > PRE-CREATION > core/src/test/java/org/apache/accumulo/core/cli/TestClientOpts.java ff49bc0 > > core/src/test/java/org/apache/accumulo/core/client/ClientConfigurationTest.java > PRE-CREATION > > core/src/test/java/org/apache/accumulo/core/client/impl/ThriftTransportKeyTest.java > PRE-CREATION > > core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java > 40be70f > > core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java > PRE-CREATION > docs/src/main/asciidoc/accumulo_user_manual.asciidoc ec8e538 > docs/src/main/asciidoc/chapters/clients.txt 64f0e55 > docs/src/main/asciidoc/chapters/kerberos.txt PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java > 27d6b19 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java > 26c23ed > pom.xml 203acdc > proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 7eb4fbf > > server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java > 09ae4f4 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > ed2189d > > server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandler.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingWrapper.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java > 641c0bf > > server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java > 5fe57b7 > > server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java > 42d1313 > > server/base/src/main/java/org/apache/accumulo/server/security/SystemCredentials.java > 79201b1 > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthorizor.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosPermissionHandler.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/thrift/UGIAssumingProcessor.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/util/Admin.java > 7d247f7 > server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java > ef182f1 > > server/base/src/test/java/org/apache/accumulo/server/AccumuloServerContextTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandlerTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/rpc/ThriftServerTypeTest.java > PRE-CREATION > > server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java > 01ff9ac > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > db37c8b > > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java > f98721f > > server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java > 99558b8 > > server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java > cad1e01 > server/master/src/main/java/org/apache/accumulo/master/Master.java 12195fa > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/Basic.java > 2d98fed > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 3063cdc > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 2bfa5a0 > shell/src/main/java/org/apache/accumulo/shell/Shell.java 9697a85 > shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java 875367d > shell/src/test/java/org/apache/accumulo/shell/ShellOptionsJCTest.java > PRE-CREATION > test/pom.xml b0a926f > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > 3bb44ff > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 0afa243 > test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java > 8f7e1b7 > test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java > abdb627 > test/src/test/java/org/apache/accumulo/harness/SharedMiniClusterIT.java > 2380f66 > test/src/test/java/org/apache/accumulo/harness/TestingKdc.java PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloMiniClusterConfiguration.java > 11b7530 > > test/src/test/java/org/apache/accumulo/server/security/SystemCredentialsIT.java > abbe5e6 > test/src/test/java/org/apache/accumulo/test/ArbitraryTablePropertiesIT.java > aa5c164 > test/src/test/java/org/apache/accumulo/test/CleanWalIT.java 1fcd5a4 > > test/src/test/java/org/apache/accumulo/test/functional/BatchScanSplitIT.java > 221889b > test/src/test/java/org/apache/accumulo/test/functional/KerberosIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/security/KerberosTokenTest.java > PRE-CREATION > test/src/test/resources/log4j.properties cb35840 > > Diff: https://reviews.apache.org/r/29386/diff/ > > > Testing > ------- > > Ensure existing unit tests still function. Accumulo is functional and ran > continuous ingest multiple times using a client with only a Kerberos identity > (no user/password provided). Used MIT Kerberos with Apache Hadoop 2.6.0 and > Apache ZooKeeper 3.4.5. > > > Thanks, > > Josh Elser > >
