[GitHub] [accumulo] dlmarion commented on a diff in pull request #3109: Check ACLs on ZNode contain expected regardless of order, improve error msg

[GitBox <gi...@apache.org>]

dlmarion commented on code in PR #3109:
URL: https://github.com/apache/accumulo/pull/3109#discussion_r1043277307


##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader9to10.java:
##########
@@ -165,8 +165,10 @@ private void validateACLs(ServerContext context) {
 
           if (((path.equals(Constants.ZROOT) || path.equals(Constants.ZROOT + Constants.ZINSTANCES))
               && !acls.equals(ZooDefs.Ids.OPEN_ACL_UNSAFE))
-              || (!privateWithAuth.equals(acls) && !publicWithAuth.equals(acls))) {
-            log.error("ZNode at {} has unexpected ACL: {}", path, acls);
+              || (!acls.containsAll(privateWithAuth) && !acls.containsAll(publicWithAuth))) {

Review Comment:
   Customer ran into an issue testing the 2.1 upgrade. Manual intervention was performed and ACLs were modified before I got involved. At the point I was involved user had set acls on the znodes manually using the instructions in the user guide. The order of the ACLs in the setAcl command appeared "correct", but this check was still failing. Customer applied this patch and the upgrade got past this step. So, either the order of the ACLs in the setACL commands was incorrect, or getAcl could return them in a different order. The objects being compared are Lists, so equals checks the order. This was an attempt to perform an order independent comparison without creating new Set objects from the Lists. If having other ACLs present is an issue, then I can modify to create Sets and call `equals`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org