refactor(kubernetes): Reduce API surface of KubernetesKindProperties (#…

…4029)

* refactor(kubernetes): Reduce API surface of KubernetesKindProperties

Only the statically-defined kinds are allowed to set isDynamic to false,
or hasClusterRelationship to true. In order to make this statically
true, make the full constructor private and expose a factory method
that doesn't allow those properties to be set for use outside the class.

(Some of the usages of the constructor in tests *did* set isDynamic and
hasClusterRelationship, but these were just testing the API that was
available; now that these are not exposed outside the class the tests
can just use the factory method.)

* refactor(kubernetes): Simplify getRegisteredKinds

The only time we ever use the getRegisteredKinds method in
KubernetesKindRegistry, what we actually want is a list of
the global kinds (so we can set up the core caching agent).

We do this by getting all the kinds and filtering on the
isDynamic property; it would be simpler to just return the
global kinds in the first place. A kind is global if and only
if it is not dynamic.

* refactor(kubernetes): Remove isDynamic property from kind properties

The prior commit removed the last usage of isDynamic in the
KubernetesKindProperties class. Remove the member variable from
the class.

* refactor(kubernetes): Move defaulting logic to account registry

In preparation for a fix to reduce live CRD lookups, move the defaulting
of unknown kinds from the GlobalKubernetesKindRegistry to the account
KubernetesKindRegistry.

This doesn't change the result of calling getRegisteredKind on the
account registry, but moves where the defaulting happens.

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/agent/KubernetesCoreCachingAgent.java

@@ -62,8 +62,7 @@ public Collection<AgentDataType> getProvidedDataTypes() {
 
   @Override
   protected List<KubernetesKind> primaryKinds() {
-    return credentials.getKindRegistry().getRegisteredKinds().stream()
-        .filter(k -> !k.isDynamic())
+    return credentials.getKindRegistry().getGlobalKinds().stream()
         .map(KubernetesKindProperties::getKubernetesKind)
         .filter(credentials::isValidKind)
         .collect(Collectors.toList());

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesResourceProperties.java

@@ -70,8 +70,7 @@ public static KubernetesResourceProperties fromCustomResource(
               "Dynamically registering {}, (namespaced: {})",
               kubernetesKind.toString(),
               customResource.isNamespaced());
-          return new KubernetesKindProperties(
-              kubernetesKind, customResource.isNamespaced(), false, true);
+          return KubernetesKindProperties.create(kubernetesKind, customResource.isNamespaced());
         });
 
     KubernetesHandler handler =

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/GlobalKubernetesKindRegistry.java

@@ -16,10 +16,10 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest;
 
