refactor(kubernetes): Remove extraneous null checks (#4013)

* refactor(kubernetes): Initialize property registry in credentials class

The property registry is owned by the credentials class; rather than
have the agent scheduler call into the class to initialize it, just
have the class handle this in its initialize method.

* refactor(kubernetes): Make AccountResourcePropertyRegistry immutable

AccountResourcePropertyRegistry support a register method, but it is only
used a single time to initialize the registry with the account's
KubernetesResourceProperties.

Instead, require the KubernetesResourceProperties in the constructor,
and remove the register method.

* refactor(kubernetes): Make GlobalResourcePropertyRegistry immutable

GlobalResourcePropertyRegistry has a register method that is only ever
called from the constructor. Remove this method and instead build an
immutable map in the constructor.

* refactor(kubernetes): Update return values from registries to be immutable

Now that both AccountResourcePropertyRegistry and GlobalResourcePropertyRegistry are
immutable, update the public methods on these classes, and the interface to return
immutable collections where approriate. Also add NonNull annotations where appropriate.

* refactor(kubernetes): Add Nonnull annotation to handler kinds

In an upcoming commit, we're going to be storing the KubernetesKind
and SpinnakerKind associated with handlers in an ImmutableMap, which
won't allow null values.

As we already have null objects for each of these (KubernetesKind.NONE
and SpinnakerKind.UNCLASSIFIED) there's no need to ever have these
methods return null.  Add annotations to the interface and
implementations, and update the one implementation returning null
to instead return SpinnakerKind.UNCLASSIFIED.

* refactor(kubernetes): Let KubernetesSpinnakerKindMap initialize itself

It's a bit strange that GlobalResourcePropertyRegistry is responsible for
initializing KubernetesSpinnakerKindMap. (It takes the singleton kind map
in the constructor, mutates it, then forgets about it.) This is a vestige
of the way the code worked before the kind registries were created.

Instead, let KubernetesSpinnakerKindMap handle its own initialization; its
constructor should expect the same list of KubernetesHandlers and should
register the kinds found on each of them.

This also means that we can make the register method private as it's only
called from the constructor. If you were to guess that the next commit would
remove the register method entirely and make the class immutable, you would
probably be correct...

* refactor(kubernetes): Make KubernetesSpinnakerKindMap immutable

Now that KubernetesSpinnakerKindMap handles its own initialization,
the constructor has all the information needed to completely construct
the maps. Create immutable maps in the constructor and remove the
register method.

* refactor(kubernetes): Enforce that defaultProperties are not null

We've added the NonNull annotation to GlobalResourcePropertyRegistry.get
because we fall back to the properties for KubernetesKind.NONE for a
kind that's not in the registry. While it is statically true that there
will be properties for the NONE kind (because
KubernetesUnregisteredCustomResourceHandler handles the NONE kind) it's
a bit confusing to see how this defaulting happens and is liable to break
if one day we remove the NONE kind from that handler.

Instead, let's be more explicit and expect to receive the unregistered custom
resource handler bean in the constructor of the global registry, and set that
as the default to return. Then if we somehow don't have this default value,
autowiring will fail at startup (instead of causing null-pointer exceptions
later).

This makes it much more clear that get() always returns a non-null value,
even though it always did.

* refactor(kubernetes): Remove extraneous null checks

Now that we have statically guaranteed that the return value of
KubernetesKindRegistry.get is non-null, we can remove multiple
extraneous non-null checks from the code.

* style(kubernetes): Use ImmutableSetMultimap to simplify constructor

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/security/KubernetesNamedAccountCredentials.java

@@ -175,7 +175,7 @@ KubernetesV2Credentials buildV2Credentials(
           spectatorRegistry,
           jobExecutor,
           managedAccount,
-          resourcePropertyRegistryFactory.create(),
+          resourcePropertyRegistryFactory,
           kindRegistryFactory.create(),
           getKubeconfigFile(managedAccount));
     }

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/KubernetesV2ProviderSynchronizable.java

@@ -24,7 +24,6 @@
 import com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesConfigurationProperties;
 import com.netflix.spinnaker.clouddriver.kubernetes.security.KubernetesNamedAccountCredentials;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.caching.agent.KubernetesV2CachingAgentDispatcher;
-import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesResourceProperties;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.KubernetesSpinnakerKindMap;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesV2Credentials;
 import com.netflix.spinnaker.clouddriver.security.*;
