This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 3db3b3b0a74d003ef497cd76d3bd66480f7dc4d7
Author: Fredy Wijaya <fwij...@cloudera.com>
AuthorDate: Fri Feb 1 11:14:12 2019 -0800
IMPALA-8150: Fix buggy AuditingTest::TestAccessEventsOnAuthFailure
I was able to reproduce this issue by running AuditingTest individually.
Running all tests did not seem to reproduce this issue, which may be due
to the test depends on the state from other tests. Looking at the code,
the test was poorly written mainly because of two reasons:
- The Sentry config was set to an empty string, which is not a valid
config file.
- Calling AuthorizationConfig.validateConfig() was missing.
This patch ensures validateConfig() is always called when
AuthorizationConfig instance is created.
Testing:
- Ran AuditingTest individually
- Ran all FE tests
- Ran all E2E authorization tests
Change-Id: I712697e6d5a3e171b259a781bd07de9871a29b95
Reviewed-on: http://gerrit.cloudera.org:8080/12334
Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
---
.../impala/authorization/AuthorizationConfig.java | 3 +-
.../org/apache/impala/service/JniFrontend.java | 1 -
.../org/apache/impala/analysis/AuditingTest.java | 6 +-
.../apache/impala/analysis/AuthorizationTest.java | 88 +++++++++++-----------
.../org/apache/impala/common/FrontendTestBase.java | 1 -
.../org/apache/impala/util/SentryProxyTest.java | 1 -
6 files changed, 47 insertions(+), 53 deletions(-)
diff --git
a/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
b/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
index 80611d1..96b65d8 100644
--- a/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
+++ b/fe/src/main/java/org/apache/impala/authorization/AuthorizationConfig.java
@@ -51,6 +51,7 @@ public class AuthorizationConfig {
policyProviderClassName = policyProviderClassName.trim();
}
policyProviderClassName_ = policyProviderClassName;
+ validateConfig();
}
/**
@@ -74,7 +75,7 @@ public class AuthorizationConfig {
* Validates the authorization configuration and throws an
AuthorizationException
* if any problems are found. If authorization is disabled, config checks
are skipped.
*/
- public void validateConfig() throws IllegalArgumentException {
+ private void validateConfig() throws IllegalArgumentException {
// If authorization is not enabled, config checks are skipped.
if (!isEnabled()) return;
diff --git a/fe/src/main/java/org/apache/impala/service/JniFrontend.java
b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
index d9f3f06..0cf5390 100644
--- a/fe/src/main/java/org/apache/impala/service/JniFrontend.java
+++ b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
@@ -142,7 +142,6 @@ public class JniFrontend {
AuthorizationConfig authConfig = new AuthorizationConfig(cfg.server_name,
cfg.authorization_policy_file, cfg.sentry_config,
cfg.authorization_policy_provider_class);
- authConfig.validateConfig();
if (authConfig.isEnabled()) {
LOG.info(String.format("Authorization is 'ENABLED' using %s",
authConfig.isFileBasedPolicy() ? " file based policy from: " +
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
index 484ae8e..680efa9 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
@@ -364,12 +364,12 @@ public class AuditingTest extends FrontendTestBase {
}
@Test
- public void TestAccessEventsOnAuthFailure() throws AuthorizationException,
- ImpalaException {
+ public void TestAccessEventsOnAuthFailure() throws ImpalaException {
// The policy file doesn't exist so all operations will result in
// an AuthorizationError
AuthorizationConfig config =
AuthorizationConfig.createHadoopGroupAuthConfig(
- "server1", "/does/not/exist", "");
+ "server1", "/does/not/exist", System.getenv("IMPALA_HOME") +
+ "/fe/src/test/resources/sentry-site.xml");
try (ImpaladCatalog catalog = new ImpaladTestCatalog(config)) {
Frontend fe = new Frontend(config, catalog);
AnalysisContext analysisCtx = createAnalysisCtx(config);
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
index ccdc8c4..15a06ee 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
@@ -141,7 +141,6 @@ public class AuthorizationTest extends FrontendTestBase {
testCtxs_ = new ArrayList<>();
// Create and init file based auth config.
AuthorizationConfig filePolicyAuthzConfig = createPolicyFileAuthzConfig();
- filePolicyAuthzConfig.validateConfig();
ImpaladTestCatalog filePolicyCatalog = new
ImpaladTestCatalog(filePolicyAuthzConfig);
testCtxs_.add(new TestContext(filePolicyAuthzConfig, filePolicyCatalog));
@@ -180,7 +179,6 @@ public class AuthorizationTest extends FrontendTestBase {
AuthorizationConfig result =
AuthorizationConfig.createHadoopGroupAuthConfig("server1",
AUTHZ_POLICY_FILE,
System.getenv("IMPALA_HOME") +
"/fe/src/test/resources/sentry-site.xml");
- result.validateConfig();
return result;
}
@@ -841,7 +839,7 @@ public class AuthorizationTest extends FrontendTestBase {
// </value>
// </property>
AuthorizationConfig authzConfig = new AuthorizationConfig("server1",
- AUTHZ_POLICY_FILE, "",
+ AUTHZ_POLICY_FILE, ctx_.authzConfig.getSentryConfig().getConfigFile(),
LocalGroupResourceAuthorizationProvider.class.getName());
try (ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig)) {
// This test relies on the auth_to_local rule -
@@ -903,7 +901,8 @@ public class AuthorizationTest extends FrontendTestBase {
if (ctx_.authzConfig.isFileBasedPolicy()) {
// Authorization config that has a different server name from policy
file.
TestWithIncorrectConfig(AuthorizationConfig.createHadoopGroupAuthConfig(
- "differentServerName", AUTHZ_POLICY_FILE, ""),
+ "differentServerName", AUTHZ_POLICY_FILE,
+ ctx_.authzConfig.getSentryConfig().getConfigFile()),
new User(System.getProperty("user.name")));
} // TODO: Test using policy server.
}
@@ -917,7 +916,8 @@ public class AuthorizationTest extends FrontendTestBase {
// Use a HadoopGroupProvider in this case so the user -> group mappings
can still be
// resolved in the absence of the policy file.
TestWithIncorrectConfig(AuthorizationConfig.createHadoopGroupAuthConfig("server1",
- AUTHZ_POLICY_FILE + "_does_not_exist", ""),
+ AUTHZ_POLICY_FILE + "_does_not_exist",
+ ctx_.authzConfig.getSentryConfig().getConfigFile()),
new User(System.getProperty("user.name")));
}
@@ -927,87 +927,83 @@ public class AuthorizationTest extends FrontendTestBase {
// Valid configs pass validation.
AuthorizationConfig config =
AuthorizationConfig.createHadoopGroupAuthConfig(
"server1", AUTHZ_POLICY_FILE, sentryConfig);
- config.validateConfig();
Assert.assertTrue(config.isEnabled());
Assert.assertTrue(config.isFileBasedPolicy());
config = AuthorizationConfig.createHadoopGroupAuthConfig("server1", null,
sentryConfig);
- config.validateConfig();
Assert.assertTrue(config.isEnabled());
Assert.assertTrue(!config.isFileBasedPolicy());
// Invalid configs
// No sentry configuration file.
- config = AuthorizationConfig.createHadoopGroupAuthConfig(
- "server1", AUTHZ_POLICY_FILE, null);
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
+ config = AuthorizationConfig.createHadoopGroupAuthConfig(
+ "server1", AUTHZ_POLICY_FILE, null);
+ Assert.assertTrue(config.isEnabled());
} catch (Exception e) {
- Assert.assertEquals(e.getMessage(), "A valid path to a sentry-site.xml
config " +
- "file must be set using --sentry_config to enable authorization.");
+ Assert.assertEquals("A valid path to a sentry-site.xml config " +
+ "file must be set using --sentry_config to enable authorization.",
+ e.getMessage());
}
// Empty / null server name.
- config = AuthorizationConfig.createHadoopGroupAuthConfig(
- "", AUTHZ_POLICY_FILE, sentryConfig);
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
+ config = AuthorizationConfig.createHadoopGroupAuthConfig(
+ "", AUTHZ_POLICY_FILE, sentryConfig);
+ Assert.assertTrue(config.isEnabled());
fail("Expected configuration to fail.");
} catch (IllegalArgumentException e) {
- Assert.assertEquals(e.getMessage(),
+ Assert.assertEquals(
"Authorization is enabled but the server name is null or empty. Set
the " +
- "server name using the impalad --server_name flag.");
+ "server name using the impalad --server_name flag.",
+ e.getMessage());
}
- config = AuthorizationConfig.createHadoopGroupAuthConfig(null,
AUTHZ_POLICY_FILE,
- sentryConfig);
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
+ config = AuthorizationConfig.createHadoopGroupAuthConfig(null,
AUTHZ_POLICY_FILE,
+ sentryConfig);
+ Assert.assertTrue(config.isEnabled());
fail("Expected configuration to fail.");
} catch (IllegalArgumentException e) {
- Assert.assertEquals(e.getMessage(),
+ Assert.assertEquals(
"Authorization is enabled but the server name is null or empty. Set
the " +
- "server name using the impalad --server_name flag.");
+ "server name using the impalad --server_name flag.",
+ e.getMessage());
}
// Sentry config file does not exist.
- config = AuthorizationConfig.createHadoopGroupAuthConfig("server1", "",
- "/path/does/not/exist.xml");
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
+ config = AuthorizationConfig.createHadoopGroupAuthConfig("server1", "",
+ "/path/does/not/exist.xml");
+ Assert.assertTrue(config.isEnabled());
fail("Expected configuration to fail.");
} catch (Exception e) {
- Assert.assertEquals(e.getMessage(),
- "Sentry configuration file does not exist:
\"/path/does/not/exist.xml\"");
+ Assert.assertEquals(
+ "Sentry configuration file does not exist:
\"/path/does/not/exist.xml\"",
+ e.getMessage());
}
// Invalid ResourcePolicyProvider class name.
- config = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE, "",
- "ClassDoesNotExist");
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
- fail("Expected configuration to fail.");
+ config = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE,
sentryConfig,
+ "ClassDoesNotExist");
+ Assert.assertTrue(config.isEnabled()); fail("Expected configuration
to fail.");
} catch (IllegalArgumentException e) {
- Assert.assertEquals(e.getMessage(),
- "The authorization policy provider class 'ClassDoesNotExist' was not
found.");
+ Assert.assertEquals(
+ "The authorization policy provider class 'ClassDoesNotExist' was not
found.",
+ e.getMessage());
}
// Valid class name, but class is not derived from ResourcePolicyProvider
- config = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE, "",
- this.getClass().getName());
- Assert.assertTrue(config.isEnabled());
try {
- config.validateConfig();
- fail("Expected configuration to fail.");
+ config = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE,
sentryConfig,
+ this.getClass().getName());
+ Assert.assertTrue(config.isEnabled()); fail("Expected configuration
to fail.");
} catch (IllegalArgumentException e) {
- Assert.assertEquals(e.getMessage(), String.format("The authorization
policy " +
+ Assert.assertEquals(String.format("The authorization policy " +
"provider class '%s' must be a subclass of '%s'.",
this.getClass().getName(),
- ResourceAuthorizationProvider.class.getName()));
+ ResourceAuthorizationProvider.class.getName()),
+ e.getMessage());
}
// Config validations skipped if authorization disabled
@@ -1027,7 +1023,7 @@ public class AuthorizationTest extends FrontendTestBase {
// Use an authorization configuration that uses the
// LocalGroupResourceAuthorizationProvider.
AuthorizationConfig authzConfig = new AuthorizationConfig("server1",
- AUTHZ_POLICY_FILE, "",
+ AUTHZ_POLICY_FILE, ctx_.authzConfig.getSentryConfig().getConfigFile(),
LocalGroupResourceAuthorizationProvider.class.getName());
try (ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig)) {
// Create an analysis context + FE with the test user
diff --git a/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
b/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
index 85ecc4c..2b6a640 100644
--- a/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
+++ b/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
@@ -451,7 +451,6 @@ public class FrontendTestBase {
AuthorizationConfig authzConfig =
AuthorizationConfig.createHadoopGroupAuthConfig(
"server1", null, System.getenv("IMPALA_HOME") +
"/fe/src/test/resources/sentry-site.xml");
- authzConfig.validateConfig();
return authzConfig;
}
}
diff --git a/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
b/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
index 7ab8e56..9d24f62 100644
--- a/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
+++ b/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java
@@ -61,7 +61,6 @@ public class SentryProxyTest {
authzConfig_ = AuthorizationConfig.createHadoopGroupAuthConfig(
SENTRY_SERVER, null, System.getenv("IMPALA_HOME") +
"/fe/src/test/resources/sentry-site.xml");
- authzConfig_.validateConfig();
sentryService_ = new SentryPolicyService(authzConfig_.getSentryConfig());
}
