From 5a980a4a54466d0458986d5628fb1e8f4c835ffc Mon Sep 17 00:00:00 2001 From: Tamas Domok Date: Mon, 30 Aug 2021 09:43:35 +0200 Subject: [PATCH 1/2] YARN-10901. Fix permission checking error on an existing directory in LogAggregationFileController. Create a temporary file with proper ownership to see if the underlying file system supports chmod. When the log directory was already created, but with a different user, there was an exception in the log: org.apache.hadoop.security.AccessControlException: Permission denied. user=yarn is not the owner of inode=/tmp/logs because the setPermission checked the ownership of the given path. Change-Id: I7c7908b7ec815501337daaf7a2253a11d79afc8c --- .../LogAggregationFileController.java | 13 ++++- .../TestLogAggregationFileController.java | 55 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileController.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileController.java index c6e34ef6a9da7..6edaac3719b51 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileController.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileController.java @@ -18,6 +18,7 @@ package org.apache.hadoop.yarn.logaggregation.filecontroller; +import org.apache.commons.lang3.RandomStringUtils; import org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting; import java.io.FileNotFoundException; @@ -427,10 +428,13 @@ public void verifyAndCreateRemoteLogDir() { throw new YarnRuntimeException("Failed to create remoteLogDir [" + remoteRootLogDir + "]", e); } - } else{ + } else { //Check if FS has capability to set/modify permissions + Path permissionCheckFile = new Path(qualified, String.format("%s.permission_check", + RandomStringUtils.randomAlphanumeric(8))); try { - remoteFS.setPermission(qualified, new FsPermission(TLDIR_PERMISSIONS)); + remoteFS.createNewFile(permissionCheckFile); + remoteFS.setPermission(permissionCheckFile, new FsPermission(TLDIR_PERMISSIONS)); } catch (UnsupportedOperationException use) { LOG.info("Unable to set permissions for configured filesystem since" + " it does not support this", remoteFS.getScheme()); @@ -438,6 +442,11 @@ public void verifyAndCreateRemoteLogDir() { } catch (IOException e) { LOG.warn("Failed to check if FileSystem suppports permissions on " + "remoteLogDir [" + remoteRootLogDir + "]", e); + } finally { + try { + remoteFS.delete(permissionCheckFile, false); + } catch (IOException ignored) { + } } } } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java index 5ade0faabd89a..6cfb85328e07d 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java @@ -19,17 +19,25 @@ package org.apache.hadoop.yarn.logaggregation.filecontroller; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; +import org.apache.hadoop.metrics2.MetricsInfo; import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.yarn.conf.YarnConfiguration; +import org.junit.Assert; import org.junit.Test; +import org.mockito.ArgumentMatcher; import org.mockito.Mockito; import java.io.FileNotFoundException; import java.net.URI; +import static org.apache.hadoop.thirdparty.com.google.common.base.Preconditions.checkNotNull; +import static org.apache.hadoop.yarn.logaggregation.filecontroller.LogAggregationFileController.TLDIR_PERMISSIONS; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; @@ -88,4 +96,51 @@ public void testRemoteDirCreationWithCustomGroup() throws Exception { verify(fs).setOwner(any(), eq("yarn_user"), eq(testGroupName)); } + + private static class PathContainsString implements ArgumentMatcher { + private final String expected; + + PathContainsString(String expected) { + this.expected = expected; + } + + @Override + public boolean matches(Path path) { + return path.getName().contains(expected); + } + + @Override + public String toString() { + return "Path with name=" + expected; + } + } + + @Test + public void testRemoteDirCreationWithCustomUser() throws Exception { + FileSystem fs = mock(FileSystem.class); + doReturn(new URI("")).when(fs).getUri(); + doReturn(new FileStatus(128, false, 0, 64, System.currentTimeMillis(), + System.currentTimeMillis(), new FsPermission(TLDIR_PERMISSIONS), + "not_yarn_user", "yarn_group", new Path("/tmp/logs"))).when(fs) + .getFileStatus(any(Path.class)); + + Configuration conf = new Configuration(); + LogAggregationFileController controller = mock( + LogAggregationFileController.class, Mockito.CALLS_REAL_METHODS); + controller.fsSupportsChmod = true; + doReturn(fs).when(controller).getFileSystem(any(Configuration.class)); + + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "yarn_user", new String[]{"yarn_group", "other_group"}); + UserGroupInformation.setLoginUser(ugi); + + controller.initialize(conf, "TFile"); + controller.verifyAndCreateRemoteLogDir(); + + verify(fs).createNewFile(argThat(new PathContainsString(".permission_check"))); + verify(fs).setPermission(argThat(new PathContainsString(".permission_check")), + eq(new FsPermission(TLDIR_PERMISSIONS))); + verify(fs).delete(argThat(new PathContainsString(".permission_check")), eq(false)); + Assert.assertTrue(controller.fsSupportsChmod); + } } From 7bdfbd501b4997624ad17992494ea20fa0d95194 Mon Sep 17 00:00:00 2001 From: Tamas Domok Date: Tue, 31 Aug 2021 07:26:09 +0200 Subject: [PATCH 2/2] fixup! YARN-10901. Fix permission checking error on an existing directory in LogAggregationFileController. Change-Id: Id48d073d036b939236f73b2755c2e4e8c3c4227a --- .../filecontroller/TestLogAggregationFileController.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java index 6cfb85328e07d..818e01129fc60 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java @@ -23,7 +23,6 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.permission.FsPermission; -import org.apache.hadoop.metrics2.MetricsInfo; import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.yarn.conf.YarnConfiguration; import org.junit.Assert; @@ -34,7 +33,6 @@ import java.io.FileNotFoundException; import java.net.URI; -import static org.apache.hadoop.thirdparty.com.google.common.base.Preconditions.checkNotNull; import static org.apache.hadoop.yarn.logaggregation.filecontroller.LogAggregationFileController.TLDIR_PERMISSIONS; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat;