fix(kubernetes): throw more helpful errors for invalid services (#4504)

* chore(kubernetes): add tests demonstrating problems with service selector validation

* fix(kubernetes): throw more helpful errors for invalid services

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

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

@@ -23,6 +23,7 @@
 import static com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesKind.SERVICE;
 import static com.netflix.spinnaker.clouddriver.kubernetes.v2.op.handler.KubernetesHandler.DeployPriority.NETWORK_RESOURCE_PRIORITY;
 
+import com.google.common.collect.ImmutableMap;
 import com.netflix.spinnaker.clouddriver.kubernetes.description.SpinnakerKind;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.caching.Keys.InfrastructureCacheKey;
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.caching.agent.KubernetesCacheDataConverter;
@@ -35,6 +36,7 @@
 import com.netflix.spinnaker.clouddriver.kubernetes.v2.model.Manifest.Status;
 import io.kubernetes.client.openapi.models.V1Service;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -101,10 +103,14 @@ public void addRelationships(
     }
   }
 
-  private Map<String, String> getSelector(KubernetesManifest manifest) {
+  @Nonnull
+  private ImmutableMap<String, String> getSelector(KubernetesManifest manifest) {
     if (manifest.getApiVersion().equals(V1)) {
       V1Service v1Service = KubernetesCacheDataConverter.getResource(manifest, V1Service.class);
-      return v1Service.getSpec().getSelector();
+      if (v1Service.getSpec() == null || v1Service.getSpec().getSelector() == null) {
+        return ImmutableMap.of();
+      }
+      return ImmutableMap.copyOf(v1Service.getSpec().getSelector());
     } else {
       throw new IllegalArgumentException(
           "No services with version " + manifest.getApiVersion() + " supported");
@@ -118,8 +124,8 @@ private List<KubernetesManifest> getRelatedManifests(
 
   private Set<KubernetesManifest> intersectLabels(
       KubernetesManifest service, Map<String, Set<KubernetesManifest>> mapLabelToManifest) {
-    Map<String, String> selector = getSelector(service);
-    if (selector == null || selector.isEmpty()) {
+    ImmutableMap<String, String> selector = getSelector(service);
+    if (selector.isEmpty()) {
       return new HashSet<>();
     }
 
@@ -174,7 +180,15 @@ private String podLabelKey(String namespace, Map.Entry<String, String> label) {
   @Override
   public void attach(KubernetesManifest loadBalancer, KubernetesManifest target) {
     Map<String, String> labels = target.getSpecTemplateLabels().orElse(target.getLabels());
-    Map<String, String> selector = getSelector(loadBalancer);
+    ImmutableMap<String, String> selector = getSelector(loadBalancer);
+    if (selector.isEmpty()) {
+      throw new IllegalArgumentException(
+          "Service must have a non-empty selector in order to be attached to a workload");
+    }
+    if (!Collections.disjoint(labels.keySet(), selector.keySet())) {
+      throw new IllegalArgumentException(
+          "Service selector must have no label keys in common with target workload");
+    }
     labels.putAll(selector);
   }
 

clouddriver-kubernetes-v2/src/test/java/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/KubernetesDeployManifestOperationTest.java

@@ -18,6 +18,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -133,6 +134,24 @@ void doesNotSendTrafficWhenEnableTrafficFalse() {
     assertThat(traffic.getLoadBalancers()).containsExactly("service my-service");
   }
 
+  @Test
+  void failsWhenServiceHasNoSelector() {
+    KubernetesDeployManifestDescription description =
+        baseDeployDescription("deploy/replicaset.yml")
+            .setServices(ImmutableList.of("service my-service-no-selector"))
+            .setEnableTraffic(true);
+    assertThatThrownBy(() -> deploy(description)).isInstanceOf(IllegalArgumentException.class);
+  }
+
+  @Test
+  void failsWhenServiceSelectorOverlapsWithTargetLabels() {
+    KubernetesDeployManifestDescription description =
+        baseDeployDescription("deploy/replicaset-overlapping-selector.yml")
+            .setServices(ImmutableList.of("service my-service"))
+            .setEnableTraffic(true);
+    assertThatThrownBy(() -> deploy(description)).isInstanceOf(IllegalArgumentException.class);
+  }
+
   private static KubernetesDeployManifestDescription baseDeployDescription(String manifest) {
     KubernetesDeployManifestDescription deployManifestDescription =
         new KubernetesDeployManifestDescription()
@@ -176,6 +195,10 @@ private static KubernetesV2Credentials getMockKubernetesV2Credentials() {
         .thenReturn(
             ManifestFetcher.getManifest(
                 KubernetesDeployManifestOperationTest.class, "deploy/service.yml"));
+    when(credentialsMock.get(KubernetesKind.SERVICE, "my-namespace", "my-service-no-selector"))
+        .thenReturn(
+            ManifestFetcher.getManifest(
+                KubernetesDeployManifestOperationTest.class, "deploy/service-no-selector.yml"));
     when(credentialsMock.deploy(any(KubernetesManifest.class)))
         .thenAnswer(
             invocation -> {

clouddriver-kubernetes-v2/src/test/resources/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/deploy/replicaset-overlapping-selector.yml

@@ -0,0 +1,13 @@
+apiVersion: extensions/v1beta1
+kind: replicaSet
+metadata:
+  name: my-name
+  namespace: my-namespace
+spec:
+  selector:
+    matchLabels:
+      selector-key: selector-value
+  template:
+    metadata:
+      labels:
+        selector-key: selector-value

clouddriver-kubernetes-v2/src/test/resources/com/netflix/spinnaker/clouddriver/kubernetes/v2/op/deploy/service-no-selector.yml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: service
+metadata:
+  name: my-service
+  namespace: my-namespace