+import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
+import java.util.Collection;
+import java.util.Optional;
 import javax.annotation.Nonnull;
 
 /**
@@ -28,14 +28,14 @@
  * immutable, they can be shared across threads without need for further synchronization.
  */
 public class GlobalKubernetesKindRegistry {
-  private final Map<KubernetesKind, KubernetesKindProperties> nameMap;
+  private final ImmutableMap<KubernetesKind, KubernetesKindProperties> nameMap;
 
   /**
    * Creates a {@link GlobalKubernetesKindRegistry} populated with the supplied {@link
    * KubernetesKindProperties}.
    */
   public GlobalKubernetesKindRegistry(
-      @Nonnull List<KubernetesKindProperties> kubernetesKindProperties) {
+      @Nonnull Collection<KubernetesKindProperties> kubernetesKindProperties) {
     ImmutableMap.Builder<KubernetesKind, KubernetesKindProperties> mapBuilder =
         new ImmutableMap.Builder<>();
     kubernetesKindProperties.forEach(kp -> mapBuilder.put(kp.getKubernetesKind(), kp));
@@ -45,21 +45,16 @@ public GlobalKubernetesKindRegistry(
   /**
    * Searches the registry for a {@link KubernetesKindProperties} with the supplied {@link
    * KubernetesKind}. If the kind has been registered, returns the {@link KubernetesKindProperties}
-   * that were registered for the kind; otherwise, returns a {@link KubernetesKindProperties}
-   * containing default values for all properties.
+   * that were registered for the kind; otherwise, returns an empty {@link Optional}.
    */
   @Nonnull
-  public KubernetesKindProperties getRegisteredKind(@Nonnull KubernetesKind kind) {
-    KubernetesKindProperties result = nameMap.get(kind);
-    if (result != null) {
-      return result;
-    }
-    return KubernetesKindProperties.withDefaultProperties(kind);
+  public Optional<KubernetesKindProperties> getRegisteredKind(@Nonnull KubernetesKind kind) {
+    return Optional.ofNullable(nameMap.get(kind));
   }
 
   /** Returns a list of all registered kinds */
   @Nonnull
-  public List<KubernetesKindProperties> getRegisteredKinds() {
-    return new ArrayList<>(nameMap.values());
+  public ImmutableCollection<KubernetesKindProperties> getRegisteredKinds() {
+    return nameMap.values();
   }
 }

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindProperties.java

@@ -19,73 +19,71 @@
 import com.google.common.collect.ImmutableList;
 import java.util.List;
 import javax.annotation.Nonnull;
+import javax.annotation.ParametersAreNonnullByDefault;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
 @EqualsAndHashCode
+@ParametersAreNonnullByDefault
 @Slf4j
 public class KubernetesKindProperties {
   public static List<KubernetesKindProperties> getGlobalKindProperties() {
     return ImmutableList.of(
-        new KubernetesKindProperties(KubernetesKind.API_SERVICE, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.CLUSTER_ROLE, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.CLUSTER_ROLE_BINDING, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.CONFIG_MAP, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.CONTROLLER_REVISION, true, false, false),
-        new KubernetesKindProperties(
-            KubernetesKind.CUSTOM_RESOURCE_DEFINITION, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.CRON_JOB, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.DAEMON_SET, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.DEPLOYMENT, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.EVENT, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.HORIZONTAL_POD_AUTOSCALER, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.INGRESS, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.JOB, true, false, false),
-        new KubernetesKindProperties(
-            KubernetesKind.MUTATING_WEBHOOK_CONFIGURATION, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.NAMESPACE, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.NETWORK_POLICY, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.PERSISTENT_VOLUME, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.PERSISTENT_VOLUME_CLAIM, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.POD, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.POD_PRESET, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.POD_SECURITY_POLICY, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.POD_DISRUPTION_BUDGET, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.REPLICA_SET, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.ROLE, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.ROLE_BINDING, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.SECRET, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.SERVICE, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.SERVICE_ACCOUNT, true, false, false),
-        new KubernetesKindProperties(KubernetesKind.STATEFUL_SET, true, true, false),
-        new KubernetesKindProperties(KubernetesKind.STORAGE_CLASS, false, false, false),
-        new KubernetesKindProperties(
-            KubernetesKind.VALIDATING_WEBHOOK_CONFIGURATION, false, false, false),
-        new KubernetesKindProperties(KubernetesKind.NONE, true, false, false));
+        new KubernetesKindProperties(KubernetesKind.API_SERVICE, false, false),
+        new KubernetesKindProperties(KubernetesKind.CLUSTER_ROLE, false, false),
+        new KubernetesKindProperties(KubernetesKind.CLUSTER_ROLE_BINDING, false, false),
+        new KubernetesKindProperties(KubernetesKind.CONFIG_MAP, true, false),
+        new KubernetesKindProperties(KubernetesKind.CONTROLLER_REVISION, true, false),
+        new KubernetesKindProperties(KubernetesKind.CUSTOM_RESOURCE_DEFINITION, false, false),
+        new KubernetesKindProperties(KubernetesKind.CRON_JOB, true, false),
+        new KubernetesKindProperties(KubernetesKind.DAEMON_SET, true, true),
+        new KubernetesKindProperties(KubernetesKind.DEPLOYMENT, true, true),
+        new KubernetesKindProperties(KubernetesKind.EVENT, true, false),
+        new KubernetesKindProperties(KubernetesKind.HORIZONTAL_POD_AUTOSCALER, true, false),
+        new KubernetesKindProperties(KubernetesKind.INGRESS, true, true),
+        new KubernetesKindProperties(KubernetesKind.JOB, true, false),
+        new KubernetesKindProperties(KubernetesKind.MUTATING_WEBHOOK_CONFIGURATION, false, false),
+        new KubernetesKindProperties(KubernetesKind.NAMESPACE, false, false),
+        new KubernetesKindProperties(KubernetesKind.NETWORK_POLICY, true, true),
+        new KubernetesKindProperties(KubernetesKind.PERSISTENT_VOLUME, false, false),
+        new KubernetesKindProperties(KubernetesKind.PERSISTENT_VOLUME_CLAIM, true, false),
+        new KubernetesKindProperties(KubernetesKind.POD, true, false),
+        new KubernetesKindProperties(KubernetesKind.POD_PRESET, true, false),
+        new KubernetesKindProperties(KubernetesKind.POD_SECURITY_POLICY, false, false),
+        new KubernetesKindProperties(KubernetesKind.POD_DISRUPTION_BUDGET, true, false),
+        new KubernetesKindProperties(KubernetesKind.REPLICA_SET, true, true),
+        new KubernetesKindProperties(KubernetesKind.ROLE, true, false),
+        new KubernetesKindProperties(KubernetesKind.ROLE_BINDING, true, false),
+        new KubernetesKindProperties(KubernetesKind.SECRET, true, false),
+        new KubernetesKindProperties(KubernetesKind.SERVICE, true, true),
+        new KubernetesKindProperties(KubernetesKind.SERVICE_ACCOUNT, true, false),
+        new KubernetesKindProperties(KubernetesKind.STATEFUL_SET, true, true),
+        new KubernetesKindProperties(KubernetesKind.STORAGE_CLASS, false, false),
+        new KubernetesKindProperties(KubernetesKind.VALIDATING_WEBHOOK_CONFIGURATION, false, false),
+        new KubernetesKindProperties(KubernetesKind.NONE, true, false));
   }
 
   @Nonnull @Getter private final KubernetesKind kubernetesKind;
   @Getter private final boolean isNamespaced;
   // generally reserved for workloads, can be read as "does this belong to a spinnaker cluster?"
   private final boolean hasClusterRelationship;
-  // was this kind found after spinnaker started?
-  @Getter private final boolean isDynamic;
 
-  public KubernetesKindProperties(
-      @Nonnull KubernetesKind kubernetesKind,
-      boolean isNamespaced,
-      boolean hasClusterRelationship,
-      boolean isDynamic) {
+  private KubernetesKindProperties(
+      KubernetesKind kubernetesKind, boolean isNamespaced, boolean hasClusterRelationship) {
     this.kubernetesKind = kubernetesKind;
     this.isNamespaced = isNamespaced;
     this.hasClusterRelationship = hasClusterRelationship;
-    this.isDynamic = isDynamic;
   }
 
-  public static KubernetesKindProperties withDefaultProperties(
-      @Nonnull KubernetesKind kubernetesKind) {
-    return new KubernetesKindProperties(kubernetesKind, true, false, true);
+  public static KubernetesKindProperties withDefaultProperties(KubernetesKind kubernetesKind) {
+    return new KubernetesKindProperties(kubernetesKind, true, false);
+  }
+
+  @Nonnull
+  public static KubernetesKindProperties create(
+      KubernetesKind kubernetesKind, boolean isNamespaced) {
+    return new KubernetesKindProperties(kubernetesKind, isNamespaced, false);
   }
 
   public boolean hasClusterRelationship() {

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/manifest/KubernetesKindRegistry.java

@@ -16,8 +16,7 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest;
 
-import java.util.ArrayList;
-import java.util.List;
+import com.google.common.collect.ImmutableCollection;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Supplier;
@@ -53,7 +52,9 @@ public KubernetesKindProperties getOrRegisterKind(
    * Searches the registry for a {@link KubernetesKindProperties} with the supplied {@link
    * KubernetesKind}. If the kind has been registered, returns the {@link KubernetesKindProperties}
    * that were registered for the kind; otherwise, looks for the kind in the {@link
-   * GlobalKubernetesKindRegistry} and returns the properties found there.
+   * GlobalKubernetesKindRegistry} and returns the properties found there. If the kind is not
+   * registered either globally or in the account, returns a {@link KubernetesKindProperties} with
+   * default properties.
    */
   @Nonnull
   public KubernetesKindProperties getRegisteredKind(@Nonnull KubernetesKind kind) {
@@ -62,15 +63,15 @@ public KubernetesKindProperties getRegisteredKind(@Nonnull KubernetesKind kind)
       return result;
     }
 
-    return globalKindRegistry.getRegisteredKind(kind);
+    return globalKindRegistry
+        .getRegisteredKind(kind)
+        .orElse(KubernetesKindProperties.withDefaultProperties(kind));
   }
 
-  /** Returns a list of all registered kinds */
+  /** Returns a list of all global kinds */
   @Nonnull
-  public List<KubernetesKindProperties> getRegisteredKinds() {
-    List<KubernetesKindProperties> result = new ArrayList<>(kindMap.values());
-    result.addAll(globalKindRegistry.getRegisteredKinds());
-    return result;
+  public ImmutableCollection<KubernetesKindProperties> getGlobalKinds() {
+    return globalKindRegistry.getRegisteredKinds();
   }
 
   @Component

clouddriver-kubernetes-v2/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java

@@ -157,7 +157,7 @@ private KubernetesV2Credentials(
                         () -> {
                           log.info(
                               "Dynamically registering {}, (namespaced: {})", k.toString(), true);
-                          return new KubernetesKindProperties(k, true, false, true);
+                          return KubernetesKindProperties.create(k, true);
                         }))
             .map(KubernetesKindProperties::getKubernetesKind)
             .collect(toImmutableSet());
@@ -245,8 +245,7 @@ private KubernetesV2Credentials(
                                     "Dynamically registering {}, (namespaced: {})",
                                     kind.toString(),
                                     isNamespaced);
-                                return new KubernetesKindProperties(
-                                    kind, isNamespaced, false, true);
+                                return KubernetesKindProperties.create(kind, isNamespaced);
                               });
                         })
                     .map(KubernetesKindProperties::getKubernetesKind)

clouddriver-kubernetes-v2/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/GlobalKubernetesKindRegistrySpec.groovy

@@ -22,8 +22,8 @@ import spock.lang.Subject
 
 class GlobalKubernetesKindRegistrySpec extends Specification {
   static final KubernetesApiGroup CUSTOM_API_GROUP =  KubernetesApiGroup.fromString("test")
-  static final KubernetesKindProperties REPLICA_SET = new KubernetesKindProperties(KubernetesKind.REPLICA_SET, true, true, true)
-  static final KubernetesKindProperties CUSTOM_KIND = new KubernetesKindProperties(KubernetesKind.from("customKind", CUSTOM_API_GROUP), true, true, true)
+  static final KubernetesKindProperties REPLICA_SET = KubernetesKindProperties.create(KubernetesKind.REPLICA_SET, true)
+  static final KubernetesKindProperties CUSTOM_KIND = KubernetesKindProperties.create(KubernetesKind.from("customKind", CUSTOM_API_GROUP), true)
 
   void "an empty registry returns no kinds"() {
     given:
@@ -63,10 +63,10 @@ class GlobalKubernetesKindRegistrySpec extends Specification {
     def properties = kindRegistry.getRegisteredKind(KubernetesKind.from("customKind", CUSTOM_API_GROUP))
 
     then:
-    properties == CUSTOM_KIND
+    properties.get() == CUSTOM_KIND
   }
 
-  void "getRegisteredKind default properties for a kind that has not been registered"() {
+  void "getRegisteredKind returns an empty optional for kinds that have not been registered"() {
     given:
     @Subject GlobalKubernetesKindRegistry kindRegistry = new GlobalKubernetesKindRegistry([
       REPLICA_SET,
@@ -77,6 +77,6 @@ class GlobalKubernetesKindRegistrySpec extends Specification {
     def properties = kindRegistry.getRegisteredKind(KubernetesKind.from("otherKind", CUSTOM_API_GROUP))
 
     then:
-    properties == KubernetesKindProperties.withDefaultProperties(KubernetesKind.from("otherKind", CUSTOM_API_GROUP))
+    !properties.isPresent()
   }
 }

clouddriver-kubernetes-v2/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/description/KubernetesKindPropertiesSpec.groovy

@@ -24,22 +24,20 @@ import spock.lang.Specification
 class KubernetesKindPropertiesSpec extends Specification {
   def "creates and returns the supplied properties"() {
     when:
-    def properties = new KubernetesKindProperties(KubernetesKind.REPLICA_SET, true, true, true)
+    def properties = KubernetesKindProperties.create(KubernetesKind.REPLICA_SET, true)
 
     then:
     properties.getKubernetesKind() == KubernetesKind.REPLICA_SET
     properties.isNamespaced()
-    properties.hasClusterRelationship()
-    properties.isDynamic()
+    !properties.hasClusterRelationship()
 
     when:
-    properties = new KubernetesKindProperties(KubernetesKind.REPLICA_SET, false, false, false)
+    properties = KubernetesKindProperties.create(KubernetesKind.REPLICA_SET, false)
 
     then:
     properties.getKubernetesKind() == KubernetesKind.REPLICA_SET
     !properties.isNamespaced()
     !properties.hasClusterRelationship()
-    !properties.isDynamic()
   }
 
   def "sets default properties to the expected values"() {
@@ -49,15 +47,25 @@ class KubernetesKindPropertiesSpec extends Specification {
     then:
     properties.isNamespaced()
     !properties.hasClusterRelationship()
-    properties.isDynamic()
   }
 
   def "returns expected results for built-in kinds"() {
     when:
     def defaultProperties = KubernetesKindProperties.getGlobalKindProperties()
+    def replicaSetProperties = defaultProperties.stream()
+      .filter({p -> p.getKubernetesKind().equals(KubernetesKind.REPLICA_SET)})
+      .findFirst()
+    def namespaceProperties = defaultProperties.stream()
+      .filter({p -> p.getKubernetesKind().equals(KubernetesKind.NAMESPACE)})
+      .findFirst()
 
     then:
-    defaultProperties.contains(new KubernetesKindProperties(KubernetesKind.REPLICA_SET, true, true, false))
-    defaultProperties.contains(new KubernetesKindProperties(KubernetesKind.NAMESPACE, false, false, false))
+    replicaSetProperties.isPresent()
+    replicaSetProperties.get().isNamespaced()
+    replicaSetProperties.get().hasClusterRelationship()
+
+    namespaceProperties.isPresent()
+    !namespaceProperties.get().isNamespaced()
+    !namespaceProperties.get().hasClusterRelationship()
   }
 }