This is an automated email from the ASF dual-hosted git repository. jbapple pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit d72f3330c1edc9086ba120e6d3469a75c0aea083 Author: Thomas Tauber-Marshall <tmarsh...@cloudera.com> AuthorDate: Tue May 14 12:21:03 2019 -0700 IMPALA-8545: Test Ldap authentication Currently, Impala does not have any automated tests for LDAP auth functionality, due to the challenge of setting up an LDAP server for use by the minicluster. This patch adds LDAP tests by utilizing the ApacheDS project's unit testing functionality, which works with JUnit to setup up a local LDAP server for the duration of a test suite. This requires running an Impala cluster with custom arguments to set up LDAP auth. This patch introduces a concept of FE custom cluster tests which must be in the package org.apache.impala.customcluster. These tests are filtered out from the other FE tests in bin/run-all-tests.sh and run with the other custom cluster tests so that they don't affect other tests that expect Impala to have been started with particular flags. Testing: - Ran a full core run and confirmed that new tests run as expected. Change-Id: I92b5e60860c60209c1bd8afe5b3ea201fb7a7513 Reviewed-on: http://gerrit.cloudera.org:8080/13337 Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Reviewed-by: Tim Armstrong <tarmstr...@cloudera.com> --- bin/rat_exclude_files.txt | 1 + bin/run-all-tests.sh | 11 ++ fe/pom.xml | 7 ++ .../impala/customcluster/CustomClusterRunner.java | 44 ++++++++ .../apache/impala/customcluster/LdapJdbcTest.java | 105 ++++++++++++++++++ .../java/org/apache/impala/service/JdbcTest.java | 90 +-------------- .../org/apache/impala/service/JdbcTestBase.java | 122 +++++++++++++++++++++ .../apache/impala/testutil/ImpalaJdbcClient.java | 17 ++- fe/src/test/resources/users.ldif | 25 +++++ 9 files changed, 333 insertions(+), 89 deletions(-) diff --git a/bin/rat_exclude_files.txt b/bin/rat_exclude_files.txt index 4c255e8..e352a17 100644 --- a/bin/rat_exclude_files.txt +++ b/bin/rat_exclude_files.txt @@ -102,6 +102,7 @@ be/src/testutil/*.pem fe/src/test/resources/*.xml fe/src/test/resources/hbase-jaas-client.conf.template fe/src/test/resources/hbase-jaas-server.conf.template +fe/src/test/resources/users.ldif testdata/AllTypesError/*.txt testdata/AllTypesErrorNoNulls/*.txt *.avsc diff --git a/bin/run-all-tests.sh b/bin/run-all-tests.sh index b74d03c..d124e03 100755 --- a/bin/run-all-tests.sh +++ b/bin/run-all-tests.sh @@ -195,6 +195,9 @@ do if [[ "$CODE_COVERAGE" == true ]]; then MVN_ARGS+="-DcodeCoverage" fi + # Don't run the FE custom cluster tests here since they restart Impala. We'll run them + # with the other custom cluster tests below. + MVN_ARGS+=" -Dtest=!org.apache.impala.customcluster.*Test " if ! "${IMPALA_HOME}/bin/mvn-quiet.sh" -fae test ${MVN_ARGS}; then TEST_RET_CODE=1 fi @@ -242,6 +245,14 @@ do TEST_RET_CODE=1 fi export IMPALA_MAX_LOG_FILES="${IMPALA_MAX_LOG_FILES_SAVE}" + + # Run the FE custom cluster tests. + pushd "${IMPALA_FE_DIR}" + MVN_ARGS=" -Dtest=org.apache.impala.customcluster.*Test " + if ! "${IMPALA_HOME}/bin/mvn-quiet.sh" -fae test ${MVN_ARGS}; then + TEST_RET_CODE=1 + fi + popd fi # Run the process failure tests. diff --git a/fe/pom.xml b/fe/pom.xml index 62fc500..d6c9908 100644 --- a/fe/pom.xml +++ b/fe/pom.xml @@ -412,6 +412,13 @@ under the License. <version>2.23.4</version> <scope>test</scope> </dependency> + + <dependency> + <groupId>org.apache.directory.server</groupId> + <artifactId>apacheds-test-framework</artifactId> + <version>2.0.0.AM25</version> + <scope>test</scope> + </dependency> </dependencies> <reporting> diff --git a/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java b/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java new file mode 100644 index 0000000..7081a9d --- /dev/null +++ b/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java @@ -0,0 +1,44 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.customcluster; + +import java.io.IOException; + +/** + * Runs an Impala cluster with custom flags. + * + * In order to prevent this from affecting other tests, we filter on the package name to + * run FE custom cluster tests when we run the python custom cluster tests, so this should + * not be used outside of this package. + */ +class CustomClusterRunner { + public static int StartImpalaCluster() throws IOException, InterruptedException { + return StartImpalaCluster(""); + } + + /** + * Starts Impala and passes 'args' to the impalads, catalogd, and statestored. + */ + public static int StartImpalaCluster(String args) + throws IOException, InterruptedException { + Process p = Runtime.getRuntime().exec(new String[] {"start-impala-cluster.py", + "--impalad_args", args, "--catalogd_args", args, "--state_store_args", args}); + p.waitFor(); + return p.exitValue(); + } +} diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java new file mode 100644 index 0000000..8c77cc4 --- /dev/null +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.customcluster; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; + +import org.apache.directory.server.core.annotations.CreateDS; +import org.apache.directory.server.core.annotations.CreatePartition; +import org.apache.directory.server.annotations.CreateLdapServer; +import org.apache.directory.server.annotations.CreateTransport; +import org.apache.directory.server.core.annotations.ApplyLdifFiles; +import org.apache.directory.server.core.integ.CreateLdapServerRule; +import org.apache.impala.testutil.ImpalaJdbcClient; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; + +import org.apache.impala.service.JdbcTestBase; + +/** + * Tests for connecting to Impala when LDAP authentication is in use. + */ +@CreateDS(name = "myDS", + partitions = { @CreatePartition(name = "test", suffix = "dc=myorg,dc=com") }) +@CreateLdapServer( + transports = { @CreateTransport(protocol = "LDAP", address = "localhost") }) +@ApplyLdifFiles({"users.ldif"}) +public class LdapJdbcTest extends JdbcTestBase { + @ClassRule + public static CreateLdapServerRule serverRule = new CreateLdapServerRule(); + + // These correspond to the values in fe/src/test/resources/users.ldif + private static final String testUser_ = "Test1Ldap"; + private static final String testPassword_ = "12345"; + + @BeforeClass + public static void setUp() throws Exception { + String uri = + String.format("ldap://localhost:%s", serverRule.getLdapServer().getPort()); + String dn = "cn=#UID,ou=Users,dc=myorg,dc=com"; + String ldapArgs = String.format("--enable_ldap_auth --ldap_uri='%s' " + + "--ldap_bind_pattern='%s' --ldap_passwords_in_clear_ok", uri, dn); + int ret = CustomClusterRunner.StartImpalaCluster(ldapArgs); + assertEquals(ret, 0); + + con_ = + createConnection(ImpalaJdbcClient.getLdapConnectionStr(testUser_, testPassword_)); + } + + @AfterClass + public static void cleanUp() throws Exception { + JdbcTestBase.cleanUp(); + CustomClusterRunner.StartImpalaCluster(); + } + + @Test + public void testLoggedInUser() throws Exception { + ResultSet rs = con_.createStatement().executeQuery("select logged_in_user() user"); + assertTrue(rs.next()); + assertEquals(rs.getString("user"), testUser_); + assertFalse(rs.next()); + } + + @Test + public void testFailedConnection() throws Exception { + try { + Connection con = createConnection( + ImpalaJdbcClient.getLdapConnectionStr(testUser_, "invalid-password")); + fail("Connecting with an invalid password should throw an error."); + } catch (SQLException e) { + assertTrue(e.getMessage().contains("Could not open client transport")); + } + + try { + Connection con = createConnection( + ImpalaJdbcClient.getLdapConnectionStr("invalid-user", testPassword_)); + fail("Connecting with an invalid user name should throw an error."); + } catch (SQLException e) { + assertTrue(e.getMessage().contains("Could not open client transport")); + } + } +} diff --git a/fe/src/test/java/org/apache/impala/service/JdbcTest.java b/fe/src/test/java/org/apache/impala/service/JdbcTest.java index 1749fbe..1fbc0d0 100644 --- a/fe/src/test/java/org/apache/impala/service/JdbcTest.java +++ b/fe/src/test/java/org/apache/impala/service/JdbcTest.java @@ -23,12 +23,10 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import java.io.StringReader; import java.sql.Connection; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; -import java.sql.Statement; import java.sql.Types; import java.util.ArrayList; import java.util.Arrays; @@ -36,22 +34,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import org.apache.impala.analysis.CreateTableStmt; -import org.apache.impala.analysis.Parser; -import org.apache.impala.analysis.Parser.ParseException; -import org.apache.impala.analysis.SqlParser; -import org.apache.impala.analysis.SqlScanner; -import org.apache.impala.analysis.StatementBase; import org.apache.impala.testutil.ImpalaJdbcClient; -import org.apache.impala.thrift.TQueryOptions; import org.apache.impala.util.Metrics; -import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; -import com.google.common.collect.Lists; - /** * JdbcTest * @@ -59,82 +47,10 @@ import com.google.common.collect.Lists; * getTableTypes, getColumnNames. * */ -public class JdbcTest { - private static Connection con_; - - // Test-local list of test tables. These are cleaned up in @After. - private final List<String> testTableNames_ = Lists.newArrayList(); - +public class JdbcTest extends JdbcTestBase { @BeforeClass public static void setUp() throws Exception { - con_ = createConnection(); - } - - @AfterClass - public static void cleanUp() throws Exception { - con_.close(); - assertTrue("Connection should be closed", con_.isClosed()); - - Exception expectedException = null; - try { - con_.createStatement(); - } catch (Exception e) { - expectedException = e; - } - - assertNotNull("createStatement() on closed connection should throw exception", - expectedException); - } - - private static Connection createConnection() throws Exception { - ImpalaJdbcClient client = ImpalaJdbcClient.createClientUsingHiveJdbcDriver(); - client.connect(); - Connection connection = client.getConnection(); - - assertNotNull("Connection is null", connection); - assertFalse("Connection should not be closed", connection.isClosed()); - Statement stmt = connection.createStatement(); - assertNotNull("Statement is null", stmt); - - return connection; - } - - protected void addTestTable(String createTableSql) throws Exception { - // Parse the stmt to extract the table name. We do this first to ensure - // that we do not execute arbitrary SQL here and pollute the test setup. - StatementBase result = Parser.parse(createTableSql); - if (!(result instanceof CreateTableStmt)) { - throw new Exception("Given stmt is not a CREATE TABLE stmt: " + createTableSql); - } - - // Execute the stmt. - Statement stmt = con_.createStatement(); - try { - stmt.execute(createTableSql); - } finally { - stmt.close(); - } - - // Once the stmt was executed successfully, add the fully-qualified table name - // for cleanup in @After. - CreateTableStmt parsedStmt = (CreateTableStmt) result; - testTableNames_.add(parsedStmt.getTblName().toString()); - } - - protected void dropTestTable(String tableName) throws SQLException { - Statement stmt = con_.createStatement(); - try { - stmt.execute("DROP TABLE " + tableName); - } finally { - stmt.close(); - } - } - - @After - public void testCleanUp() throws SQLException { - for (String tableName: testTableNames_) { - dropTestTable(tableName); - } + con_ = createConnection(ImpalaJdbcClient.getNoAuthConnectionStr()); } @Test @@ -638,7 +554,7 @@ public class JdbcTest { List<Long> lastTimeSessionActive = new ArrayList<>(); for (int timeout : timeoutPeriods) { - connections.add(createConnection()); + connections.add(createConnection(ImpalaJdbcClient.getNoAuthConnectionStr())); } Long numOpenSessions = (Long)metrics.getMetric( diff --git a/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java b/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java new file mode 100644 index 0000000..74e309a --- /dev/null +++ b/fe/src/test/java/org/apache/impala/service/JdbcTestBase.java @@ -0,0 +1,122 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.service; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; + +import org.apache.impala.analysis.CreateTableStmt; +import org.apache.impala.analysis.Parser; +import org.apache.impala.analysis.StatementBase; +import org.apache.impala.testutil.ImpalaJdbcClient; +import org.junit.After; +import org.junit.AfterClass; + +import com.google.common.collect.Lists; + +/** + * Base class providing utility functions for tests that need a Jdbc connection. + */ +public abstract class JdbcTestBase { + protected static Connection con_; + + // Test-local list of test tables. These are cleaned up in @After. + protected final List<String> testTableNames_ = Lists.newArrayList(); + + /** + * Closes 'con_'. Any subclasses that specify their own 'AfterClass' will need to call + * this function there. + */ + @AfterClass + public static void cleanUp() throws Exception { + con_.close(); + assertTrue("Connection should be closed", con_.isClosed()); + + Exception expectedException = null; + try { + con_.createStatement(); + } catch (Exception e) { + expectedException = e; + } + + assertNotNull("createStatement() on closed connection should throw exception", + expectedException); + } + + protected static Connection createConnection(String connStr) throws Exception { + ImpalaJdbcClient client = ImpalaJdbcClient.createClientUsingHiveJdbcDriver(connStr); + client.connect(); + Connection connection = client.getConnection(); + + assertNotNull("Connection is null", connection); + assertFalse("Connection should not be closed", connection.isClosed()); + Statement stmt = connection.createStatement(); + assertNotNull("Statement is null", stmt); + + return connection; + } + + /** + * Runs 'createTableSql', which must be a "CREATE TABLE" sql statement, and stores the + * name of the created table in 'testTableNames_' so that it can be dropped after the + * test case by testCleanUp(). + */ + protected void addTestTable(String createTableSql) throws Exception { + // Parse the stmt to extract the table name. We do this first to ensure + // that we do not execute arbitrary SQL here and pollute the test setup. + StatementBase result = Parser.parse(createTableSql); + if (!(result instanceof CreateTableStmt)) { + throw new Exception("Given stmt is not a CREATE TABLE stmt: " + createTableSql); + } + + // Execute the stmt. + Statement stmt = con_.createStatement(); + try { + stmt.execute(createTableSql); + } finally { + stmt.close(); + } + + // Once the stmt was executed successfully, add the fully-qualified table name + // for cleanup in @After. + CreateTableStmt parsedStmt = (CreateTableStmt) result; + testTableNames_.add(parsedStmt.getTblName().toString()); + } + + protected void dropTestTable(String tableName) throws SQLException { + Statement stmt = con_.createStatement(); + try { + stmt.execute("DROP TABLE " + tableName); + } finally { + stmt.close(); + } + } + + @After + public void testCleanUp() throws SQLException { + for (String tableName : testTableNames_) { + dropTestTable(tableName); + } + } +} diff --git a/fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java b/fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java index 1d34ef0..32b3e77 100644 --- a/fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java +++ b/fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java @@ -58,6 +58,8 @@ public class ImpalaJdbcClient { // As of Hive 0.11 'noSasl' is case sensitive. See HIVE-4232 for more details. private final static String NOSASL_AUTH_SPEC = ";auth=noSasl"; + private final static String LDAP_AUTH_SPEC = ";user=%s;password=%s"; + // The default connection string connects to localhost at the default hs2_port without // Sasl. private final static String DEFAULT_CONNECTION_STRING = @@ -137,14 +139,25 @@ public class ImpalaJdbcClient { } public static ImpalaJdbcClient createClientUsingHiveJdbcDriver() { - return new ImpalaJdbcClient( - HIVE_SERVER2_DRIVER_NAME, DEFAULT_CONNECTION_STRING + NOSASL_AUTH_SPEC); + return new ImpalaJdbcClient(HIVE_SERVER2_DRIVER_NAME, getNoAuthConnectionStr()); } public static ImpalaJdbcClient createClientUsingHiveJdbcDriver(String connString) { return new ImpalaJdbcClient(HIVE_SERVER2_DRIVER_NAME, connString); } + public static String getNoAuthConnectionStr() { + return getConnectionStr(NOSASL_AUTH_SPEC); + } + + public static String getLdapConnectionStr(String username, String password) { + return getConnectionStr(String.format(LDAP_AUTH_SPEC, username, password)); + } + + private static String getConnectionStr(String authStr) { + return DEFAULT_CONNECTION_STRING + authStr; + } + /** * Used to store the execution options passed via command line */ diff --git a/fe/src/test/resources/users.ldif b/fe/src/test/resources/users.ldif new file mode 100644 index 0000000..93ced04 --- /dev/null +++ b/fe/src/test/resources/users.ldif @@ -0,0 +1,25 @@ +version: 1 +dn: dc=myorg,dc=com +objectClass: domain +objectClass: top +dc: myorg + +dn: ou=Users,dc=myorg,dc=com +objectClass: organizationalUnit +objectClass: top +ou: Users + +dn: ou=Groups,dc=myorg,dc=com +objectClass: organizationalUnit +objectClass: top +ou: Groups + +dn: cn=Test1Ldap,ou=Users,dc=myorg,dc=com +objectClass: inetOrgPerson +objectClass: organizationalPerson +objectClass: person +objectClass: top +cn: Test1Ldap +sn: Ldap +uid: ldaptest1 +userPassword: 12345 \ No newline at end of file