From 927084887e22232be377508ebda9b96cc82369d3 Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Fri, 10 Jul 2026 13:11:59 -0700 Subject: [PATCH] HDDS-15804. Use Set instead of List for ACLs in OMKeyRequest. --- .../org/apache/hadoop/ozone/OzoneAcl.java | 22 ++++++-- .../ozone/om/helpers/AclListBuilder.java | 7 ++- .../ozone/om/helpers/OmDirectoryInfo.java | 4 +- .../hadoop/ozone/om/helpers/OmKeyInfo.java | 3 +- .../hadoop/ozone/om/helpers/OzoneAclUtil.java | 55 ++++++------------- .../ozone/security/acl/IAccessAuthorizer.java | 16 +----- .../ozone/om/request/key/OMKeyRequest.java | 17 ++---- .../ozone/s3/endpoint/BucketAclHandler.java | 7 ++- 8 files changed, 52 insertions(+), 79 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java index 695b85afcdf6..2d22c4879e14 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OzoneAcl.java @@ -76,7 +76,7 @@ public final class OzoneAcl { @JsonIgnore private final Supplier toStringMethod; @JsonIgnore - private final Supplier hashCodeMethod; + private final MemoizedSupplier hashCodeMethod; public static OzoneAcl of(ACLIdentityType type, String name, AclScope scope, ACLType... acls) { return new OzoneAcl(type, name, scope, toInt(acls)); @@ -348,6 +348,13 @@ public ACLIdentityType getType() { return type; } + public boolean sameNameTypeScope(OzoneAcl that) { + return this.getType() == that.getType() + && this.getAclScope() == that.getAclScope() + // compare string at last since it is expensive + && this.getName().equals(that.getName()); + } + /** * Indicates whether some other object is "equal to" this one. * @@ -364,11 +371,14 @@ public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) { return false; } - OzoneAcl otherAcl = (OzoneAcl) obj; - return otherAcl.getName().equals(this.getName()) && - otherAcl.getType().equals(this.getType()) && - this.aclBits == otherAcl.aclBits && - otherAcl.getAclScope().equals(this.getAclScope()); + final OzoneAcl that = (OzoneAcl) obj; + if (this.hashCodeMethod.isInitialized() && that.hashCodeMethod.isInitialized()) { + if (!Objects.equals(this.hashCodeMethod.get(), that.hashCodeMethod.get())) { + return false; + } + } + return this.aclBits == that.aclBits + && sameNameTypeScope(that); } /** diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/AclListBuilder.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/AclListBuilder.java index 5e097ff16639..23d00cc4bdca 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/AclListBuilder.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/AclListBuilder.java @@ -21,6 +21,7 @@ import jakarta.annotation.Nonnull; import jakarta.annotation.Nullable; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Objects; import org.apache.hadoop.ozone.OzoneAcl; @@ -31,7 +32,7 @@ public final class AclListBuilder { /** The original list being built from, used if no changes are made, to reduce copying. */ private final ImmutableList originalList; /** The updated list being built, created lazily on the first modification. */ - private List updatedList; + private Collection updatedList; /** Whether any changes were made. */ private boolean changed; @@ -80,7 +81,7 @@ public boolean add(@Nonnull OzoneAcl acl) { return added; } - public boolean addAll(@Nullable List newAcls) { + public boolean addAll(@Nullable Collection newAcls) { if (newAcls == null || newAcls.isEmpty()) { return false; } @@ -91,7 +92,7 @@ public boolean addAll(@Nullable List newAcls) { } /** Set the list being built to {@code acls}. For further mutations to work, it must be modifiable. */ - public boolean set(@Nonnull List acls) { + public boolean set(@Nonnull Collection acls) { Objects.requireNonNull(acls, "acls == null"); boolean set = !acls.equals(updatedList != null ? updatedList : originalList); changed |= set; diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDirectoryInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDirectoryInfo.java index 3657d5ee68b6..7f489b528f26 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDirectoryInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDirectoryInfo.java @@ -18,7 +18,7 @@ package org.apache.hadoop.ozone.om.helpers; import com.google.common.collect.ImmutableList; -import java.util.List; +import java.util.Collection; import java.util.Map; import java.util.Objects; import net.jcip.annotations.Immutable; @@ -154,7 +154,7 @@ public Builder setModificationTime(long newModificationTime) { return this; } - public Builder setAcls(List listOfAcls) { + public Builder setAcls(Collection listOfAcls) { this.acls.addAll(listOfAcls); return this; } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index dcb4b1baafba..4d8d3d4c7eb6 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableMap; import jakarta.annotation.Nullable; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -617,7 +618,7 @@ public Builder setFileEncryptionInfo(FileEncryptionInfo feInfo) { return this; } - public Builder setAcls(List listOfAcls) { + public Builder setAcls(Collection listOfAcls) { if (listOfAcls != null) { this.acls.set(listOfAcls); } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java index 3dae69f7110d..6cfb905a68e2 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java @@ -24,6 +24,8 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.function.Predicate; @@ -33,7 +35,6 @@ import org.apache.hadoop.ozone.om.OmConfig; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OzoneAclInfo; -import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType; import org.apache.hadoop.ozone.security.acl.RequestContext; import org.apache.hadoop.security.UserGroupInformation; @@ -85,26 +86,6 @@ public static List getAclList(UserGroupInformation ugi, ACLType userPr return listOfAcls; } - /** - * Helper function to get acl list for one user/group. - * - * @param identityName - * @param type - * @param aclList - * @return list of OzoneAcls - * */ - public static List filterAclList(String identityName, - IAccessAuthorizer.ACLIdentityType type, List aclList) { - - if (aclList == null || aclList.isEmpty()) { - return new ArrayList<>(); - } - - List retList = aclList.stream().filter(acl -> acl.getType() == type - && acl.getName().equals(identityName)).collect(Collectors.toList()); - return retList; - } - private static boolean checkAccessInAcl(OzoneAcl a, UserGroupInformation ugi, ACLType aclToCheck) { switch (a.getType()) { @@ -163,7 +144,7 @@ public static boolean inheritDefaultAcls(AclListBuilder acls, * @param scope scope applied to inherited ACL * @return true if any ACL was inherited from parent, false otherwise */ - public static boolean inheritDefaultAcls(List acls, + public static boolean inheritDefaultAcls(Collection acls, List parentAcls, OzoneAcl.AclScope scope) { return inheritDefaultAcls(acl -> addAcl(acls, acl), parentAcls, scope); } @@ -219,20 +200,19 @@ public static List toProtobuf(List protoAcls) { * Add an OzoneAcl to existing list of OzoneAcls. * @return true if current OzoneAcls are changed, false otherwise. */ - public static boolean addAcl(List existingAcls, OzoneAcl acl) { + public static boolean addAcl(Collection existingAcls, OzoneAcl acl) { if (existingAcls == null || acl == null) { return false; } - for (int i = 0; i < existingAcls.size(); i++) { - final OzoneAcl a = existingAcls.get(i); - if (a.getName().equals(acl.getName()) && - a.getType().equals(acl.getType()) && - a.getAclScope().equals(acl.getAclScope())) { + for (Iterator i = existingAcls.iterator(); i.hasNext();) { + final OzoneAcl a = i.next(); + if (a.sameNameTypeScope(acl)) { final OzoneAcl updated = a.add(acl); final boolean changed = !Objects.equals(updated, a); if (changed) { - existingAcls.set(i, updated); + i.remove(); + existingAcls.add(updated); } return changed; } @@ -242,7 +222,7 @@ public static boolean addAcl(List existingAcls, OzoneAcl acl) { return true; } - public static boolean addAllAcl(List existingAcls, List acls) { + public static boolean addAllAcl(Collection existingAcls, Collection acls) { // TOOD optimize boolean changed = false; for (OzoneAcl acl : acls) { @@ -255,22 +235,21 @@ public static boolean addAllAcl(List existingAcls, List acls * remove OzoneAcl from existing list of OzoneAcls. * @return true if current OzoneAcls are changed, false otherwise. */ - public static boolean removeAcl(List existingAcls, OzoneAcl acl) { + static boolean removeAcl(Collection existingAcls, OzoneAcl acl) { if (existingAcls == null || existingAcls.isEmpty() || acl == null) { return false; } - for (int i = 0; i < existingAcls.size(); i++) { - final OzoneAcl a = existingAcls.get(i); - if (a.getName().equals(acl.getName()) && - a.getType().equals(acl.getType()) && - a.getAclScope().equals(acl.getAclScope())) { + for (Iterator i = existingAcls.iterator(); i.hasNext();) { + final OzoneAcl a = i.next(); + if (a.sameNameTypeScope(acl)) { final OzoneAcl updated = a.remove(acl); final boolean changed = !Objects.equals(updated, a); if (updated.isEmpty()) { - existingAcls.remove(i); + i.remove(); } else if (changed) { - existingAcls.set(i, updated); + i.remove(); + existingAcls.add(updated); } return changed; } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/IAccessAuthorizer.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/IAccessAuthorizer.java index 8a07bab606b0..a9d59c635918 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/IAccessAuthorizer.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/IAccessAuthorizer.java @@ -91,8 +91,8 @@ enum ACLType { NONE, ASSUME_ROLE; // ability to create STS tokens - private static int length = ACLType.values().length; static { + final int length = values().length; if (length > 16) { // must update getAclBytes(..) and other code throw new AssertionError("BUG: Length = " + length @@ -100,20 +100,6 @@ enum ACLType { } } - private static ACLType[] vals = ACLType.values(); - - public static int getNoOfAcls() { - return length; - } - - public static ACLType getAclTypeFromOrdinal(int ordinal) { - if (ordinal > length - 1 && ordinal > -1) { - throw new IllegalArgumentException("Ordinal greater than array length" + - ". ordinal:" + ordinal); - } - return vals[ordinal]; - } - /** * Returns the ACL rights based on passed in String. * diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index 7967007263e7..3652b7c44901 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -42,10 +42,12 @@ import java.util.EnumSet; import java.util.HashMap; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import org.apache.commons.lang3.tuple.Pair; @@ -328,12 +330,11 @@ public EncryptedKeyVersion run() throws IOException { return edek; } - protected List getAclsForKey(KeyArgs keyArgs, + protected Set getAclsForKey(KeyArgs keyArgs, OmBucketInfo bucketInfo, OMFileRequest.OMPathInfo omPathInfo, PrefixManager prefixManager, OmConfig config) throws OMException { - List acls = new ArrayList<>(); - acls.addAll(getDefaultAclList(createUGIForApi(), config)); + final Set acls = new LinkedHashSet<>(getDefaultAclList(createUGIForApi(), config)); if (!keyArgs.getAclsList().isEmpty() && !config.ignoreClientACLs()) { acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); } @@ -352,7 +353,6 @@ protected List getAclsForKey(KeyArgs keyArgs, if (prefixInfo != null) { if (OzoneAclUtil.inheritDefaultAcls(acls, prefixInfo.getAcls(), ACCESS)) { // Remove the duplicates - acls = acls.stream().distinct().collect(Collectors.toList()); return acls; } } @@ -363,7 +363,6 @@ protected List getAclsForKey(KeyArgs keyArgs, // prefix are not set if (omPathInfo != null) { if (OzoneAclUtil.inheritDefaultAcls(acls, omPathInfo.getAcls(), ACCESS)) { - acls = acls.stream().distinct().collect(Collectors.toList()); return acls; } } @@ -372,12 +371,10 @@ protected List getAclsForKey(KeyArgs keyArgs, // parent-dir are not set. if (bucketInfo != null) { if (OzoneAclUtil.inheritDefaultAcls(acls, bucketInfo.getAcls(), ACCESS)) { - acls = acls.stream().distinct().collect(Collectors.toList()); return acls; } } - acls = acls.stream().distinct().collect(Collectors.toList()); return acls; } @@ -389,12 +386,11 @@ protected List getAclsForKey(KeyArgs keyArgs, * @param config * @return Acls which inherited parent DEFAULT and keyArgs ACCESS acls. */ - protected List getAclsForDir(KeyArgs keyArgs, OmBucketInfo bucketInfo, + protected Set getAclsForDir(KeyArgs keyArgs, OmBucketInfo bucketInfo, OMFileRequest.OMPathInfo omPathInfo, OmConfig config) throws OMException { // Acls inherited from parent or bucket will convert to DEFAULT scope - List acls = new ArrayList<>(); // add default ACLs - acls.addAll(getDefaultAclList(createUGIForApi(), config)); + final Set acls = new LinkedHashSet<>(getDefaultAclList(createUGIForApi(), config)); // Inherit DEFAULT acls from parent-dir if (omPathInfo != null) { @@ -411,7 +407,6 @@ protected List getAclsForDir(KeyArgs keyArgs, OmBucketInfo bucketInfo, if (!keyArgs.getAclsList().isEmpty() && !config.ignoreClientACLs()) { acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); } - acls = acls.stream().distinct().collect(Collectors.toList()); return acls; } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketAclHandler.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketAclHandler.java index ace1110791ac..bc9e9dca2818 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketAclHandler.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketAclHandler.java @@ -38,7 +38,6 @@ import org.apache.hadoop.ozone.client.OzoneBucket; import org.apache.hadoop.ozone.client.OzoneVolume; import org.apache.hadoop.ozone.om.exceptions.OMException; -import org.apache.hadoop.ozone.om.helpers.OzoneAclUtil; import org.apache.hadoop.ozone.s3.endpoint.S3BucketAcl.Grant; import org.apache.hadoop.ozone.s3.exception.OS3Exception; import org.apache.hadoop.ozone.s3.exception.S3ErrorTable; @@ -181,8 +180,10 @@ Response handlePutRequest(S3RequestContext context, String bucketName, InputStre if (!currentAclsOnVolume.isEmpty()) { for (OzoneAcl acl : acls) { if (acl.getAclScope() == ACCESS) { - aclsToRemoveOnVolume.addAll(OzoneAclUtil.filterAclList( - acl.getName(), acl.getType(), currentAclsOnVolume)); + currentAclsOnVolume.stream() + .filter(a -> a.getType() == acl.getType()) + .filter(a -> a.getName().equals(acl.getName())) + .forEach(aclsToRemoveOnVolume::add); } } for (OzoneAcl acl : aclsToRemoveOnVolume) {