perf: Reduce Cost Of Sync Process (#854)

* perf: use a Set for the collection of roles With a large number of roles the calculation of authorizations gets expensive doing a `disjoint` on lists. * perf: reduce cost of set usage and use more sets Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
spinnaker · Jul 28, 2021 · b42ac29 · b42ac29
1 parent 361bcf4
commit b42ac29
Show file tree

Hide file tree

Showing 20 changed files with 173 additions and 93 deletions.
diff --git a/...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy
@@ -79,7 +79,7 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification {
             applications: [
                 new Application(name: "abc-def",
                                 permissions: Permissions.factory([
-                                    (Authorization.READ): ["testRole"]
+                                    (Authorization.READ): ["testRole"] as Set
                                 ])),
             ],
             roles: [new Role("testRole")]

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Account.java
@@ -26,7 +26,9 @@
 
 @Component
 @Data
-@EqualsAndHashCode(callSuper = false)
+@EqualsAndHashCode(
+    callSuper = false,
+    of = {"resourceType", "cloudProvider", "name"})
 public class Account extends BaseAccessControlled<Account> implements Viewable {
   final ResourceType resourceType = ResourceType.ACCOUNT;
 

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Application.java
@@ -29,7 +29,9 @@
 
 @Component
 @Data
-@EqualsAndHashCode(callSuper = false)
+@EqualsAndHashCode(
+    callSuper = false,
+    of = {"resourceType", "name"})
 public class Application extends BaseAccessControlled<Application> implements Viewable {
   final ResourceType resourceType = ResourceType.APPLICATION;
 

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BaseAccessControlled.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BaseAccessControlled.java
@@ -17,7 +17,7 @@
 package com.netflix.spinnaker.fiat.model.resources;
 
 import com.netflix.spinnaker.fiat.model.Authorization;
-import java.util.List;
+import java.util.Set;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
@@ -31,7 +31,7 @@ public abstract class BaseAccessControlled<R extends BaseAccessControlled>
    * permissions.
    */
   @SuppressWarnings("unchecked")
-  public <T extends BaseAccessControlled> T setRequiredGroupMembership(List<String> membership) {
+  public <T extends BaseAccessControlled> T setRequiredGroupMembership(Set<String> membership) {
     if (membership == null || membership.isEmpty()) {
       return (T) this;
     }

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/BuildService.java
@@ -26,7 +26,9 @@
 
 @Component
 @Data
-@EqualsAndHashCode(callSuper = false)
+@EqualsAndHashCode(
+    callSuper = false,
+    of = {"resourceType", "name"})
 public class BuildService implements Resource.AccessControlled, Viewable {
 
   private final ResourceType resourceType = ResourceType.BUILD_SERVICE;

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Permissions.java
@@ -24,39 +24,38 @@
 import com.netflix.spinnaker.fiat.model.Authorization;
 import java.util.*;
 import java.util.stream.Collectors;
-import lombok.EqualsAndHashCode;
-import lombok.ToString;
 import lombok.val;
 
 /**
  * Representation of authorization configuration for a resource. This object is immutable, which
  * makes it challenging when working with Jackson's {@code ObjectMapper} and Spring's
  * {@code @ConfigurationProperties}. The {@link Builder} is a helper class for the latter use case.
  */
-@ToString
-@EqualsAndHashCode
 public class Permissions {
 
   public static final Permissions EMPTY = Builder.fromMap(Collections.emptyMap());
 
-  private final Map<Authorization, List<String>> permissions;
+  private final Map<Authorization, Set<String>> permissions;
 
-  private Permissions(Map<Authorization, List<String>> p) {
+  private final int hashCode;
+
+  private Permissions(Map<Authorization, Set<String>> p) {
     this.permissions = Collections.unmodifiableMap(p);
+    this.hashCode = Objects.hash(this.permissions);
   }
 
   /**
    * Specifically here for Jackson deserialization. Sends data through the {@link Builder} in order
    * to sanitize the input data (just in case).
    */
   @JsonCreator
-  public static Permissions factory(Map<Authorization, List<String>> data) {
+  public static Permissions factory(Map<Authorization, Set<String>> data) {
     return new Builder().set(data).build();
   }
 
   /** Here specifically for Jackson serialization. */
   @JsonValue
-  private Map<Authorization, List<String>> getPermissions() {
+  private Map<Authorization, Set<String>> getPermissions() {
     return permissions;
   }
 
@@ -84,11 +83,15 @@ public boolean isAuthorized(Set<Role> userRoles) {
   }
 
   public Set<Authorization> getAuthorizations(Set<Role> userRoles) {
-    val r = userRoles.stream().map(Role::getName).collect(Collectors.toList());
-    return getAuthorizations(r);
+    val r = userRoles.stream().map(Role::getName).collect(Collectors.toSet());
+    return getAuthorizationsFromRoles(r);
   }
 
   public Set<Authorization> getAuthorizations(List<String> userRoles) {
+    return getAuthorizationsFromRoles(new LinkedHashSet<>(userRoles));
+  }
+
+  private Set<Authorization> getAuthorizationsFromRoles(Set<String> userRoles) {
     if (!isRestricted()) {
       return Authorization.ALL;
     }
@@ -99,14 +102,30 @@ public Set<Authorization> getAuthorizations(List<String> userRoles) {
         .collect(Collectors.toSet());
   }
 
-  public List<String> get(Authorization a) {
-    return permissions.getOrDefault(a, new ArrayList<>());
+  public Set<String> get(Authorization a) {
+    return permissions.getOrDefault(a, new HashSet<>());
   }
 
-  public Map<Authorization, List<String>> unpack() {
+  public Map<Authorization, Set<String>> unpack() {
     return Arrays.stream(Authorization.values()).collect(toMap(identity(), this::get));
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    Permissions that = (Permissions) o;
+    return Objects.equals(this.permissions, that.permissions);
+  }
+
+  public int hashCode() {
+    return hashCode;
+  }
+
+  public String toString() {
+    return "Permissions(permissions=" + this.getPermissions() + ")";
+  }
+
   /**
    * This is a helper class for setting up an immutable Permissions object. It also acts as the
    * target Java Object for Spring's ConfigurationProperties deserialization.
@@ -117,10 +136,10 @@ public Map<Authorization, List<String>> unpack() {
    *
    * <p>Group/Role names are trimmed of whitespace and lowercased.
    */
-  public static class Builder extends LinkedHashMap<Authorization, List<String>> {
+  public static class Builder extends LinkedHashMap<Authorization, Set<String>> {
 
-    private static Permissions fromMap(Map<Authorization, List<String>> authConfig) {
-      final Map<Authorization, List<String>> perms = new EnumMap<>(Authorization.class);
+    private static Permissions fromMap(Map<Authorization, Set<String>> authConfig) {
+      final Map<Authorization, Set<String>> perms = new EnumMap<>(Authorization.class);
       for (Authorization auth : Authorization.values()) {
         Optional.ofNullable(authConfig.get(auth))
             .map(
@@ -129,31 +148,31 @@ private static Permissions fromMap(Map<Authorization, List<String>> authConfig)
                         .map(String::trim)
                         .filter(s -> !s.isEmpty())
                         .map(String::toLowerCase)
-                        .collect(Collectors.toList()))
+                        .collect(Collectors.toSet()))
             .filter(g -> !g.isEmpty())
-            .map(Collections::unmodifiableList)
+            .map(Collections::unmodifiableSet)
             .ifPresent(roles -> perms.put(auth, roles));
       }
       return new Permissions(perms);
     }
 
     @JsonCreator
-    public static Builder factory(Map<Authorization, List<String>> data) {
+    public static Builder factory(Map<Authorization, Set<String>> data) {
       return new Builder().set(data);
     }
 
-    public Builder set(Map<Authorization, List<String>> p) {
+    public Builder set(Map<Authorization, Set<String>> p) {
       this.clear();
       this.putAll(p);
       return this;
     }
 
     public Builder add(Authorization a, String group) {
-      this.computeIfAbsent(a, ignored -> new ArrayList<>()).add(group);
+      this.computeIfAbsent(a, ignored -> new LinkedHashSet<>()).add(group);
       return this;
     }
 
-    public Builder add(Authorization a, List<String> groups) {
+    public Builder add(Authorization a, Set<String> groups) {
       groups.forEach(group -> add(a, group));
       return this;
     }

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ResourceType.java
@@ -21,6 +21,7 @@
 public class ResourceType {
 
   private final String name;
+  private final int hashCode;
 
   public static ResourceType ACCOUNT = new ResourceType("account");
   public static ResourceType APPLICATION = new ResourceType("application");
@@ -30,7 +31,8 @@ public class ResourceType {
   public static ResourceType BUILD_SERVICE = new ResourceType("build_service");
 
   public ResourceType(String name) {
-    this.name = name;
+    this.name = name.toLowerCase();
+    this.hashCode = Objects.hash(name);
   }
 
   // TODO(ttomsu): This is Redis-specific, so it probably shouldn't go here.
@@ -56,12 +58,12 @@ public boolean equals(Object o) {
     if (this == o) return true;
     if (o == null || getClass() != o.getClass()) return false;
     ResourceType that = (ResourceType) o;
-    return Objects.equals(name.toLowerCase(), that.name.toLowerCase());
+    return Objects.equals(this.name, that.name);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(name.toLowerCase());
+    return hashCode;
   }
 
   @Override
@@ -70,6 +72,6 @@ public String toString() {
   }
 
   public String keySuffix() {
-    return name.toLowerCase() + "s";
+    return name + "s";
   }
 }
diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/Role.java
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.fiat.model.resources;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import java.util.Objects;
 import java.util.Set;
 import javax.annotation.Nonnull;
 import lombok.Data;
@@ -26,13 +27,10 @@
 import org.springframework.util.StringUtils;
 
 @Component
-@Data
-@EqualsAndHashCode(of = "name")
-@NoArgsConstructor
 public class Role implements Resource, Viewable {
 
-  private final ResourceType resourceType = ResourceType.ROLE;
   private String name;
+  private int hashCode;
 
   public enum Source {
     EXTERNAL,
@@ -44,18 +42,62 @@ public enum Source {
 
   private Source source;
 
+  public Role() {}
+
   public Role(String name) {
     this.setName(name);
   }
 
+  public ResourceType getResourceType() {
+    return ResourceType.ROLE;
+  }
+
+  public String getName() {
+    return this.name;
+  }
+
   public Role setName(@Nonnull String name) {
     if (!StringUtils.hasText(name)) {
       throw new IllegalArgumentException("name cannot be empty");
     }
     this.name = name.toLowerCase();
+    this.hashCode = Objects.hash(ResourceType.ROLE, this.name);
+    return this;
+  }
+
+  public Source getSource() {
+    return this.source;
+  }
+
+  public Role setSource(Source source) {
+    this.source = source;
     return this;
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o == null || getClass() != o.getClass()) return false;
+    Role that = (Role) o;
+    return Objects.equals(this.name, that.name);
+  }
+
+  @Override
+  public int hashCode() {
+    return hashCode;
+  }
+
+  @Override
+  public String toString() {
+    return "Role(resourceType="
+        + this.getResourceType()
+        + ", name="
+        + this.getName()
+        + ", source="
+        + this.getSource()
+        + ")";
+  }
+
   @JsonIgnore
   @Override
   public View getView(Set<Role> ignored, boolean isAdmin) {

diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ServiceAccount.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/resources/ServiceAccount.java
@@ -18,10 +18,9 @@
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.netflix.spinnaker.fiat.model.UserPermission;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
+import java.util.*;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import lombok.Data;
 import lombok.EqualsAndHashCode;
 import lombok.NoArgsConstructor;
@@ -31,12 +30,14 @@
 
 @Component
 @Data
-@EqualsAndHashCode(callSuper = false)
+@EqualsAndHashCode(
+    callSuper = false,
+    of = {"resourceType", "name"})
 public class ServiceAccount implements Resource, Viewable {
   private final ResourceType resourceType = ResourceType.SERVICE_ACCOUNT;
 
   private String name;
-  private List<String> memberOf = new ArrayList<>();
+  private Set<String> memberOf = new LinkedHashSet<>();
 
   public UserPermission toUserPermission() {
     val roles =
@@ -48,11 +49,13 @@ public UserPermission toUserPermission() {
   }
 
   public ServiceAccount setMemberOf(List<String> membership) {
-    if (membership == null) {
-      membership = new ArrayList<>();
-    }
     memberOf =
-        membership.stream().map(String::trim).map(String::toLowerCase).collect(Collectors.toList());
+        Optional.ofNullable(membership)
+            .map(Collection::stream)
+            .orElseGet(Stream::empty)
+            .map(String::trim)
+            .map(String::toLowerCase)
+            .collect(Collectors.toSet());
     return this;
   }
 
@@ -71,7 +74,7 @@ public static class View extends BaseView {
 
     public View(ServiceAccount serviceAccount) {
       this.name = serviceAccount.name;
-      this.memberOf = serviceAccount.memberOf;
+      this.memberOf = new ArrayList<>(serviceAccount.memberOf);
     }
   }
 }