@@ -156,19 +155,6 @@ private void synchronizeKubernetesV2Provider(Set<KubernetesNamedAccountCredentia
       for (KubernetesNamedAccountCredentials credentials : allAccounts) {
         KubernetesV2Credentials v2Credentials =
             (KubernetesV2Credentials) credentials.getCredentials();
-        v2Credentials
-            .getCustomResources()
-            .forEach(
-                cr -> {
-                  try {
-                    KubernetesResourceProperties properties =
-                        KubernetesResourceProperties.fromCustomResource(
-                            cr, v2Credentials.getKindRegistry());
-                    v2Credentials.getResourcePropertyRegistry().register(properties);
-                  } catch (Exception e) {
-                    log.warn("Error encountered registering {}: ", cr, e);
-                  }
-                });
         v2Credentials.initialize();
         List<Agent> newlyAddedAgents =
             kubernetesV2CachingAgentDispatcher.buildAllCachingAgents(credentials).stream()

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/KubernetesV2SearchProvider.java

@@ -142,12 +142,6 @@ private Map<String, Object> convertKeyToMap(String key) {
           resourcePropertyResolver
               .getResourcePropertyRegistry(infraKey.getAccount())
               .get(infraKey.getKubernetesKind());
-      if (properties == null) {
-        log.warn(
-            "No hydrator for type {}, this is possibly a developer error",
-            infraKey.getKubernetesKind());
-        return null;
-      }
 
       result = properties.getHandler().hydrateSearchResult(infraKey, cacheUtils);
     } else if (parsedKey instanceof Keys.LogicalKey) {

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/view/provider/KubernetesV2AbstractManifestProvider.java

@@ -82,9 +82,6 @@ protected KubernetesV2Manifest buildManifest(
     KubernetesKind kind = manifest.getKind();
 
     KubernetesResourceProperties properties = getRegistry(account).get(kind);
-    if (properties == null) {
-      return null;
-    }
 
     Function<KubernetesManifest, String> lastEventTimestamp =
         (m) -> (String) m.getOrDefault("lastTimestamp", m.getOrDefault("firstTimestamp", "n/a"));

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/view/provider/KubernetesV2ManifestProvider.java

@@ -97,9 +97,6 @@ public List<KubernetesV2Manifest> getClusterAndSortAscending(
       String account, String location, String kind, String app, String cluster, Sort sort) {
     KubernetesResourceProperties properties =
         getRegistry(account).get(KubernetesKind.fromString(kind));
-    if (properties == null) {
-      return null;
-    }
 
     KubernetesHandler handler = properties.getHandler();
 

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

@@ -17,24 +17,33 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
 import javax.annotation.Nonnull;
+import javax.annotation.ParametersAreNonnullByDefault;
 import org.springframework.stereotype.Component;
 
+@ParametersAreNonnullByDefault
 public class AccountResourcePropertyRegistry implements ResourcePropertyRegistry {
   private final GlobalResourcePropertyRegistry globalResourcePropertyRegistry;
-
-  private final ConcurrentHashMap<KubernetesKind, KubernetesResourceProperties> propertyMap =
-      new ConcurrentHashMap<>();
+  private final ImmutableMap<KubernetesKind, KubernetesResourceProperties> propertyMap;
 
   private AccountResourcePropertyRegistry(
-      GlobalResourcePropertyRegistry globalResourcePropertyRegistry) {
+      GlobalResourcePropertyRegistry globalResourcePropertyRegistry,
+      Collection<KubernetesResourceProperties> resourceProperties) {
     this.globalResourcePropertyRegistry = globalResourcePropertyRegistry;
+    this.propertyMap =
+        resourceProperties.stream()
+            .collect(toImmutableMap(p -> p.getHandler().kind(), Function.identity()));
   }
 
+  @Override
   @Nonnull
   public KubernetesResourceProperties get(KubernetesKind kind) {
     KubernetesResourceProperties accountResult = propertyMap.get(kind);
@@ -45,16 +54,13 @@ public KubernetesResourceProperties get(KubernetesKind kind) {
     return globalResourcePropertyRegistry.get(kind);
   }
 
-  public void register(KubernetesResourceProperties properties) {
-    propertyMap.put(properties.getHandler().kind(), properties);
-  }
-
-  public Collection<KubernetesResourceProperties> values() {
-    Collection<KubernetesResourceProperties> result =
-        new ArrayList<>(globalResourcePropertyRegistry.values());
-    result.addAll(propertyMap.values());
-
-    return result;
+  @Override
+  @Nonnull
+  public ImmutableCollection<KubernetesResourceProperties> values() {
+    return new ImmutableList.Builder<KubernetesResourceProperties>()
+        .addAll(globalResourcePropertyRegistry.values())
+        .addAll(propertyMap.values())
+        .build();
   }
 
   @Component
@@ -65,8 +71,10 @@ public Factory(GlobalResourcePropertyRegistry globalResourcePropertyRegistry) {
       this.globalResourcePropertyRegistry = globalResourcePropertyRegistry;
     }
 
-    public AccountResourcePropertyRegistry create() {
-      return new AccountResourcePropertyRegistry(globalResourcePropertyRegistry);
+    public AccountResourcePropertyRegistry create(
+        Collection<KubernetesResourceProperties> resourceProperties) {
+      return new AccountResourcePropertyRegistry(
+          globalResourcePropertyRegistry, resourceProperties);
     }
   }
 }

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

@@ -16,53 +16,53 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableMap;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.KubernetesHandler;
-import java.util.Collection;
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.KubernetesUnregisteredCustomResourceHandler;
 import java.util.List;
-import java.util.concurrent.ConcurrentHashMap;
 import javax.annotation.Nonnull;
+import javax.annotation.ParametersAreNonnullByDefault;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
 
 @Component
+@ParametersAreNonnullByDefault
 public class GlobalResourcePropertyRegistry implements ResourcePropertyRegistry {
-  private final ConcurrentHashMap<KubernetesKind, KubernetesResourceProperties> globalProperties =
-      new ConcurrentHashMap<>();
-  private final KubernetesSpinnakerKindMap kindMap;
+  private final ImmutableMap<KubernetesKind, KubernetesResourceProperties> globalProperties;
+  private final KubernetesResourceProperties defaultProperties;
 
   @Autowired
   public GlobalResourcePropertyRegistry(
-      List<KubernetesHandler> handlers, KubernetesSpinnakerKindMap kindMap) {
-    this.kindMap = kindMap;
-    registerHandlers(handlers);
-  }
-
-  private void registerHandlers(List<KubernetesHandler> handlers) {
-    for (KubernetesHandler handler : handlers) {
-      KubernetesResourceProperties properties =
-          new KubernetesResourceProperties(handler, handler.versioned());
-      register(properties);
-    }
+      List<KubernetesHandler> handlers,
+      KubernetesUnregisteredCustomResourceHandler defaultHandler) {
+    this.globalProperties =
+        handlers.stream()
+            .collect(
+                toImmutableMap(
+                    KubernetesHandler::kind,
+                    h -> new KubernetesResourceProperties(h, h.versioned())));
+    this.defaultProperties =
+        new KubernetesResourceProperties(defaultHandler, defaultHandler.versioned());
   }
 
+  @Override
   @Nonnull
   public KubernetesResourceProperties get(KubernetesKind kind) {
     KubernetesResourceProperties globalResult = globalProperties.get(kind);
     if (globalResult != null) {
       return globalResult;
     }
 
-    return globalProperties.get(KubernetesKind.NONE);
+    return defaultProperties;
   }
 
-  public Collection<KubernetesResourceProperties> values() {
+  @Override
+  @Nonnull
+  public ImmutableCollection<KubernetesResourceProperties> values() {
     return globalProperties.values();
   }
-
-  public void register(KubernetesResourceProperties properties) {
-    KubernetesHandler handler = properties.getHandler();
-    kindMap.addRelationship(handler.spinnakerKind(), handler.kind());
-    globalProperties.put(handler.kind(), properties);
-  }
 }

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

@@ -18,12 +18,14 @@
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.KubernetesHandler;
 import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.stream.Collectors;
 import org.springframework.stereotype.Component;
 
@@ -58,29 +60,33 @@ public static SpinnakerKind fromString(String name) {
     }
   }
 
-  private final Map<SpinnakerKind, Set<KubernetesKind>> spinnakerToKubernetes = new HashMap<>();
-  private final Map<KubernetesKind, SpinnakerKind> kubernetesToSpinnaker = new HashMap<>();
+  private final ImmutableMap<KubernetesKind, SpinnakerKind> kubernetesToSpinnaker;
+  private final ImmutableSetMultimap<SpinnakerKind, KubernetesKind> spinnakerToKubernetes;
 
-  void addRelationship(SpinnakerKind spinnakerKind, KubernetesKind kubernetesKind) {
-    Set<KubernetesKind> kinds = spinnakerToKubernetes.get(spinnakerKind);
-    if (kinds == null) {
-      kinds = new HashSet<>();
+  public KubernetesSpinnakerKindMap(List<KubernetesHandler> handlers) {
+    ImmutableMap.Builder<KubernetesKind, SpinnakerKind> kubernetesToSpinnakerBuilder =
+        new ImmutableMap.Builder<>();
+    ImmutableSetMultimap.Builder<SpinnakerKind, KubernetesKind> spinnakerToKubernetesBuilder =
+        new ImmutableSetMultimap.Builder<>();
+    for (KubernetesHandler handler : handlers) {
+      SpinnakerKind spinnakerKind = handler.spinnakerKind();
+      KubernetesKind kubernetesKind = handler.kind();
+      kubernetesToSpinnakerBuilder.put(kubernetesKind, spinnakerKind);
+      spinnakerToKubernetesBuilder.put(spinnakerKind, kubernetesKind);
     }
-
-    kinds.add(kubernetesKind);
-    spinnakerToKubernetes.put(spinnakerKind, kinds);
-    kubernetesToSpinnaker.put(kubernetesKind, spinnakerKind);
+    this.kubernetesToSpinnaker = kubernetesToSpinnakerBuilder.build();
+    this.spinnakerToKubernetes = spinnakerToKubernetesBuilder.build();
   }
 
   public SpinnakerKind translateKubernetesKind(KubernetesKind kubernetesKind) {
     return kubernetesToSpinnaker.getOrDefault(kubernetesKind, SpinnakerKind.UNCLASSIFIED);
   }
 
-  public Set<KubernetesKind> translateSpinnakerKind(SpinnakerKind spinnakerKind) {
+  public ImmutableSet<KubernetesKind> translateSpinnakerKind(SpinnakerKind spinnakerKind) {
     return spinnakerToKubernetes.get(spinnakerKind);
   }
 
-  public Set<KubernetesKind> allKubernetesKinds() {
+  public ImmutableSet<KubernetesKind> allKubernetesKinds() {
     return kubernetesToSpinnaker.keySet();
   }
 

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

@@ -34,9 +34,6 @@ private static Optional<KubernetesHandler> lookupHandler(
     }
 
     KubernetesResourceProperties properties = propertyRegistry.get(kind);
-    if (properties == null) {
-      return Optional.empty();
-    }
 
     KubernetesHandler handler = properties.getHandler();
 

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

@@ -16,13 +16,16 @@
 
 package com.netflix.spinnaker.clouddriver.kubernetes.v2.description;
 
+import com.google.common.collect.ImmutableCollection;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind;
-import java.util.Collection;
+import javax.annotation.Nonnull;
+import javax.annotation.ParametersAreNonnullByDefault;
 
+@ParametersAreNonnullByDefault
 public interface ResourcePropertyRegistry {
+  @Nonnull
   KubernetesResourceProperties get(KubernetesKind kind);
 
-  Collection<KubernetesResourceProperties> values();
-
-  void register(KubernetesResourceProperties properties);
+  @Nonnull
+  ImmutableCollection<KubernetesResourceProperties> values();
 }

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/artifact/KubernetesCleanupArtifactsOperation.java

@@ -80,10 +80,6 @@ public OperationResult operate(List priorOutputs) {
           String kind = type.substring("kubernetes/".length());
           KubernetesResourceProperties properties =
               credentials.getResourcePropertyRegistry().get(KubernetesKind.fromString(kind));
-          if (properties == null) {
-            log.warn("No properties for artifact {}, ignoring", a);
-            return;
-          }
 
           getTask().updateStatus(OP_NAME, "Deleting artifact '" + a + '"');
           KubernetesHandler handler = properties.getHandler();

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/handler/CanLoadBalance.java

@@ -34,12 +34,6 @@ public interface CanLoadBalance {
   static CanLoadBalance lookupProperties(
       ResourcePropertyRegistry registry, Pair<KubernetesKind, String> name) {
     KubernetesResourceProperties loadBalancerProperties = registry.get(name.getLeft());
-    if (loadBalancerProperties == null) {
-      throw new IllegalArgumentException(
-          "No properties are registered for "
-              + name
-              + ", are you sure it's a valid load balancer type?");
-    }
 
     KubernetesHandler loadBalancerHandler = loadBalancerProperties.getHandler();
     if (loadBalancerHandler == null) {

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/handler/CustomKubernetesHandlerFactory.java

@@ -34,6 +34,7 @@
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.security.KubernetesV2Credentials;
 import com.netflix.spinnaker.clouddriver.model.Manifest;
+import javax.annotation.Nonnull;
 import lombok.extern.slf4j.Slf4j;
 
 public class CustomKubernetesHandlerFactory {
@@ -68,6 +69,7 @@ public int deployPriority() {
       return deployPriority;
     }
 
+    @Nonnull
     @Override
     public KubernetesKind kind() {
       return kubernetesKind;
@@ -78,6 +80,7 @@ public boolean versioned() {
       return versioned;
     }
 
+    @Nonnull
     @Override
     public SpinnakerKind spinnakerKind() {
       return spinnakerKind;

clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/handler/HasPods.java

@@ -29,12 +29,6 @@ public interface HasPods {
 
   static HasPods lookupProperties(ResourcePropertyRegistry registry, KubernetesKind kind) {
     KubernetesResourceProperties hasPodsProperties = registry.get(kind);
-    if (hasPodsProperties == null) {
-      throw new IllegalArgumentException(
-          "No properties are registered for "
-              + kind
-              + ", are you sure it's a valid pod manager type?");
-    }
     KubernetesHandler hasPodsHandler = hasPodsProperties.getHandler();
     if (hasPodsHandler == null) {
       throw new IllegalArgumentException(