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 9105c5c5427dd0479caf280b5754b31845df5fc2 Author: Fredy Wijaya <fwij...@cloudera.com> AuthorDate: Fri Jan 18 09:38:23 2019 -0800 IMPALA-8073: Fix FE tests that leak HMS connections This patch fixes FE tests that leak the HMS connections. I was to reproduce the issue by in SentryTestProxy by looping the test multiple times. Testing: - Ran SentryProxyTest in a loop without any issue. - Ran all FE tests Change-Id: I56930bc5f7a7bda4db11d4447461f907b1017b91 Reviewed-on: http://gerrit.cloudera.org:8080/12241 Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> --- .../java/org/apache/impala/catalog/Catalog.java | 4 +- .../org/apache/impala/analysis/AuditingTest.java | 19 +-- .../apache/impala/analysis/AuthorizationTest.java | 149 +++++++++++---------- .../impala/analysis/StmtMetadataLoaderTest.java | 26 ++-- .../catalog/CatalogObjectToFromThriftTest.java | 1 - .../org/apache/impala/catalog/CatalogTest.java | 4 + .../catalog/CatalogdTableInvalidatorTest.java | 6 +- .../impala/catalog/PartialCatalogInfoTest.java | 4 + .../org/apache/impala/common/FrontendTestBase.java | 1 + .../apache/impala/testutil/BlockIdGenerator.java | 53 ++++---- .../apache/impala/testutil/ImpaladTestCatalog.java | 6 + .../org/apache/impala/util/SentryProxyTest.java | 131 +++++++++--------- 12 files changed, 215 insertions(+), 189 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/catalog/Catalog.java b/fe/src/main/java/org/apache/impala/catalog/Catalog.java index 1f46882..7a3aa93 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Catalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/Catalog.java @@ -57,7 +57,7 @@ import com.google.common.base.Preconditions; * database that the user cannot modify. * Builtins are populated on startup in initBuiltins(). */ -public abstract class Catalog { +public abstract class Catalog implements AutoCloseable { // Initial catalog version and ID. public final static long INITIAL_CATALOG_VERSION = 0L; public static final TUniqueId INITIAL_CATALOG_SERVICE_ID = new TUniqueId(0L, 0L); @@ -322,9 +322,9 @@ public abstract class Catalog { * Release the Hive Meta Store Client resources. Can be called multiple times * (additional calls will be no-ops). */ + @Override public void close() { metaStoreClientPool_.close(); } - /** * Returns a managed meta store client from the client connection pool. */ 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 836020b..484ae8e 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java @@ -370,15 +370,16 @@ public class AuditingTest extends FrontendTestBase { // an AuthorizationError AuthorizationConfig config = AuthorizationConfig.createHadoopGroupAuthConfig( "server1", "/does/not/exist", ""); - ImpaladCatalog catalog = new ImpaladTestCatalog(config); - Frontend fe = new Frontend(config, catalog); - AnalysisContext analysisCtx = createAnalysisCtx(config); - // We should get an audit event even when an authorization failure occurs. - try { - parseAndAnalyze("create table foo_does_not_exist(i int)", analysisCtx, fe); - Assert.fail("Expected AuthorizationException"); - } catch (AuthorizationException e) { - Assert.assertEquals(1, analysisCtx.getAnalyzer().getAccessEvents().size()); + try (ImpaladCatalog catalog = new ImpaladTestCatalog(config)) { + Frontend fe = new Frontend(config, catalog); + AnalysisContext analysisCtx = createAnalysisCtx(config); + // We should get an audit event even when an authorization failure occurs. + try { + parseAndAnalyze("create table foo_does_not_exist(i int)", analysisCtx, fe); + Assert.fail("Expected AuthorizationException"); + } catch (AuthorizationException e) { + Assert.assertEquals(1, analysisCtx.getAnalyzer().getAccessEvents().size()); + } } } 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 7e1214e..9de90d6 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java @@ -842,58 +842,59 @@ public class AuthorizationTest extends FrontendTestBase { AuthorizationConfig authzConfig = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE, "", LocalGroupResourceAuthorizationProvider.class.getName()); - ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig); - - // This test relies on the auth_to_local rule - - // "RULE:[2:$1@$0](autht...@realm.com)s/(.*)@REALM.COM/auth_to_local_user/" - // which converts any principal of type authtest/<hostname>@REALM.COM to user - // auth_to_local_user. We have a sentry privilege in place that grants 'SELECT' - // privilege on tpcds.* to user auth_to_local_user. To test the integration, we try - // running the query with authtest/hostn...@realm.com user which is converted to user - // 'auth_to_local_user' and the authz should pass. - User.setRulesForTesting( - new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, "DEFAULT")); - User user = new User("authtest/hostn...@realm.com"); - AnalysisContext ctx = createAnalysisCtx(authzConfig, user.getName()); - Frontend fe = new Frontend(authzConfig, catalog); - - // Can select from table that user has privileges on. - AuthzOk(fe, ctx, "select * from tpcds.customer"); - // Does not have privileges to execute a query - AuthzError(fe, ctx, "select * from functional.alltypes", - "User '%s' does not have privileges to execute 'SELECT' on: functional.alltypes"); - - // Unit tests for User#getShortName() - // Different auth_to_local rules to apply on the username. - List<String> rules = Lists.newArrayList( - // Expects user@REALM and returns 'usera' (appends a in the end) - "RULE:[1:$1@$0](.*@REALM.COM)s/(.*)@REALM.COM/$1a/g", - // Same as above but expects user/host@REALM - "RULE:[2:$1@$0](.*@REALM.COM)s/(.*)@REALM.COM/$1a/g"); - - // Map from the user to the expected getShortName() output after applying - // the corresponding rule from 'rules' list. - List<Map.Entry<User, String>> users = Lists.newArrayList( - Maps.immutableEntry(new User(USER.getName() + "@REALM.COM"), USER.getName() - + "a"), - Maps.immutableEntry(new User(USER.getName() + "/abc.host....@realm.com"), - USER.getName() + "a")); - - for (int idx = 0; idx < users.size(); ++idx) { - Map.Entry<User, String> userObj = users.get(idx); - assertEquals(userObj.getKey().getShortNameForTesting(rules.get(idx)), - userObj.getValue()); - } + try (ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig)) { + // This test relies on the auth_to_local rule - + // "RULE:[2:$1@$0](autht...@realm.com)s/(.*)@REALM.COM/auth_to_local_user/" + // which converts any principal of type authtest/<hostname>@REALM.COM to user + // auth_to_local_user. We have a sentry privilege in place that grants 'SELECT' + // privilege on tpcds.* to user auth_to_local_user. To test the integration, we try + // running the query with authtest/hostn...@realm.com user which is converted to + // user 'auth_to_local_user' and the authz should pass. + User.setRulesForTesting( + new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, "DEFAULT")); + User user = new User("authtest/hostn...@realm.com"); + AnalysisContext ctx = createAnalysisCtx(authzConfig, user.getName()); + Frontend fe = new Frontend(authzConfig, catalog); - // Test malformed rules. RuleParser throws an IllegalArgumentException. - String malformedRule = "{((()"; - try { - user.getShortNameForTesting(malformedRule); - } catch (IllegalArgumentException e) { - Assert.assertTrue(e.getMessage().contains("Invalid rule")); - return; + // Can select from table that user has privileges on. + AuthzOk(fe, ctx, "select * from tpcds.customer"); + // Does not have privileges to execute a query + AuthzError(fe, ctx, "select * from functional.alltypes", + "User '%s' does not have privileges to execute 'SELECT' on: " + + "functional.alltypes"); + + // Unit tests for User#getShortName() + // Different auth_to_local rules to apply on the username. + List<String> rules = Lists.newArrayList( + // Expects user@REALM and returns 'usera' (appends a in the end) + "RULE:[1:$1@$0](.*@REALM.COM)s/(.*)@REALM.COM/$1a/g", + // Same as above but expects user/host@REALM + "RULE:[2:$1@$0](.*@REALM.COM)s/(.*)@REALM.COM/$1a/g"); + + // Map from the user to the expected getShortName() output after applying + // the corresponding rule from 'rules' list. + List<Map.Entry<User, String>> users = Lists.newArrayList( + Maps.immutableEntry(new User(USER.getName() + "@REALM.COM"), USER.getName() + + "a"), + Maps.immutableEntry(new User(USER.getName() + "/abc.host....@realm.com"), + USER.getName() + "a")); + + for (int idx = 0; idx < users.size(); ++idx) { + Map.Entry<User, String> userObj = users.get(idx); + assertEquals(userObj.getKey().getShortNameForTesting(rules.get(idx)), + userObj.getValue()); + } + + // Test malformed rules. RuleParser throws an IllegalArgumentException. + String malformedRule = "{((()"; + try { + user.getShortNameForTesting(malformedRule); + } catch (IllegalArgumentException e) { + Assert.assertTrue(e.getMessage().contains("Invalid rule")); + return; + } + Assert.fail("No exception caught while using getShortName() on a malformed rule"); } - Assert.fail("No exception caught while using getShortName() on a malformed rule"); } @Test @@ -1027,30 +1028,32 @@ public class AuthorizationTest extends FrontendTestBase { AuthorizationConfig authzConfig = new AuthorizationConfig("server1", AUTHZ_POLICY_FILE, "", LocalGroupResourceAuthorizationProvider.class.getName()); - ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig); + try (ImpaladCatalog catalog = new ImpaladTestCatalog(authzConfig)) { + // Create an analysis context + FE with the test user + // (as defined in the policy file) + User user = new User("test_user"); + AnalysisContext ctx = createAnalysisCtx(authzConfig, user.getName()); + Frontend fe = new Frontend(authzConfig, catalog); - // Create an analysis context + FE with the test user (as defined in the policy file) - User user = new User("test_user"); - AnalysisContext ctx = createAnalysisCtx(authzConfig, user.getName()); - Frontend fe = new Frontend(authzConfig, catalog); - - // Can select from table that user has privileges on. - AuthzOk(fe, ctx, "select * from functional.alltypesagg"); - // Does not have privileges to execute a query - AuthzError(fe, ctx, "select * from functional.alltypes", - "User '%s' does not have privileges to execute 'SELECT' on: functional.alltypes"); - - // Verify with the admin user - user = new User("admin_user"); - ctx = createAnalysisCtx(authzConfig, user.getName()); - fe = new Frontend(authzConfig, catalog); - - // Admin user should have privileges to do anything - AuthzOk(fe, ctx, "select * from functional.alltypesagg"); - AuthzOk(fe, ctx, "select * from functional.alltypes"); - AuthzOk(fe, ctx, "invalidate metadata"); - AuthzOk(fe, ctx, "create external table tpch.kudu_tbl stored as kudu " + - "TBLPROPERTIES ('kudu.master_addresses'='127.0.0.1', 'kudu.table_name'='tbl')"); + // Can select from table that user has privileges on. + AuthzOk(fe, ctx, "select * from functional.alltypesagg"); + // Does not have privileges to execute a query + AuthzError(fe, ctx, "select * from functional.alltypes", + "User '%s' does not have privileges to execute 'SELECT' on: " + + "functional.alltypes"); + + // Verify with the admin user + user = new User("admin_user"); + ctx = createAnalysisCtx(authzConfig, user.getName()); + fe = new Frontend(authzConfig, catalog); + + // Admin user should have privileges to do anything + AuthzOk(fe, ctx, "select * from functional.alltypesagg"); + AuthzOk(fe, ctx, "select * from functional.alltypes"); + AuthzOk(fe, ctx, "invalidate metadata"); + AuthzOk(fe, ctx, "create external table tpch.kudu_tbl stored as kudu " + + "TBLPROPERTIES ('kudu.master_addresses'='127.0.0.1', 'kudu.table_name'='tbl')"); + } } private void TestWithIncorrectConfig(AuthorizationConfig authzConfig, User user) diff --git a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java index 9ef923a..5f33683 100644 --- a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java @@ -38,21 +38,23 @@ public class StmtMetadataLoaderTest { int expectedNumLoadRequests, int expectedNumCatalogUpdates, String[] expectedDbs, String[] expectedTables) throws ImpalaException { - ImpaladTestCatalog catalog = new ImpaladTestCatalog(); - Frontend fe = new Frontend(AuthorizationConfig.createAuthDisabledConfig(), catalog); - StatementBase stmt = Parser.parse(stmtStr); - // Catalog is fresh and no tables are cached. - validateUncached(stmt, fe, expectedNumLoadRequests, expectedNumCatalogUpdates, - expectedDbs, expectedTables); - // All relevant tables should be cached now. - validateCached(stmt, fe, expectedDbs, expectedTables); + try (ImpaladTestCatalog catalog = new ImpaladTestCatalog()) { + Frontend fe = new Frontend(AuthorizationConfig.createAuthDisabledConfig(), catalog); + StatementBase stmt = Parser.parse(stmtStr); + // Catalog is fresh and no tables are cached. + validateUncached(stmt, fe, expectedNumLoadRequests, expectedNumCatalogUpdates, + expectedDbs, expectedTables); + // All relevant tables should be cached now. + validateCached(stmt, fe, expectedDbs, expectedTables); + } } private void testNoLoad(String stmtStr) throws ImpalaException { - ImpaladTestCatalog catalog = new ImpaladTestCatalog(); - Frontend fe = new Frontend(AuthorizationConfig.createAuthDisabledConfig(), catalog); - StatementBase stmt = Parser.parse(stmtStr); - validateCached(stmt, fe, new String[]{}, new String[]{}); + try (ImpaladTestCatalog catalog = new ImpaladTestCatalog()) { + Frontend fe = new Frontend(AuthorizationConfig.createAuthDisabledConfig(), catalog); + StatementBase stmt = Parser.parse(stmtStr); + validateCached(stmt, fe, new String[]{}, new String[]{}); + } } private void validateDbs(StmtTableCache stmtTableCache, String[] expectedDbs) { diff --git a/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java b/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java index 0747464..9762a3e 100644 --- a/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java +++ b/fe/src/test/java/org/apache/impala/catalog/CatalogObjectToFromThriftTest.java @@ -24,7 +24,6 @@ import java.util.Collection; import java.util.Map; import org.apache.impala.analysis.LiteralExpr; -import org.apache.impala.common.AnalysisException; import org.apache.impala.common.ImpalaException; import org.apache.impala.common.SqlCastException; import org.apache.impala.testutil.CatalogServiceTestCatalog; diff --git a/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java b/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java index 0946b38..bac5864 100644 --- a/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java +++ b/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java @@ -64,6 +64,7 @@ import org.apache.impala.thrift.TPrivilege; import org.apache.impala.thrift.TPrivilegeLevel; import org.apache.impala.thrift.TPrivilegeScope; import org.apache.impala.thrift.TTableName; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -80,6 +81,9 @@ public class CatalogTest { catalog_ = CatalogServiceTestCatalog.create(); } + @After + public void cleanUp() { catalog_.close(); } + public static void checkTableCols(FeDb db, String tblName, int numClusteringCols, String[] colNames, Type[] colTypes) throws TableLoadingException { FeTable tbl = db.getTable(tblName); diff --git a/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java b/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java index 69e99e3..4410ea5 100644 --- a/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java +++ b/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java @@ -22,6 +22,7 @@ import org.apache.impala.common.Reference; import org.apache.impala.testutil.CatalogServiceTestCatalog; import org.apache.impala.thrift.TTableName; import org.junit.After; +import org.junit.AfterClass; import org.junit.Assert; import org.junit.Test; @@ -31,7 +32,10 @@ import static java.lang.Thread.sleep; public class CatalogdTableInvalidatorTest { - private CatalogServiceCatalog catalog_ = CatalogServiceTestCatalog.create(); + private static CatalogServiceCatalog catalog_ = CatalogServiceTestCatalog.create(); + + @AfterClass + public static void tearDown() { catalog_.close(); } private long waitForTrigger(long previousTriggerCount) throws InterruptedException { long triggerCount; diff --git a/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java b/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java index 1b5e288..657509a 100644 --- a/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java +++ b/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java @@ -49,6 +49,7 @@ import org.apache.impala.thrift.TTableInfoSelector; import org.apache.thrift.TDeserializer; import org.apache.thrift.TException; import org.apache.thrift.TSerializer; +import org.junit.AfterClass; import org.junit.Test; import com.google.common.base.Preconditions; @@ -58,6 +59,9 @@ public class PartialCatalogInfoTest { private static CatalogServiceCatalog catalog_ = CatalogServiceTestCatalog.create(); + @AfterClass + public static void cleanUp() { catalog_.close(); } + /** * A Callable wrapper around getPartialCatalogObject() call. */ 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 0ea33a7..85ecc4c 100644 --- a/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java +++ b/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java @@ -97,6 +97,7 @@ public class FrontendTestBase { @AfterClass public static void cleanUp() throws Exception { RuntimeEnv.INSTANCE.reset(); + catalog_.close(); } // Adds a Udf: default.name(args) to the catalog. diff --git a/fe/src/test/java/org/apache/impala/testutil/BlockIdGenerator.java b/fe/src/test/java/org/apache/impala/testutil/BlockIdGenerator.java index f04ce4a..ef03d67 100644 --- a/fe/src/test/java/org/apache/impala/testutil/BlockIdGenerator.java +++ b/fe/src/test/java/org/apache/impala/testutil/BlockIdGenerator.java @@ -61,38 +61,39 @@ public class BlockIdGenerator { writer = new FileWriter(output); // Load all tables in the catalog - Catalog catalog = CatalogServiceTestCatalog.create(); - for (FeDb database: catalog.getDbs(PatternMatcher.MATCHER_MATCH_ALL)) { - for (String tableName: database.getAllTableNames()) { - FeTable table = database.getTable(tableName); - // Only do this for hdfs tables - if (table == null || !(table instanceof HdfsTable)) { - continue; - } - HdfsTable hdfsTable = (HdfsTable)table; + try (Catalog catalog = CatalogServiceTestCatalog.create()) { + for (FeDb database : catalog.getDbs(PatternMatcher.MATCHER_MATCH_ALL)) { + for (String tableName : database.getAllTableNames()) { + FeTable table = database.getTable(tableName); + // Only do this for hdfs tables + if (table == null || !(table instanceof HdfsTable)) { + continue; + } + HdfsTable hdfsTable = (HdfsTable) table; - // Write the output as <tablename>: <blockid1> <blockid2> <etc> - writer.write(tableName + ":"); - Collection<? extends FeFsPartition> parts = - FeCatalogUtils.loadAllPartitions(hdfsTable); - for (FeFsPartition partition: parts) { - List<FileDescriptor> fileDescriptors = partition.getFileDescriptors(); - for (FileDescriptor fd : fileDescriptors) { - Path p = new Path(partition.getLocation(), fd.getFileName()); + // Write the output as <tablename>: <blockid1> <blockid2> <etc> + writer.write(tableName + ":"); + Collection<? extends FeFsPartition> parts = + FeCatalogUtils.loadAllPartitions(hdfsTable); + for (FeFsPartition partition : parts) { + List<FileDescriptor> fileDescriptors = partition.getFileDescriptors(); + for (FileDescriptor fd : fileDescriptors) { + Path p = new Path(partition.getLocation(), fd.getFileName()); - // Use a deprecated API to get block ids - DistributedFileSystem dfs = - (DistributedFileSystem)p.getFileSystem(hdfsConfig); - LocatedBlocks locations = dfs.getClient().getNamenode().getBlockLocations( - p.toUri().getPath(), 0, fd.getFileLength()); + // Use a deprecated API to get block ids + DistributedFileSystem dfs = + (DistributedFileSystem) p.getFileSystem(hdfsConfig); + LocatedBlocks locations = dfs.getClient().getNamenode().getBlockLocations( + p.toUri().getPath(), 0, fd.getFileLength()); - for (LocatedBlock lb : locations.getLocatedBlocks()) { - long id = lb.getBlock().getBlockId(); - writer.write(" " + id); + for (LocatedBlock lb : locations.getLocatedBlocks()) { + long id = lb.getBlock().getBlockId(); + writer.write(" " + id); + } } } + writer.write("\n"); } - writer.write("\n"); } } } finally { diff --git a/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java b/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java index 60bbaeb..3395c54 100644 --- a/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java +++ b/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java @@ -147,4 +147,10 @@ public class ImpaladTestCatalog extends ImpaladCatalog { } public void removeUser(String userName) { srcCatalog_.removeUser(userName); } + + @Override + public void close() { + super.close(); + srcCatalog_.close(); + } } 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 d3246ab..7ab8e56 100644 --- a/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java +++ b/fe/src/test/java/org/apache/impala/util/SentryProxyTest.java @@ -274,41 +274,41 @@ public class SentryProxyTest { String mixedCaseRoleName = lowerCaseRoleName.substring(0, 1).toUpperCase() + lowerCaseRoleName.substring(1); - CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( - authzConfig_.getSentryConfig()); - - addSentryRolePrivileges(sentryService_, lowerCaseRoleName, "functional"); - CatalogState noReset = refreshSentryAuthorization(catalog, sentryService_, false); - - // Impala stores the role name in case insensitive way. - for (String roleName: new String[]{ - lowerCaseRoleName, upperCaseRoleName, mixedCaseRoleName}) { - Role role = catalog.getAuthPolicy().getRole(roleName); - assertEquals(lowerCaseRoleName, role.getName()); - assertEquals(1, role.getPrivileges().size()); - assertNotNull(role.getPrivilege( - "server=server1->db=functional->grantoption=false")); - } + try (CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( + authzConfig_.getSentryConfig())) { + addSentryRolePrivileges(sentryService_, lowerCaseRoleName, "functional"); + CatalogState noReset = refreshSentryAuthorization(catalog, sentryService_, false); + + // Impala stores the role name in case insensitive way. + for (String roleName : new String[]{ + lowerCaseRoleName, upperCaseRoleName, mixedCaseRoleName}) { + Role role = catalog.getAuthPolicy().getRole(roleName); + assertEquals(lowerCaseRoleName, role.getName()); + assertEquals(1, role.getPrivileges().size()); + assertNotNull(role.getPrivilege( + "server=server1->db=functional->grantoption=false")); + } - try { - sentryService_.createRole(USER, upperCaseRoleName, false); - fail("Exception should be thrown when creating a duplicate role name."); - } catch (Exception e) { - assertTrue(e.getMessage().startsWith("Error creating role")); - } + try { + sentryService_.createRole(USER, upperCaseRoleName, false); + fail("Exception should be thrown when creating a duplicate role name."); + } catch (Exception e) { + assertTrue(e.getMessage().startsWith("Error creating role")); + } - // No new role should be added. - for (String roleName: new String[]{ - lowerCaseRoleName, upperCaseRoleName, mixedCaseRoleName}) { - Role role = catalog.getAuthPolicy().getRole(roleName); - assertEquals(lowerCaseRoleName, role.getName()); - assertEquals(1, role.getPrivileges().size()); - assertNotNull(role.getPrivilege( - "server=server1->db=functional->grantoption=false")); - } + // No new role should be added. + for (String roleName : new String[]{ + lowerCaseRoleName, upperCaseRoleName, mixedCaseRoleName}) { + Role role = catalog.getAuthPolicy().getRole(roleName); + assertEquals(lowerCaseRoleName, role.getName()); + assertEquals(1, role.getPrivileges().size()); + assertNotNull(role.getPrivilege( + "server=server1->db=functional->grantoption=false")); + } - CatalogState reset = refreshSentryAuthorization(catalog, sentryService_, true); - checkCatalogState(noReset, reset); + CatalogState reset = refreshSentryAuthorization(catalog, sentryService_, true); + checkCatalogState(noReset, reset); + } } @Test @@ -319,34 +319,34 @@ public class SentryProxyTest { String mixedCaseUserName = lowerCaseUserName.substring(0, 1).toUpperCase() + lowerCaseUserName.substring(1); - CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( - authzConfig_.getSentryConfig()); - - SentryPolicyServiceStub sentryService = createSentryPolicyServiceStub( - authzConfig_.getSentryConfig()); - // We grant different privileges to different users to ensure each user is - // granted with a distinct privilege. - addSentryUserPrivileges(sentryService, lowerCaseUserName, "functional"); - addSentryUserPrivileges(sentryService, upperCaseUserName, "functional_kudu"); - addSentryUserPrivileges(sentryService, mixedCaseUserName, "functional_parquet"); - - CatalogState noReset = refreshSentryAuthorization(catalog, sentryService, false); - - // Impala stores the user name in case sensitive way. - for (Pair<String, String> userPrivilege: new Pair[]{ - new Pair(lowerCaseUserName, "server=server1->db=functional->grantoption=false"), - new Pair(upperCaseUserName, "server=server1->db=functional_kudu" + - "->grantoption=false"), - new Pair(mixedCaseUserName, "server=server1->db=functional_parquet" + - "->grantoption=false")}) { - User user = catalog.getAuthPolicy().getUser(userPrivilege.first); - assertEquals(userPrivilege.first, user.getName()); - assertEquals(1, user.getPrivileges().size()); - assertNotNull(user.getPrivilege(userPrivilege.second)); - } + try (CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( + authzConfig_.getSentryConfig())) { + SentryPolicyServiceStub sentryService = createSentryPolicyServiceStub( + authzConfig_.getSentryConfig()); + // We grant different privileges to different users to ensure each user is + // granted with a distinct privilege. + addSentryUserPrivileges(sentryService, lowerCaseUserName, "functional"); + addSentryUserPrivileges(sentryService, upperCaseUserName, "functional_kudu"); + addSentryUserPrivileges(sentryService, mixedCaseUserName, "functional_parquet"); + + CatalogState noReset = refreshSentryAuthorization(catalog, sentryService, false); + + // Impala stores the user name in case sensitive way. + for (Pair<String, String> userPrivilege : new Pair[]{ + new Pair(lowerCaseUserName, "server=server1->db=functional->grantoption=false"), + new Pair(upperCaseUserName, "server=server1->db=functional_kudu" + + "->grantoption=false"), + new Pair(mixedCaseUserName, "server=server1->db=functional_parquet" + + "->grantoption=false")}) { + User user = catalog.getAuthPolicy().getUser(userPrivilege.first); + assertEquals(userPrivilege.first, user.getName()); + assertEquals(1, user.getPrivileges().size()); + assertNotNull(user.getPrivilege(userPrivilege.second)); + } - CatalogState reset = refreshSentryAuthorization(catalog, sentryService, true); - checkCatalogState(noReset, reset); + CatalogState reset = refreshSentryAuthorization(catalog, sentryService, true); + checkCatalogState(noReset, reset); + } } private static void addCatalogPrincipalPrivileges(TPrincipalType type, @@ -601,13 +601,14 @@ public class SentryProxyTest { private void withAllPrincipalTypes(Consumer<SentryProxyTestContext> consumer) { for (TPrincipalType type: TPrincipalType.values()) { - CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( - authzConfig_.getSentryConfig()); - SentryPolicyService sentryService = sentryService_; - if (type == TPrincipalType.USER) { - sentryService = createSentryPolicyServiceStub(authzConfig_.getSentryConfig()); + try (CatalogServiceCatalog catalog = CatalogServiceTestCatalog.createWithAuth( + authzConfig_.getSentryConfig())) { + SentryPolicyService sentryService = sentryService_; + if (type == TPrincipalType.USER) { + sentryService = createSentryPolicyServiceStub(authzConfig_.getSentryConfig()); + } + consumer.accept(new SentryProxyTestContext(type, catalog, sentryService)); } - consumer.accept(new SentryProxyTestContext(type, catalog, sentryService)); } } } \ No newline at end of file