Repository: incubator-sentry Updated Branches: refs/heads/master 1b6fe629c -> 2ae1befda
SENTRY-1008: Path should be not be updated if the create/drop table/partition event fails (Hao Hao via Lenni Kuff) Change-Id: I0b83686612ea10ea7c678f70c16ca975fc7c338e Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/2ae1befd Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/2ae1befd Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/2ae1befd Branch: refs/heads/master Commit: 2ae1befdad80a5d23f4efe25d29555d767c592d8 Parents: 1b6fe62 Author: Lenni Kuff <lsk...@cloudera.com> Authored: Thu Jan 21 12:40:52 2016 -0800 Committer: Lenni Kuff <lsk...@cloudera.com> Committed: Thu Jan 21 12:40:52 2016 -0800 ---------------------------------------------------------------------- .../SentryMetastorePostEventListener.java | 84 +++++-- .../tests/e2e/hdfs/TestHDFSIntegration.java | 232 +++++++++++++++++++ 2 files changed, 299 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/2ae1befd/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java ---------------------------------------------------------------------- diff --git a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java index a45d115..cb797af 100644 --- a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java +++ b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListener.java @@ -88,6 +88,14 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onCreateTable (CreateTableEvent tableEvent) throws MetaException { + + // don't sync paths/privileges if the operation has failed + if (!tableEvent.getStatus()) { + LOGGER.debug("Skip sync paths/privileges with Sentry server for onCreateTable event," + + " since the operation failed. \n"); + return; + } + if (tableEvent.getTable().getSd().getLocation() != null) { String authzObj = tableEvent.getTable().getDbName() + "." + tableEvent.getTable().getTableName(); @@ -96,21 +104,27 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { plugin.addPath(authzObj, path); } } + // drop the privileges on the given table, in case if anything was left // behind during the drop if (!syncWithPolicyStore(AuthzConfVars.AUTHZ_SYNC_CREATE_WITH_POLICY_STORE)) { return; } - // don't sync privileges if the operation has failed - if (!tableEvent.getStatus()) { - return; - } + dropSentryTablePrivilege(tableEvent.getTable().getDbName(), tableEvent.getTable().getTableName()); } @Override public void onDropTable(DropTableEvent tableEvent) throws MetaException { + + // don't sync paths/privileges if the operation has failed + if (!tableEvent.getStatus()) { + LOGGER.debug("Skip syncing paths/privileges with Sentry server for onDropTable event," + + " since the operation failed. \n"); + return; + } + if (tableEvent.getTable().getSd().getLocation() != null) { String authzObj = tableEvent.getTable().getDbName() + "." + tableEvent.getTable().getTableName(); @@ -122,10 +136,11 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { if (!syncWithPolicyStore(AuthzConfVars.AUTHZ_SYNC_DROP_WITH_POLICY_STORE)) { return; } - // don't sync privileges if the operation has failed + if (!tableEvent.getStatus()) { return; } + dropSentryTablePrivilege(tableEvent.getTable().getDbName(), tableEvent.getTable().getTableName()); } @@ -133,6 +148,14 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onCreateDatabase(CreateDatabaseEvent dbEvent) throws MetaException { + + // don't sync paths/privileges if the operation has failed + if (!dbEvent.getStatus()) { + LOGGER.debug("Skip syncing paths/privileges with Sentry server for onCreateDatabase event," + + " since the operation failed. \n"); + return; + } + if (dbEvent.getDatabase().getLocationUri() != null) { String authzObj = dbEvent.getDatabase().getName(); String path = dbEvent.getDatabase().getLocationUri(); @@ -140,25 +163,30 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { plugin.addPath(authzObj, path); } } - // drop the privileges on the database, incase anything left behind during + // drop the privileges on the database, in case anything left behind during // last drop db if (!syncWithPolicyStore(AuthzConfVars.AUTHZ_SYNC_CREATE_WITH_POLICY_STORE)) { return; } - // don't sync privileges if the operation has failed - if (!dbEvent.getStatus()) { - return; - } + dropSentryDbPrivileges(dbEvent.getDatabase().getName()); } /** - * Drop the privileges on the database // note that child tables will be - * dropped individually by client, so we // just need to handle the removing - * the db privileges. The table drop // should cleanup the table privileges + * Drop the privileges on the database. Note that child tables will be + * dropped individually by client, so we just need to handle the removing + * the db privileges. The table drop should cleanup the table privileges. */ @Override public void onDropDatabase(DropDatabaseEvent dbEvent) throws MetaException { + + // don't sync paths/privileges if the operation has failed + if (!dbEvent.getStatus()) { + LOGGER.debug("Skip syncing paths/privileges with Sentry server for onDropDatabase event," + + " since the operation failed. \n"); + return; + } + String authzObj = dbEvent.getDatabase().getName(); for (SentryMetastoreListenerPlugin plugin : sentryPlugins) { List<String> tNames = dbEvent.getHandler().get_all_tables(authzObj); @@ -167,10 +195,7 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { if (!syncWithPolicyStore(AuthzConfVars.AUTHZ_SYNC_DROP_WITH_POLICY_STORE)) { return; } - // don't sync privileges if the operation has failed - if (!dbEvent.getStatus()) { - return; - } + dropSentryDbPrivileges(dbEvent.getDatabase().getName()); } @@ -180,17 +205,22 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onAlterTable (AlterTableEvent tableEvent) throws MetaException { String oldTableName = null, newTableName = null; + // don't sync privileges if the operation has failed if (!tableEvent.getStatus()) { + LOGGER.debug("Skip syncing privileges with Sentry server for onAlterTable event," + + " since the operation failed. \n"); return; } if (tableEvent.getOldTable() != null) { oldTableName = tableEvent.getOldTable().getTableName(); } + if (tableEvent.getNewTable() != null) { newTableName = tableEvent.getNewTable().getTableName(); } + renameSentryTablePrivilege(tableEvent.getOldTable().getDbName(), oldTableName, tableEvent.getOldTable().getSd().getLocation(), tableEvent.getNewTable().getDbName(), newTableName, @@ -200,10 +230,14 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onAlterPartition(AlterPartitionEvent partitionEvent) throws MetaException { + // don't sync privileges if the operation has failed if (!partitionEvent.getStatus()) { + LOGGER.debug("Skip syncing privileges with Sentry server for onAlterPartition event," + + " since the operation failed. \n"); return; } + String oldLoc = null, newLoc = null; if (partitionEvent.getOldPartition() != null) { oldLoc = partitionEvent.getOldPartition().getSd().getLocation(); @@ -226,6 +260,14 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onAddPartition(AddPartitionEvent partitionEvent) throws MetaException { + + // don't sync path if the operation has failed + if (!partitionEvent.getStatus()) { + LOGGER.debug("Skip syncing path with Sentry server for onAddPartition event," + + " since the operation failed. \n"); + return; + } + for (Partition part : partitionEvent.getPartitions()) { if (part.getSd() != null && part.getSd().getLocation() != null) { String authzObj = part.getDbName() + "." + part.getTableName(); @@ -241,6 +283,14 @@ public class SentryMetastorePostEventListener extends MetaStoreEventListener { @Override public void onDropPartition(DropPartitionEvent partitionEvent) throws MetaException { + + // don't sync path if the operation has failed + if (!partitionEvent.getStatus()) { + LOGGER.debug("Skip syncing path with Sentry server for onDropPartition event," + + " since the operation failed. \n"); + return; + } + String authzObj = partitionEvent.getTable().getDbName() + "." + partitionEvent.getTable().getTableName(); String path = partitionEvent.getPartition().getSd().getLocation(); http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/2ae1befd/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java index 5a93ba0..fc7f324 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java @@ -252,6 +252,12 @@ public class TestHDFSIntegration { hiveConf.set("datanucleus.autoStartMechanism", "SchemaTable"); hmsPort = findPort(); LOGGER.info("\n\n HMS port : " + hmsPort + "\n\n"); + + // Sets hive.metastore.authorization.storage.checks to true, so that + // disallow the operations such as drop-partition if the user in question + // doesn't have permissions to delete the corresponding directory + // on the storage. + hiveConf.set("hive.metastore.authorization.storage.checks", "true"); hiveConf.set("hive.metastore.uris", "thrift://localhost:" + hmsPort); hiveConf.set("hive.metastore.pre.event.listeners", "org.apache.sentry.binding.metastore.MetastoreAuthzBinding"); hiveConf.set("hive.metastore.event.listeners", "org.apache.sentry.binding.metastore.SentryMetastorePostEventListener"); @@ -944,6 +950,232 @@ public class TestHDFSIntegration { } + /** + * Make sure when events such as table creation fail, the path should not be sync to NameNode plugin. + */ + @Test + public void testTableCreationFailure() throws Throwable { + String dbName = "db1"; + + tmpHDFSDir = new Path("/tmp/external"); + if (!miniDFS.getFileSystem().exists(tmpHDFSDir)) { + miniDFS.getFileSystem().mkdirs(tmpHDFSDir); + } + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwx---")); + Path partitionDir = new Path("/tmp/external/p1"); + if (!miniDFS.getFileSystem().exists(partitionDir)) { + miniDFS.getFileSystem().mkdirs(partitionDir); + } + + dbNames = new String[]{dbName}; + roles = new String[]{"admin_role"}; + admin = StaticUserGroup.ADMIN1; + + Connection conn; + Statement stmt; + + conn = hiveServer2.createConnection("hive", "hive"); + stmt = conn.createStatement(); + stmt.execute("create role admin_role"); + stmt.execute("grant all on server server1 to role admin_role"); + stmt.execute("grant all on uri 'hdfs:///tmp/external' to role admin_role"); + stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP); + stmt.execute("grant role admin_role to group " + StaticUserGroup.HIVE); + stmt.close(); + conn.close(); + + conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1); + stmt = conn.createStatement(); + stmt.execute("create database " + dbName); + + // Expect table creation to fail because hive:hive does not have + // permission to write at parent directory. + try { + stmt.execute("create external table tab1(a int) location 'hdfs:///tmp/external/p1'"); + Assert.fail("Expect table creation to fail"); + } catch (Exception ex) { + LOGGER.error("Exception when creating table: " + ex.getMessage()); + } + + // When the table creation failed, the path will not be managed by sentry. And the + // permission of the path will not be hive:hive. + verifyOnAllSubDirs("/tmp/external/p1", null, StaticUserGroup.HIVE, true); + + stmt.close(); + conn.close(); + } + + /** + * Make sure when events such as add partition fail, the path should not be sync to NameNode plugin. + */ + @Test + public void testAddPartitionFailure() throws Throwable { + String dbName = "db1"; + + tmpHDFSDir = new Path("/tmp/external"); + if (!miniDFS.getFileSystem().exists(tmpHDFSDir)) { + miniDFS.getFileSystem().mkdirs(tmpHDFSDir); + } + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwx---")); + Path partitionDir = new Path("/tmp/external/p1"); + if (!miniDFS.getFileSystem().exists(partitionDir)) { + miniDFS.getFileSystem().mkdirs(partitionDir); + } + + dbNames = new String[]{dbName}; + roles = new String[]{"admin_role"}; + admin = StaticUserGroup.ADMIN1; + + Connection conn; + Statement stmt; + + conn = hiveServer2.createConnection("hive", "hive"); + stmt = conn.createStatement(); + stmt.execute("create role admin_role"); + stmt.execute("grant all on server server1 to role admin_role"); + stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP); + stmt.close(); + conn.close(); + + conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1); + stmt = conn.createStatement(); + stmt.execute("create database " + dbName); + stmt.execute("create external table tab2 (s string) partitioned by (month int)"); + + // Expect adding partition to fail because hive:hive does not have + // permission to write at parent directory. + try { + stmt.execute("alter table tab2 add partition (month = 1) location '/tmp/external/p1'"); + Assert.fail("Expect adding partition to fail"); + } catch (Exception ex) { + LOGGER.error("Exception when adding partition: " + ex.getMessage()); + } + + // When the table creation failed, the path will not be managed by sentry. And the + // permission of the path will not be hive:hive. + verifyOnAllSubDirs("/tmp/external/p1", null, StaticUserGroup.HIVE, true); + + stmt.close(); + conn.close(); + } + + /** + * Make sure when events such as drop table fail, the path should not be sync to NameNode plugin. + */ + @Test + public void testDropTableFailure() throws Throwable { + String dbName = "db1"; + tmpHDFSDir = new Path("/tmp/external"); + if (!miniDFS.getFileSystem().exists(tmpHDFSDir)) { + miniDFS.getFileSystem().mkdirs(tmpHDFSDir); + } + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwxrwx")); + Path partitionDir = new Path("/tmp/external/p1"); + if (!miniDFS.getFileSystem().exists(partitionDir)) { + miniDFS.getFileSystem().mkdirs(partitionDir); + } + dbNames = new String[]{dbName}; + roles = new String[]{"admin_role"}; + admin = StaticUserGroup.ADMIN1; + + Connection conn; + Statement stmt; + + conn = hiveServer2.createConnection("hive", "hive"); + stmt = conn.createStatement(); + stmt.execute("create role admin_role"); + stmt.execute("grant all on server server1 to role admin_role"); + stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP); + stmt.close(); + conn.close(); + + conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1); + stmt = conn.createStatement(); + stmt.execute("create database " + dbName); + stmt.execute("create external table tab1(a int) location 'hdfs:///tmp/external/p1'"); + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwx---")); + + // Expect dropping table to fail because hive:hive does not have + // permission to write at parent directory when + // hive.metastore.authorization.storage.checks property is true. + try { + stmt.execute("drop table tab1"); + Assert.fail("Expect dropping table to fail"); + } catch (Exception ex) { + LOGGER.error("Exception when creating table: " + ex.getMessage()); + } + + // When the table dropping failed, the path will still be managed by sentry. And the + // permission of the path still should be hive:hive. + verifyOnAllSubDirs("/tmp/external/p1", FsAction.ALL, StaticUserGroup.HIVE, true); + + stmt.close(); + conn.close(); + } + + /** + * Make sure when events such as drop table fail, the path should not be sync to NameNode plugin. + */ + @Test + public void testDropPartitionFailure() throws Throwable { + String dbName = "db1"; + + tmpHDFSDir = new Path("/tmp/external"); + if (!miniDFS.getFileSystem().exists(tmpHDFSDir)) { + miniDFS.getFileSystem().mkdirs(tmpHDFSDir); + } + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwxrwx")); + Path partitionDir = new Path("/tmp/external/p1"); + if (!miniDFS.getFileSystem().exists(partitionDir)) { + miniDFS.getFileSystem().mkdirs(partitionDir); + } + + dbNames = new String[]{dbName}; + roles = new String[]{"admin_role"}; + admin = StaticUserGroup.ADMIN1; + + Connection conn; + Statement stmt; + + conn = hiveServer2.createConnection("hive", "hive"); + stmt = conn.createStatement(); + stmt.execute("create role admin_role"); + stmt.execute("grant all on server server1 to role admin_role"); + stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP); + stmt.close(); + conn.close(); + + conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1); + stmt = conn.createStatement(); + stmt.execute("create database " + dbName); + stmt.execute("create table tab3 (s string) partitioned by (month int)"); + stmt.execute("alter table tab3 add partition (month = 1) location '/tmp/external/p1'"); + + miniDFS.getFileSystem().setPermission(tmpHDFSDir, FsPermission.valueOf("drwxrwx---")); + + + // Expect dropping partition to fail because because hive:hive does not have + // permission to write at parent directory. + try { + stmt.execute("ALTER TABLE tab3 DROP PARTITION (month = 1)"); + Assert.fail("Expect dropping partition to fail"); + } catch (Exception ex) { + LOGGER.error("Exception when dropping partition: " + ex.getMessage()); + } + + // When the partition dropping failed, the path for the partition will still + // be managed by sentry. And the permission of the path still should be hive:hive. + verifyOnAllSubDirs("/tmp/external/p1", FsAction.ALL, StaticUserGroup.HIVE, true); + + stmt.close(); + conn.close(); + } + @Test public void testColumnPrivileges() throws Throwable { String dbName = "db2";
