diff --git common/src/java/org/apache/hadoop/hive/common/FileUtils.java common/src/java/org/apache/hadoop/hive/common/FileUtils.java index ad82f62..b36a016 100644 --- common/src/java/org/apache/hadoop/hive/common/FileUtils.java +++ common/src/java/org/apache/hadoop/hive/common/FileUtils.java @@ -18,9 +18,11 @@ package org.apache.hadoop.hive.common; +import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.util.ArrayList; import java.util.BitSet; import java.util.List; @@ -441,4 +443,50 @@ public static boolean isOwnerOfFileHierarchy(FileSystem fs, FileStatus fileStatu } return true; } + + /** + * Creates the directory and all necessary parent directories. + * @param fs FileSystem to use + * @param f path to create. + * @param inheritPerms whether directory inherits the permission of the last-existing parent path + * @return true if directory created successfully. False otherwise, including if it exists. + * @throws IOException exception in creating the directory + */ + public static boolean mkdir(FileSystem fs, Path f, boolean inheritPerms) throws IOException { + LOG.info("Creating directory if it doesn't exist: " + f); + if (!inheritPerms) { + //just create the directory + return fs.mkdirs(f); + } else { + //Check if the directory already exists. We want to change the permission + //to that of the parent directory only for newly created directories. + try { + return fs.getFileStatus(f).isDir(); + } catch (FileNotFoundException ignore) { + } + //inherit perms: need to find last existing parent path, and apply its permission on entire subtree. + Path path = f; + List pathsToSet = new ArrayList(); + while (!fs.exists(path)) { + pathsToSet.add(path); + path = path.getParent(); + } + //at the end of this loop, path is the last-existing parent path. + boolean success = fs.mkdirs(f); + if (!success) { + return false; + } else { + FsPermission parentPerm = fs.getFileStatus(path).getPermission(); + String parentGroup = fs.getFileStatus(path).getGroup(); + for (Path pathToSet : pathsToSet) { + String currOwner = fs.getFileStatus(pathToSet).getOwner(); + LOG.info("Setting permission and group of parent directory: " + path.toString() + + " on new directory: " + pathToSet.toString()); + fs.setPermission(pathToSet, parentPerm); + fs.setOwner(pathToSet, currOwner, parentGroup); + } + return true; + } + } + } } diff --git itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java index 47e94ea..74a4aff 100644 --- itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java +++ itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java @@ -393,7 +393,7 @@ public static void partitionTester(HiveMetaStoreClient client, HiveConf hiveConf // create dir for /mpart5 Path mp5Path = new Path(mpart5.getSd().getLocation()); - warehouse.mkdirs(mp5Path); + warehouse.mkdirs(mp5Path, true); assertTrue(fs.exists(mp5Path)); assertEquals(dbPermission, fs.getFileStatus(mp5Path).getPermission()); diff --git itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java index f1c7b7b..a635bb0 100644 --- itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java +++ itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestFolderPermissions.java @@ -19,12 +19,15 @@ package org.apache.hadoop.hive.ql.security; import java.net.URI; +import java.util.ArrayList; +import java.util.List; import junit.framework.Assert; -import junit.framework.TestCase; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.PathFilter; import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hive.cli.CliSessionState; import org.apache.hadoop.hive.conf.HiveConf; @@ -32,27 +35,37 @@ import org.apache.hadoop.hive.ql.Driver; import org.apache.hadoop.hive.ql.processors.CommandProcessorResponse; import org.apache.hadoop.hive.ql.session.SessionState; +import org.apache.hadoop.hive.shims.HadoopShims; import org.apache.hadoop.hive.shims.ShimLoader; +import org.junit.BeforeClass; import org.junit.Test; /** * This test the flag 'hive.warehouse.subdir.inherit.perms'. */ -public class TestFolderPermissions extends TestCase { - protected HiveConf conf; - protected Driver driver; - protected String dataFileDir; - protected Path dataFilePath; - protected String testDir; +public class TestFolderPermissions { + protected static HiveConf conf; + protected static Driver driver; + protected static String dataFileDir; + protected static Path dataFilePath; + protected static String testDir; + protected static FileSystem fs; + public static final PathFilter hiddenFileFilter = new PathFilter(){ + public boolean accept(Path p){ + String name = p.getName(); + return !name.startsWith("_") && !name.startsWith("."); + } + }; - @Override - protected void setUp() throws Exception { - super.setUp(); + + @BeforeClass + public static void setUp() throws Exception { testDir = System.getProperty("test.warehouse.dir"); - conf = new HiveConf(this.getClass()); + conf = new HiveConf(TestFolderPermissions.class); + fs = FileSystem.get(new URI(testDir), conf); dataFileDir = conf.get("test.data.files").replace('\\', '/') .replace("c:", ""); dataFilePath = new Path(dataFileDir, "kv1.txt"); @@ -60,54 +73,175 @@ protected void setUp() throws Exception { int port = MetaStoreUtils.findFreePort(); conf.setBoolVar(HiveConf.ConfVars.HIVE_SUPPORT_CONCURRENCY, false); conf.setBoolVar(HiveConf.ConfVars.HIVE_WAREHOUSE_SUBDIR_INHERIT_PERMS, true); - - // Turn off metastore-side authorization - System.setProperty(HiveConf.ConfVars.METASTORE_PRE_EVENT_LISTENERS.varname, - ""); + conf.setVar(HiveConf.ConfVars.DYNAMICPARTITIONINGMODE, "nonstrict"); MetaStoreUtils.startMetaStore(port, ShimLoader.getHadoopThriftAuthBridge()); SessionState.start(new CliSessionState(conf)); driver = new Driver(conf); + + setupDataTable(); + } + + + private static void setupDataTable() throws Exception { + CommandProcessorResponse ret = driver.run("DROP TABLE IF EXISTS mysrc"); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("CREATE TABLE mysrc (key STRING, value STRING) PARTITIONED BY (part1 string, part2 string) STORED AS TEXTFILE"); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("LOAD DATA LOCAL INPATH '" + dataFilePath + "' INTO TABLE mysrc PARTITION (part1='1',part2='1')"); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("LOAD DATA LOCAL INPATH '" + dataFilePath + "' INTO TABLE mysrc PARTITION (part1='2',part2='2')"); + Assert.assertEquals(0,ret.getResponseCode()); + } + + @Test + public void testCreateTablePerms() throws Exception { + String testDb = "mydb"; + String tableName = "createtable"; + CommandProcessorResponse ret = driver.run("CREATE DATABASE " + testDb); + Assert.assertEquals(0,ret.getResponseCode()); + + assertExistence(testDir + "/" + testDb + ".db"); + setPermissions(testDir + "/" + testDb + ".db", FsPermission.createImmutable((short) 0777)); + + ret = driver.run("USE " + testDb); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("CREATE TABLE " + tableName + " (key string, value string)"); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("insert into table " + tableName + " select key,value from default.mysrc"); + + assertExistence(testDir + "/" + testDb + ".db/" + tableName); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + testDb + ".db/" + tableName).toString()); + + ret = driver.run("USE default"); + Assert.assertEquals(0,ret.getResponseCode()); } + @Test public void testStaticPartitionPerms() throws Exception { + String tableName = "staticpart"; + CommandProcessorResponse ret = driver.run("CREATE TABLE " + tableName + " (key string, value string) partitioned by (part1 string, part2 string)"); + Assert.assertEquals(0,ret.getResponseCode()); - CommandProcessorResponse ret = driver.run("DROP TABLE IF EXISTS mysrc"); - assertEquals(0,ret.getResponseCode()); + assertExistence(testDir + "/" + tableName); + setPermissions(testDir + "/" + tableName, FsPermission.createImmutable((short) 0777)); - ret = driver.run("CREATE TABLE mysrc (key STRING, value STRING) STORED AS TEXTFILE"); - assertEquals(0,ret.getResponseCode()); - ret = driver.run("LOAD DATA LOCAL INPATH '" + dataFilePath + "' INTO TABLE mysrc"); - assertEquals(0,ret.getResponseCode()); + ret = driver.run("insert into table " + tableName + " partition(part1='1', part2='1') select key,value from mysrc where part1='1' and part2='1'"); + Assert.assertEquals(0,ret.getResponseCode()); - ret = driver.run("CREATE TABLE newtable (key string, value string) partitioned by (part1 int, part2 int)"); - assertEquals(0,ret.getResponseCode()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=1").toString()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=1/part2=1").toString()); - assertExistence(testDir + "/newtable"); - setPermissions(testDir + "/newtable", FsPermission.createImmutable((short) 0777)); + Assert.assertTrue(listChildrenPerms(testDir + "/" + tableName + "/part1=1/part2=1").size() > 0); + for (FsPermission perm : listChildrenPerms(testDir + "/" + tableName + "/part1=1/part2=1")) { + Assert.assertEquals("rwxrwxrwx", perm.toString()); + } + } + + @Test + public void testAlterPartitionPerms() throws Exception { + String tableName = "alterpart"; + CommandProcessorResponse ret = driver.run("CREATE TABLE " + tableName + " (key string, value string) partitioned by (part1 int, part2 int, part3 int)"); + Assert.assertEquals(0,ret.getResponseCode()); - ret = driver.run("insert into table newtable partition(part1='1',part2='1') select * from mysrc"); - assertEquals(0,ret.getResponseCode()); + assertExistence(testDir + "/" + tableName); + setPermissions(testDir + "/" + tableName, FsPermission.createImmutable((short) 0777)); - Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/newtable/part1=1").toString()); - Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/newtable/part1=1/part2=1").toString()); + ret = driver.run("insert into table " + tableName + " partition(part1='1',part2='1',part3='1') select key,value from mysrc"); + Assert.assertEquals(0,ret.getResponseCode()); + + //alter partition + ret = driver.run("alter table " + tableName + " partition (part1='1',part2='1',part3='1') rename to partition (part1='2',part2='2',part3='2')"); + Assert.assertEquals(0,ret.getResponseCode()); + + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=2").toString()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=2/part2=2").toString()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=2/part2=2/part3=2").toString()); + + Assert.assertTrue(listChildrenPerms(testDir + "/" + tableName + "/part1=2/part2=2/part3=2").size() > 0); + for (FsPermission perm : listChildrenPerms(testDir + "/" + tableName + "/part1=2/part2=2/part3=2")) { + Assert.assertEquals("rwxrwxrwx", perm.toString()); + } + } + + + @Test + public void testDynamicPartitions() throws Exception { + String tableName = "dynamicpart"; + + CommandProcessorResponse ret = driver.run("CREATE TABLE " + tableName + " (key string, value string) partitioned by (part1 string, part2 string)"); + Assert.assertEquals(0,ret.getResponseCode()); + + assertExistence(testDir + "/" + tableName); + setPermissions(testDir + "/" + tableName, FsPermission.createImmutable((short) 0777)); + + ret = driver.run("insert into table " + tableName + " partition (part1,part2) select key,value,part1,part2 from mysrc"); + Assert.assertEquals(0,ret.getResponseCode()); + + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=1").toString()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=1/part2=1").toString()); + + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=2").toString()); + Assert.assertEquals("rwxrwxrwx", getPermissions(testDir + "/" + tableName + "/part1=2/part2=2").toString()); + + Assert.assertTrue(listChildrenPerms(testDir + "/" + tableName + "/part1=1/part2=1").size() > 0); + for (FsPermission perm : listChildrenPerms(testDir + "/" + tableName + "/part1=1/part2=1")) { + Assert.assertEquals("rwxrwxrwx", perm.toString()); + } + + Assert.assertTrue(listChildrenPerms(testDir + "/" + tableName + "/part1=2/part2=2").size() > 0); + for (FsPermission perm : listChildrenPerms(testDir + "/" + tableName + "/part1=2/part2=2")) { + Assert.assertEquals("rwxrwxrwx", perm.toString()); + } + } + + @Test + public void testExternalTable() throws Exception { + String tableName = "externaltable"; + + String myLocation = testDir + "/myfolder"; + FileSystem fs = FileSystem.get(new URI(myLocation), conf); + fs.mkdirs(new Path(myLocation), FsPermission.createImmutable((short) 0777)); + + CommandProcessorResponse ret = driver.run("CREATE TABLE " + tableName + " (key string, value string) LOCATION '" + myLocation + "'"); + Assert.assertEquals(0,ret.getResponseCode()); + + ret = driver.run("insert into table " + tableName + " select key,value from mysrc"); + Assert.assertEquals(0,ret.getResponseCode()); + + Assert.assertTrue(listChildrenPerms(myLocation).size() > 0); + for (FsPermission perm : listChildrenPerms(myLocation)) { + Assert.assertEquals("rwxrwxrwx", perm.toString()); + } } private void setPermissions(String locn, FsPermission permissions) throws Exception { - FileSystem fs = FileSystem.get(new URI(locn), conf); fs.setPermission(new Path(locn), permissions); } private FsPermission getPermissions(String locn) throws Exception { - FileSystem fs = FileSystem.get(new URI(locn), conf); return fs.getFileStatus(new Path(locn)).getPermission(); } private void assertExistence(String locn) throws Exception { - FileSystem fs = FileSystem.get(new URI(locn), conf); Assert.assertTrue(fs.exists(new Path(locn))); } + + private List listChildrenPerms(String locn) throws Exception { + HadoopShims hadoopShims = ShimLoader.getHadoopShims(); + List result = new ArrayList(); + List fileStatuses = hadoopShims.listLocatedStatus(fs, new Path(locn), hiddenFileFilter); + for (FileStatus status : fileStatuses) { + result.add(status.getPermission()); + } + return result; + } } diff --git metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java index 8345d70..221b010 100644 --- metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java @@ -395,7 +395,7 @@ public Partition alterPartition(final RawStore msdb, Warehouse wh, final String if (srcFs.exists(srcPath)) { //if destPath's parent path doesn't exist, we should mkdir it Path destParentPath = destPath.getParent(); - if (!wh.mkdirs(destParentPath)) { + if (!wh.mkdirs(destParentPath, true)) { throw new IOException("Unable to create path " + destParentPath); } srcFs.rename(srcPath, destPath); diff --git metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java index c62e085..5fc4af4 100644 --- metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java @@ -728,7 +728,7 @@ private void create_database_core(RawStore ms, final Database db) firePreEvent(new PreCreateDatabaseEvent(db, this)); if (!wh.isDir(dbPath)) { - if (!wh.mkdirs(dbPath)) { + if (!wh.mkdirs(dbPath, false)) { throw new MetaException("Unable to create database path " + dbPath + ", failed to create database " + db.getName()); } @@ -1233,7 +1233,7 @@ private void create_table_core(final RawStore ms, final Table tbl, if (tblPath != null) { if (!wh.isDir(tblPath)) { - if (!wh.mkdirs(tblPath)) { + if (!wh.mkdirs(tblPath, true)) { throw new MetaException(tblPath + " is not a directory or unable to create one"); } @@ -1707,7 +1707,7 @@ private Partition append_partition_common(RawStore ms, String dbName, String tab } if (!wh.isDir(partLocation)) { - if (!wh.mkdirs(partLocation)) { + if (!wh.mkdirs(partLocation, true)) { throw new MetaException(partLocation + " is not a directory or unable to create one"); } @@ -1999,7 +1999,7 @@ private boolean createLocationForAddedPartition( // mkdirs() because if the file system is read-only, mkdirs will // throw an exception even if the directory already exists. if (!wh.isDir(partLocation)) { - if (!wh.mkdirs(partLocation)) { + if (!wh.mkdirs(partLocation, true)) { throw new MetaException(partLocation + " is not a directory or unable to create one"); } diff --git metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java index f731dab..fe15101 100755 --- metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java +++ metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java @@ -44,6 +44,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.FsAction; +import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hive.common.FileUtils; import org.apache.hadoop.hive.common.HiveStatsUtils; import org.apache.hadoop.hive.common.JavaUtils; @@ -71,7 +72,6 @@ private MetaStoreFS fsHandler = null; private boolean storageAuthCheck = false; - private boolean inheritPerms = false; public Warehouse(Configuration conf) throws MetaException { this.conf = conf; @@ -83,8 +83,6 @@ public Warehouse(Configuration conf) throws MetaException { fsHandler = getMetaStoreFsHandler(conf); storageAuthCheck = HiveConf.getBoolVar(conf, HiveConf.ConfVars.METASTORE_AUTHORIZATION_STORAGE_AUTH_CHECKS); - inheritPerms = HiveConf.getBoolVar(conf, - HiveConf.ConfVars.HIVE_WAREHOUSE_SUBDIR_INHERIT_PERMS); } private MetaStoreFS getMetaStoreFsHandler(Configuration conf) @@ -185,25 +183,13 @@ public Path getTablePath(Database db, String tableName) return getDnsPath(new Path(getDatabasePath(db), tableName.toLowerCase())); } - public boolean mkdirs(Path f) throws MetaException { + public boolean mkdirs(Path f, boolean inheritPermCandidate) throws MetaException { + boolean inheritPerms = HiveConf.getBoolVar(conf, + HiveConf.ConfVars.HIVE_WAREHOUSE_SUBDIR_INHERIT_PERMS) && inheritPermCandidate; FileSystem fs = null; try { fs = getFs(f); - LOG.debug("Creating directory if it doesn't exist: " + f); - //Check if the directory already exists. We want to change the permission - //to that of the parent directory only for newly created directories. - if (this.inheritPerms) { - try { - return fs.getFileStatus(f).isDir(); - } catch (FileNotFoundException ignore) { - } - } - boolean success = fs.mkdirs(f); - if (this.inheritPerms && success) { - // Set the permission of parent directory. - fs.setPermission(f, fs.getFileStatus(f.getParent()).getPermission()); - } - return success; + return FileUtils.mkdir(fs, f, inheritPerms); } catch (IOException e) { closeFs(fs); MetaStoreUtils.logAndThrowMetaException(e); diff --git ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java index 2559e0e..a47df51 100644 --- ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java +++ ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java @@ -2279,27 +2279,7 @@ static protected void copyFiles(HiveConf conf, Path srcf, Path destf, try { // create the destination if it does not exist if (!fs.exists(destf)) { - if (inheritPerms) { - //need to find last existing path, and apply its permission on entire subtree. - Path path = destf; - List pathsToSet = new ArrayList(); - while (!fs.exists(path)) { - pathsToSet.add(path); - path = path.getParent(); - } - - //at the end of this loop, path is the last existing path (the real parent). - fs.mkdirs(destf); - FsPermission parentPerm = fs.getFileStatus(path).getPermission(); - for (Path pathToSet : pathsToSet) { - LOG.info("setting permission of parent folder: " + path.toString() + - " on new directory: " + pathToSet.toString()); - fs.setPermission(pathToSet, parentPerm); - } - } else { - //simply make the directory. - fs.mkdirs(destf); - } + FileUtils.mkdir(fs, destf, inheritPerms); } } catch (IOException e) { throw new HiveException(