fix(kubernetes/artifacts): Support for legacy Clouddriver's artifact …

…replacement logic (backport #5513) (#5537) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Cristhian Castaneda <ccastanedarivera@gmail.com>
spinnaker · Sep 17, 2021 · 91ebbba · 91ebbba
1 parent d9f86a9
commit 91ebbba
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 30 deletions.
diff --git a/...src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/ArtifactReplacer.java b/...src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/ArtifactReplacer.java
@@ -88,6 +88,7 @@ private static Predicate<Artifact> accountMatches(@Nonnull String account) {
 
   @Nonnull
   public ReplaceResult replaceAll(
+      String dockerImageBinding,
       KubernetesManifest input,
       List<Artifact> artifacts,
       @Nonnull String namespace,
@@ -104,7 +105,7 @@ public ReplaceResult replaceAll(
     ImmutableSet.Builder<Artifact> replacedArtifacts = ImmutableSet.builder();
     for (Replacer replacer : replacers) {
       ImmutableCollection<Artifact> replaced =
-          replacer.replaceArtifacts(document, filteredArtifacts);
+          replacer.replaceArtifacts(dockerImageBinding, document, filteredArtifacts);
       replacedArtifacts.addAll(replaced);
     }
 

diff --git a/...ernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/Replacer.java b/...ernetes/src/main/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/Replacer.java
@@ -42,6 +42,7 @@
 import javax.annotation.Nullable;
 import lombok.AccessLevel;
 import lombok.Builder;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -52,6 +53,7 @@ public final class Replacer {
   private final KubernetesArtifactType type;
   private final JsonPath findPath;
   private final Function<Artifact, JsonPath> replacePathSupplier;
+  private final Function<Artifact, JsonPath> legacyReplacePathSupplier;
   private final Function<String, String> nameFromReference;
 
   /**
@@ -60,6 +62,9 @@ public final class Replacer {
    *     representing a filter
    * @param findFilter a filter that should be applied to the path when finding any artifacts in a
    *     manifest; defaults to a filter matching all nodes
+   * @param legacyReplaceFilter a function that takes an artifact and returns the filter that should
+   *     be applied to the path when replacing artifacts; if a findFilter is supplied both the
+   *     findFilter and replaceFilter must match for the artifact to be replaced
    * @param replacePathFromPlaceholder a string that represents the path from the [?] placeholder to
    *     the replaced field.
    * @param nameFromReference a function to extract an artifact name from its reference; defaults to
@@ -70,6 +75,7 @@ private Replacer(
       KubernetesArtifactType type,
       String path,
       @Nullable Filter findFilter,
+      Function<Artifact, Filter> legacyReplaceFilter,
       String replacePathFromPlaceholder,
       @Nullable Function<String, String> nameFromReference) {
     this.type = Objects.requireNonNull(type);
@@ -82,9 +88,12 @@ private Replacer(
       this.findPath = JsonPath.compile(path, findFilter);
       this.replacePathSupplier =
           a -> JsonPath.compile(path, replaceFilter.apply(a).and(findFilter));
+      this.legacyReplacePathSupplier =
+          a -> JsonPath.compile(path, legacyReplaceFilter.apply(a).and(findFilter));
     } else {
       this.findPath = JsonPath.compile(path, filter(a -> true));
       this.replacePathSupplier = a -> JsonPath.compile(path, replaceFilter.apply(a));
+      this.legacyReplacePathSupplier = a -> JsonPath.compile(path, legacyReplaceFilter.apply(a));
     }
   }
 
@@ -101,10 +110,10 @@ Stream<Artifact> getArtifacts(DocumentContext document) {
   }
 
   ImmutableCollection<Artifact> replaceArtifacts(
-      DocumentContext obj, Collection<Artifact> artifacts) {
+      String dockerImageBinding, DocumentContext obj, Collection<Artifact> artifacts) {
     ImmutableSet.Builder<Artifact> replacedArtifacts = ImmutableSet.builder();
     for (Artifact artifact : artifacts) {
-      boolean wasReplaced = replaceIfPossible(obj, artifact);
+      boolean wasReplaced = replaceIfPossible(dockerImageBinding, obj, artifact);
       if (wasReplaced) {
         replacedArtifacts.add(artifact);
       }
@@ -123,12 +132,19 @@ private Predicate createReplaceFilterPredicate(String replacePath, String name)
     };
   }
 
-  private boolean replaceIfPossible(DocumentContext obj, Artifact artifact) {
+  private boolean replaceIfPossible(
+      String dockerImageBinding, DocumentContext obj, Artifact artifact) {
     if (!type.getType().equals(artifact.getType())) {
       return false;
     }
 
-    JsonPath path = replacePathSupplier.apply(artifact);
+    JsonPath path;
+    if (!StringUtils.isBlank(dockerImageBinding) && dockerImageBinding.equals("match-name-only")) {
+      path = legacyReplacePathSupplier.apply(artifact);
+    } else {
+      path = replacePathSupplier.apply(artifact);
+    }
+
     log.debug("Processed jsonPath == {}", path.getPath());
 
     Object get;
@@ -150,6 +166,7 @@ private boolean replaceIfPossible(DocumentContext obj, Artifact artifact) {
   private static final Replacer DOCKER_IMAGE =
       builder()
           .path("$..spec.template.spec['containers', 'initContainers'].[?].image")
+          .legacyReplaceFilter(a -> filter(where("image").is(a.getName())))
           .replacePathFromPlaceholder("image")
           .nameFromReference(
               ref -> {
@@ -176,58 +193,67 @@ private boolean replaceIfPossible(DocumentContext obj, Artifact artifact) {
   private static final Replacer POD_DOCKER_IMAGE =
       builder()
           .path("$.spec.containers.[?].image")
+          .legacyReplaceFilter(a -> filter(where("image").is(a.getName())))
           .replacePathFromPlaceholder("image")
           .type(KubernetesArtifactType.DockerImage)
           .build();
   private static final Replacer CONFIG_MAP_VOLUME =
       builder()
           .path("$..spec.template.spec.volumes.[?].configMap.name")
+          .legacyReplaceFilter(a -> filter(where("configMap.name").is(a.getName())))
           .replacePathFromPlaceholder("configMap.name")
           .type(KubernetesArtifactType.ConfigMap)
           .build();
   private static final Replacer SECRET_VOLUME =
       builder()
           .path("$..spec.template.spec.volumes.[?].secret.secretName")
+          .legacyReplaceFilter(a -> filter(where("secret.secretName").is(a.getName())))
           .replacePathFromPlaceholder("secret.secretName")
           .type(KubernetesArtifactType.Secret)
           .build();
   private static final Replacer CONFIG_MAP_PROJECTED_VOLUME =
       builder()
           .path("$..spec.template.spec.volumes.*.projected.sources.[?].configMap.name")
+          .legacyReplaceFilter(a -> filter(where("configMap.name").is(a.getName())))
           .replacePathFromPlaceholder("configMap.name")
           .type(KubernetesArtifactType.ConfigMap)
           .build();
   private static final Replacer SECRET_PROJECTED_VOLUME =
       builder()
           .path("$..spec.template.spec.volumes.*.projected.sources.[?].secret.name")
+          .legacyReplaceFilter(a -> filter(where("secret.name").is(a.getName())))
           .replacePathFromPlaceholder("secret.name")
           .type(KubernetesArtifactType.Secret)
           .build();
   private static final Replacer CONFIG_MAP_KEY_VALUE =
       builder()
           .path(
               "$..spec.template.spec['containers', 'initContainers'].*.env.[?].valueFrom.configMapKeyRef.name")
+          .legacyReplaceFilter(a -> filter(where("valueFrom.configMapKeyRef.name").is(a.getName())))
           .replacePathFromPlaceholder("valueFrom.configMapKeyRef.name")
           .type(KubernetesArtifactType.ConfigMap)
           .build();
   private static final Replacer SECRET_KEY_VALUE =
       builder()
           .path(
               "$..spec.template.spec['containers', 'initContainers'].*.env.[?].valueFrom.secretKeyRef.name")
+          .legacyReplaceFilter(a -> filter(where("valueFrom.secretKeyRef.name").is(a.getName())))
           .replacePathFromPlaceholder("valueFrom.secretKeyRef.name")
           .type(KubernetesArtifactType.Secret)
           .build();
   private static final Replacer CONFIG_MAP_ENV =
       builder()
           .path(
               "$..spec.template.spec['containers', 'initContainers'].*.envFrom.[?].configMapRef.name")
+          .legacyReplaceFilter(a -> filter(where("configMapRef.name").is(a.getName())))
           .replacePathFromPlaceholder("configMapRef.name")
           .type(KubernetesArtifactType.ConfigMap)
           .build();
   private static final Replacer SECRET_ENV =
       builder()
           .path(
               "$..spec.template.spec['containers', 'initContainers'].*.envFrom.[?].secretRef.name")
+          .legacyReplaceFilter(a -> filter(where("secretRef.name").is(a.getName())))
           .replacePathFromPlaceholder("secretRef.name")
           .type(KubernetesArtifactType.Secret)
           .build();
@@ -237,6 +263,7 @@ private boolean replaceIfPossible(DocumentContext obj, Artifact artifact) {
           .findFilter(
               filter(where("spec.scaleTargetRef.kind").is("Deployment"))
                   .or(where("spec.scaleTargetRef.kind").is("deployment")))
+          .legacyReplaceFilter(a -> filter(where("spec.scaleTargetRef.name").is(a.getName())))
           .replacePathFromPlaceholder("spec.scaleTargetRef.name")
           .type(KubernetesArtifactType.Deployment)
           .build();
@@ -246,6 +273,7 @@ private boolean replaceIfPossible(DocumentContext obj, Artifact artifact) {
           .findFilter(
               filter(where("spec.scaleTargetRef.kind").is("ReplicaSet"))
                   .or(where("spec.scaleTargetRef.kind").is("replicaSet")))
+          .legacyReplaceFilter(a -> filter(where("spec.scaleTargetRef.name").is(a.getName())))
           .replacePathFromPlaceholder("spec.scaleTargetRef.name")
           .type(KubernetesArtifactType.ReplicaSet)
           .build();

diff --git a/.../main/java/com/netflix/spinnaker/clouddriver/kubernetes/op/handler/KubernetesHandler.java b/.../main/java/com/netflix/spinnaker/clouddriver/kubernetes/op/handler/KubernetesHandler.java
@@ -39,12 +39,16 @@
 import java.util.*;
 import javax.annotation.Nonnull;
 import lombok.Getter;
+import org.springframework.beans.factory.annotation.Value;
 
 public abstract class KubernetesHandler implements CanDeploy, CanDelete, CanPatch {
   protected static final ObjectMapper objectMapper = new ObjectMapper();
 
   private final ArtifactReplacer artifactReplacer;
 
+  @Value("${kubernetes.artifact-binding.docker-image:match-name-and-tag}")
+  protected String dockerImageBinding;
+
   protected KubernetesHandler() {
     this.artifactReplacer = new ArtifactReplacer(artifactReplacers());
   }
@@ -77,15 +81,17 @@ protected ImmutableList<Replacer> artifactReplacers() {
 
   public ReplaceResult replaceArtifacts(
       KubernetesManifest manifest, List<Artifact> artifacts, @Nonnull String account) {
-    return artifactReplacer.replaceAll(manifest, artifacts, manifest.getNamespace(), account);
+    return artifactReplacer.replaceAll(
+        this.dockerImageBinding, manifest, artifacts, manifest.getNamespace(), account);
   }
 
   public ReplaceResult replaceArtifacts(
       KubernetesManifest manifest,
       List<Artifact> artifacts,
       @Nonnull String namespace,
       @Nonnull String account) {
-    return artifactReplacer.replaceAll(manifest, artifacts, namespace, account);
+    return artifactReplacer.replaceAll(
+        this.dockerImageBinding, manifest, artifacts, namespace, account);
   }
 
   protected abstract KubernetesCachingAgentFactory cachingAgentFactory();

diff --git a/...test/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/ArtifactReplacerTest.java b/...test/java/com/netflix/spinnaker/clouddriver/kubernetes/artifact/ArtifactReplacerTest.java
@@ -64,6 +64,7 @@ final class ArtifactReplacerTest {
 
   private static final String NAMESPACE = "ns";
   private static final String ACCOUNT = "my-account";
+  private static final String DEFAULT_BINDING = "match-name-and-tag";
 
   @Test
   void extractsDeploymentNameFromHpa() {
@@ -156,7 +157,8 @@ void emptyReplace() {
     KubernetesManifest deployment = getDeploymentWithContainer(getContainer("nginx:112"));
 
     ReplaceResult result =
-        artifactReplacer.replaceAll(deployment, ImmutableList.of(), NAMESPACE, ACCOUNT);
+        artifactReplacer.replaceAll(
+            DEFAULT_BINDING, deployment, ImmutableList.of(), NAMESPACE, ACCOUNT);
 
     assertThat(result.getManifest()).isEqualTo(deployment);
     assertThat(result.getBoundArtifacts()).isEmpty();
@@ -172,7 +174,7 @@ void replacesDockerImage() {
         Artifact.builder().type("docker/image").name("nginx").reference("nginx:1.19.1").build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractImage(result.getManifest())).contains("nginx:1.19.1");
     assertThat(result.getBoundArtifacts()).hasSize(1);
@@ -189,13 +191,34 @@ void replacesDockerImageWithTag() {
         Artifact.builder().type("docker/image").name("nginx").reference("nginx:1.19.1").build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractImage(result.getManifest())).contains("nginx:1.19.1");
     assertThat(result.getBoundArtifacts()).hasSize(1);
     assertThat(Iterables.getOnlyElement(result.getBoundArtifacts())).isEqualTo(inputArtifact);
   }
 
+  /**
+   * This is a support for a legacy behavior, it's disabled by default and enabled by
+   * kubernetes.artifact-binding.docker-image with value 'match-name-only'. If there is already a
+   * tag on the image in the manifest, we are not replacing it.
+   */
+  @Test
+  void doesNotReplaceImageWithTag() {
+    ArtifactReplacer artifactReplacer =
+        new ArtifactReplacer(ImmutableList.of(Replacer.dockerImage()));
+    KubernetesManifest deployment = getDeploymentWithContainer(getContainer("nginx:1.18.0"));
+
+    Artifact inputArtifact =
+        Artifact.builder().type("docker/image").name("nginx").reference("nginx:1.19.1").build();
+    ReplaceResult result =
+        artifactReplacer.replaceAll(
+            "match-name-only", deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+
+    assertThat(result.getManifest()).isEqualTo(deployment);
+    assertThat(result.getBoundArtifacts()).isEmpty();
+  }
+
   /**
    * Only artifacts of type kubernetes/* need to have the same account as the manifest to be
    * replaced.
@@ -215,7 +238,7 @@ void nonKubernetesArtifactIgnoresDifferentAccount() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractImage(result.getManifest())).contains("nginx:1.19.1");
     assertThat(result.getBoundArtifacts()).hasSize(1);
@@ -241,7 +264,7 @@ void nonKubernetesArtifactIgnoresDifferentNamespace() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, deployment, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractImage(result.getManifest())).contains("nginx:1.19.1");
     assertThat(result.getBoundArtifacts()).hasSize(1);
@@ -265,7 +288,7 @@ void replacesConfigMap() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractEnvRef(result.getManifest())).contains("my-config-map-v003");
     assertThat(result.getBoundArtifacts()).hasSize(1);
@@ -290,7 +313,7 @@ void replacesConfigMapArtifactMissingAccount() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractEnvRef(result.getManifest())).contains("my-config-map-v003");
     assertThat(result.getBoundArtifacts()).hasSize(1);
@@ -316,7 +339,7 @@ void doesNotReplaceConfigmapWrongAccount() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractEnvRef(result.getManifest())).contains("my-config-map");
     assertThat(result.getBoundArtifacts()).hasSize(0);
@@ -339,7 +362,7 @@ void doesNotReplaceConfigmapWrongNamespace() {
             .build();
     ReplaceResult result =
         artifactReplacer.replaceAll(
-            replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
+            DEFAULT_BINDING, replicaSet, ImmutableList.of(inputArtifact), NAMESPACE, ACCOUNT);
 
     assertThat(extractEnvRef(result.getManifest())).contains("my-config-map");
     assertThat(result.getBoundArtifacts()).hasSize(0);
@@ -360,7 +383,8 @@ void replacesConfigMapNoNamespace() {
             .putMetadata("account", ACCOUNT)
             .build();
     ReplaceResult result =
-        artifactReplacer.replaceAll(replicaSet, ImmutableList.of(inputArtifact), "", ACCOUNT);
+        artifactReplacer.replaceAll(
+            DEFAULT_BINDING, replicaSet, ImmutableList.of(inputArtifact), "", ACCOUNT);
 
     assertThat(extractEnvRef(result.getManifest())).contains("my-config-map-v003");
     assertThat(result.getBoundArtifacts()).hasSize(1);