replace method setAcl with modifyAclEntries while preserving acls

apache · Sep 1, 2023 · 9486b5a · 9486b5a
1 parent 8f0938f
commit 9486b5a
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 33 deletions.
diff --git a/...n-data-management/src/main/java/org/apache/gobblin/data/management/copy/CopyableFile.java b/...n-data-management/src/main/java/org/apache/gobblin/data/management/copy/CopyableFile.java
@@ -29,10 +29,7 @@
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.permission.AclEntry;
-import org.apache.hadoop.fs.permission.AclEntryScope;
-import org.apache.hadoop.fs.permission.AclEntryType;
 import org.apache.hadoop.fs.permission.AclStatus;
-import org.apache.hadoop.fs.permission.FsAction;
 import org.apache.hadoop.fs.permission.FsPermission;
 
 import com.google.common.base.Optional;
@@ -444,33 +441,7 @@ public static Map<String, OwnerAndPermission> resolveReplicatedAncestorOwnerAndP
 
   private static List<AclEntry> getAclEntries(FileSystem srcFs, Path path) throws IOException {
     AclStatus aclStatus = srcFs.getAclStatus(path);
-    return addOthersEntryTypeToAclEntriesIfMissing(aclStatus.getEntries());
-  }
-  /*
-   * When we get AclEntry from org.apache.hadoop.fs.permission.AclStatus, it's missing AclEntryType.OTHER.
-   * This causes AclTransformation validation to fail on the list of AclEntries. As a result, adding this helper
-   * method to provide DEFAULT scope in cases where AclEntryType.OTHER is absent
-   */
-  private static List<AclEntry> addOthersEntryTypeToAclEntriesIfMissing(List<AclEntry> aclEntries) {
-    // Check if "others" entry is missing
-    boolean othersAclEntryTypeMissing = true;
-
-    for (AclEntry aclEntry : aclEntries) {
-      if (aclEntry.getType() == AclEntryType.OTHER) {
-        othersAclEntryTypeMissing = false;
-        break;
-      }
-    }
-    // If "others" entry is missing, add it
-    if (othersAclEntryTypeMissing) {
-      AclEntry othersEntry = new AclEntry.Builder().setType(AclEntryType.OTHER).setScope(AclEntryScope.DEFAULT)
-          .setPermission(FsAction.READ_EXECUTE).build();
-
-      // Modify the ACL entries to include the new "others" entry
-      aclEntries.add(othersEntry);
-    }
-    return aclEntries;
-
+    return aclStatus.getEntries();
   }
 
   @Override

diff --git a/...n/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java b/...n/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriter.java
@@ -362,7 +362,9 @@ public static void safeSetPathPermission(FileSystem fs, FileStatus file, OwnerAn
         fs.setPermission(path, targetOwnerAndPermission.getFsPermission());
       }
       if (!targetOwnerAndPermission.getAclEntries().isEmpty()) {
-        fs.setAcl(path, targetOwnerAndPermission.getAclEntries());
+        // use modify acls instead of setAcl since latter requires all three acl entry types: user, group and others
+        // while overwriting the acls for a given path. If anyone is absent it fails acl transformation validation.
+        fs.modifyAclEntries(path, targetOwnerAndPermission.getAclEntries());
       }
     } catch (IOException ioe) {
       log.warn("Failed to set permission for directory " + path, ioe);
@@ -519,7 +521,9 @@ private void ensureDirectoryExists(FileSystem fs, Path path, Iterator<OwnerAndPe
         log.warn("Failed to set owner and/or group for path " + path + " to " + owner + ":" + group, ioe);
       }
       if (!aclEntries.isEmpty()) {
-        fs.setAcl(path, aclEntries);
+        // use modify acls instead of setAcl since latter requires all three acl entry types: user, group and others
+        // while overwriting the acls for a given path. If anyone is absent it fails acl transformation validation.
+        fs.modifyAclEntries(path, aclEntries);
       }
     } else {
       fs.mkdirs(path);

diff --git a/...va/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java b/...va/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java
@@ -647,7 +647,7 @@ public void cleanup() {
   protected class TestLocalFileSystem extends LocalFileSystem {
     private final ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries = new ConcurrentHashMap<>();
     @Override
-    public void setAcl(Path path, List<AclEntry> aclEntries) {
+    public void modifyAclEntries(Path path, List<AclEntry> aclEntries) {
       pathToAclEntries.put(path, aclEntries);
     }
     public ImmutableMap<Path, List<AclEntry>> getPathToAclEntries() {

diff --git a/gobblin-utility/src/main/java/org/apache/gobblin/util/filesystem/FileSystemDecorator.java b/gobblin-utility/src/main/java/org/apache/gobblin/util/filesystem/FileSystemDecorator.java
@@ -110,6 +110,9 @@ public AclStatus getAclStatus(Path path) throws IOException {
   public void setAcl(Path path, List<AclEntry> aclSpec) throws IOException {
     this.underlyingFs.setAcl(path, aclSpec);
   }
+  public void modifyAclEntries(Path path, List<AclEntry> aclSpec) throws IOException {
+    this.underlyingFs.modifyAclEntries(path, aclSpec);
+  }
 
   public FsStatus getStatus() throws java.io.IOException {
     return this.underlyingFs.getStatus();