Browse Source

Optimize FLS/DLS setup in IndicePermission authz (#77832)

This change optimizes the creation and tracking of FieldPermissions
and DocumentLevelPermissions in IndiciesPermission.authorize so that
the method executes more quickly when dealing with large numbers of
indices that do not make use of FLS/DLS

The core of this change is a recognition that
1. Most usage of Elasticsearch does not rely on DLS/FLS and therefore
   the FieldPermissions and DocumentLevelPermissions objects will be
   the default/allow-all objects only.
2. In cases where DLS/FLS are used, most security configurations will
   have a single set of DLS/FLS permissions per index

However, prior to this change the internal data structures were
optimized for cases where there were multiple FLS/DLS rules to merge
and apply. Performance for the overwhelming majority of use cases can
be improved by optimizing for the single-rule scenario and treating
the mergine of DLS/FLS rules as an exceptional case.
Tim Vernum 4 years ago
parent
commit
a8c25fd596

+ 37 - 12
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

@@ -383,21 +383,45 @@ public final class IndicesPermission {
                     if (actionCheck || bwcMappingActionCheck) {
                     if (actionCheck || bwcMappingActionCheck) {
                         // propagate DLS and FLS permissions over the concrete indices
                         // propagate DLS and FLS permissions over the concrete indices
                         for (String index : concreteIndices) {
                         for (String index : concreteIndices) {
-                            Set<FieldPermissions> fieldPermissions = fieldPermissionsByIndex.computeIfAbsent(index, (k) -> new HashSet<>());
-                            fieldPermissionsByIndex.put(resource.name, fieldPermissions);
-                            fieldPermissions.add(group.getFieldPermissions());
+                            final Set<FieldPermissions> fieldPermissions = fieldPermissionsByIndex.compute(index, (k, existingSet) -> {
+                                if (existingSet == null) {
+                                    // Most indices rely on the default (empty) field permissions object, so we optimize for that case
+                                    // Using an immutable single item set is significantly faster because it avoids any of the hashing
+                                    // and backing set creation.
+                                    return Set.of(group.getFieldPermissions());
+                                } else if (existingSet.size() == 1) {
+                                    FieldPermissions fp = group.getFieldPermissions();
+                                    if (existingSet.contains(fp)) {
+                                        return existingSet;
+                                    }
+                                    // This index doesn't have a single field permissions object, replace the singleton with a real Set
+                                    final Set<FieldPermissions> hashSet = new HashSet<>(existingSet);
+                                    hashSet.add(fp);
+                                    return hashSet;
+                                } else {
+                                    existingSet.add(group.getFieldPermissions());
+                                    return existingSet;
+                                }
+                            });
 
 
-                            DocumentLevelPermissions permissions =
-                                    roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions());
-                            roleQueriesByIndex.putIfAbsent(resource.name, permissions);
+                            DocumentLevelPermissions docPermissions;
                             if (group.hasQuery()) {
                             if (group.hasQuery()) {
-                                permissions.addAll(group.getQuery());
+                                docPermissions = roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions());
+                                docPermissions.addAll(group.getQuery());
                             } else {
                             } else {
                                 // if more than one permission matches for a concrete index here and if
                                 // if more than one permission matches for a concrete index here and if
                                 // a single permission doesn't have a role query then DLS will not be
                                 // a single permission doesn't have a role query then DLS will not be
                                 // applied even when other permissions do have a role query
                                 // applied even when other permissions do have a role query
-                                permissions.setAllowAll();
+                                docPermissions = DocumentLevelPermissions.ALLOW_ALL;
+                                // don't worry about what's already there - just overwrite it, it avoids doing a 2nd hash lookup.
+                                roleQueriesByIndex.put(index, docPermissions);
+                            }
+
+                            if (index.equals(resource.name) == false) {
+                                fieldPermissionsByIndex.put(resource.name, fieldPermissions);
+                                roleQueriesByIndex.put(resource.name, docPermissions);
                             }
                             }
+
                         }
                         }
                         if (false == actionCheck) {
                         if (false == actionCheck) {
                             for (String privilegeName : group.privilege.name()) {
                             for (String privilegeName : group.privilege.name()) {
@@ -547,6 +571,11 @@ public final class IndicesPermission {
 
 
     private static class DocumentLevelPermissions {
     private static class DocumentLevelPermissions {
 
 
+        public static final DocumentLevelPermissions ALLOW_ALL = new DocumentLevelPermissions();
+        static {
+            ALLOW_ALL.allowAll = true;
+        }
+
         private Set<BytesReference> queries = null;
         private Set<BytesReference> queries = null;
         private boolean allowAll = false;
         private boolean allowAll = false;
 
 
@@ -562,9 +591,5 @@ public final class IndicesPermission {
         private boolean isAllowAll() {
         private boolean isAllowAll() {
             return allowAll;
             return allowAll;
         }
         }
-
-        private void setAllowAll() {
-            this.allowAll = true;
-        }
     }
     }
 }
 }