fix(artifacts): Fix broken Docker image name matching and add tests
The changes in
used the wrong pattern, and didn't match the beginning and end of
the string, so it allowed partial matches in invalid strings.
Adding some tests to make it less likely that this happens again.
MODIFY
clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacer.java
clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacer.java
@@ -42,6 +42,7 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
+import java.util.Objects;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -104,12 +105,20 @@ public class ArtifactReplacer {
try {
return ((List<String>) mapper.convertValue(r.findAll(document), new TypeReference<List<String>>() { }))
.stream()
- .map(s -> Artifact.builder()
+ .map(s -> {
+ String nameFromReference = r.getNameFromReference(s);
+ String name = nameFromReference == null ? s : nameFromReference;
+ if (r.namePattern == null || nameFromReference != null) {
+ return Artifact.builder()
.type(r.getType().toString())
.reference(s)
- .name(r.getNameFromReference(s))
- .build()
- );
+ .name(name)
+ .build();
+ } else {
+ return null;
+ }
+ }
+ ).filter(Objects::nonNull);
} catch (Exception e) {
// This happens when a manifest isn't fully defined (e.g. not all properties are there)
log.debug("Failure converting artifacts for {} using {} (skipping)", input.getFullResourceName(), r, e);

MODIFY
clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacerFactory.java
clouddriver-kubernetes/src/main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacerFactory.java
@@ -30,20 +30,19 @@ public class ArtifactReplacerFactory {
private final static String DOCKER_DOMAIN = "(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?";
private final static String DOCKER_OPTIONAL_PORT = "(?::[0-9]+)?";
private final static String DOCKER_OPTIONAL_DOMAIN_AND_PORT = "(?:" + DOCKER_DOMAIN + DOCKER_OPTIONAL_PORT + "/)?";
- private final static String DOCKER_IMAGE_NAME = DOCKER_OPTIONAL_DOMAIN_AND_PORT + DOCKER_NAME_COMPONENT + "(?:/" + DOCKER_NAME_COMPONENT + ")+";
- private final static String DOCKER_IMAGE_REFERENCE = "(" + DOCKER_IMAGE_NAME + ")" + "(" + DOCKER_OPTIONAL_TAG + "|"+ DOCKER_OPTIONAL_DIGEST + ")";
+ private final static String DOCKER_IMAGE_NAME = "(" + DOCKER_OPTIONAL_DOMAIN_AND_PORT + DOCKER_NAME_COMPONENT + "(?:/" + DOCKER_NAME_COMPONENT + ")*)";
+ private final static String DOCKER_IMAGE_REFERENCE = DOCKER_IMAGE_NAME + "(" + DOCKER_OPTIONAL_TAG + "|"+ DOCKER_OPTIONAL_DIGEST + ")";
- public static final Pattern DOCKER_IMAGE_NAME_PATTERN = Pattern.compile(DOCKER_IMAGE_NAME);
// the image reference pattern has two capture groups.
// - the first captures the image name
// - the second captures the image tag (including the leading ":") or digest (including the leading "@").
- public static final Pattern DOCKER_IMAGE_REFERENCE_PATTERN = Pattern.compile(DOCKER_IMAGE_REFERENCE);
+ public static final Pattern DOCKER_IMAGE_REFERENCE_PATTERN = Pattern.compile("^" + DOCKER_IMAGE_REFERENCE + "$");
public static Replacer dockerImageReplacer() {
return Replacer.builder()
.replacePath("$.spec.template.spec.containers.[?( @.image == \"{%name%}\" )].image")
.findPath("$.spec.template.spec.containers.*.image")
- .namePattern(DOCKER_IMAGE_NAME_PATTERN)
+ .namePattern(DOCKER_IMAGE_REFERENCE_PATTERN)
.type(ArtifactTypes.DOCKER_IMAGE)
.build();
}

ADD
clouddriver-kubernetes/src/test/groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/artifact/ArtifactReplacerSpec.groovy
+package com.netflix.spinnaker.clouddriver.kubernetes.v2.artifact
+
+import com.fasterxml.jackson.databind.ObjectMapper
+import com.netflix.spinnaker.clouddriver.kubernetes.v2.description.manifest.KubernetesManifest
+import com.netflix.spinnaker.kork.artifacts.model.Artifact
+import org.yaml.snakeyaml.Yaml
+import spock.lang.Specification
+import spock.lang.Unroll
+
+
+class ArtifactReplacerSpec extends Specification {
+ def objectMapper = new ObjectMapper()
+ def yaml = new Yaml()
+
+ KubernetesManifest stringToManifest(String input) {
+ return objectMapper.convertValue(yaml.load(input), KubernetesManifest)
+ }
+
+ @Unroll
+ def "correctly extracts Docker artifacts from image names"() {
+ expect:
+ def deploymentManifest = """
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: my-app-deployment
+ labels:
+ app: my-app
+spec:
+ replicas: 3
+ selector:
+ matchLabels:
+ app: my-app
+ template:
+ metadata:
+ labels:
+ app: my-app
+ spec:
+ containers:
+ - name: container
+ image: $image
+ ports:
+ - containerPort: 80
+"""
+ def artifactReplacer = new ArtifactReplacer()
+ artifactReplacer.addReplacer(ArtifactReplacerFactory.dockerImageReplacer())
+ def manifest = stringToManifest(deploymentManifest)
+ def artifacts = artifactReplacer.findAll(manifest)
+
+ artifacts.size() == 1
+ Artifact artifact = artifacts.toList().get(0)
+ artifact.getType() == ArtifactTypes.DOCKER_IMAGE.toString()
+ artifact.getName() == name
+ artifact.getReference() == image
+
+ where:
+ image || name
+ "nginx:112" || "nginx"
+ "nginx:1.12-alpine" || "nginx"
+ "my-nginx:100000" || "my-nginx"
+ "my.nginx:100000" || "my.nginx"
+ "reg/repo:1.2.3" || "reg/repo"
+ "reg.default.svc/r/j:485fabc" || "reg.default.svc/r/j"
+ "reg:5000/r/j:485fabc" || "reg:5000/r/j"
+ "reg:5000/r__j:485fabc" || "reg:5000/r__j"
+ "clouddriver" || "clouddriver"
+ "localhost:5000/test/busybox@sha256:cbbf2f9a99b47fc460d422812b6a5adff7dfee951d8fa2e4a98caa0382cfbdbf" \
+ || "localhost:5000/test/busybox"
+ }
+
+ @Unroll
+ def "does not generate invalid Docker artifacts from image names"() {
+ expect:
+ def deploymentManifest = """
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+ name: my-app-deployment
+ labels:
+ app: my-app
+spec:
+ template:
+ spec:
+ containers:
+ - name: container
+ image: $image
+ - name: other-container
+ image: my-image:1.0
+"""
+ def artifactReplacer = new ArtifactReplacer()
+ artifactReplacer.addReplacer(ArtifactReplacerFactory.dockerImageReplacer())
+ def manifest = stringToManifest(deploymentManifest)
+ def artifacts = artifactReplacer.findAll(manifest)
+
+ artifacts.size() == 1
+ Artifact artifact = artifacts.toList().get(0)
+ artifact.getType() == ArtifactTypes.DOCKER_IMAGE.toString()
+ artifact.getName() == "my-image"
+ artifact.getReference() == "my-image:1.0"
+
+ where:
+ image | _
+ ":500" | _
+ "'@registry.default.svc/myrepo/jenkins:485fabc'" | _
+ "registry___myrepo/" | _
+ "'!!clouddriver'" | _
+ "localhost@5000/test/busybox@sha256:cbbf2f9" | _
+ "registry/myrepo/_myimage:1.2.3" | _
+ "nginx:-alpine" | _
+ "nginx:.alpine" | _
+ "reg:5000/r___j:485fabc" | _
+ "my..nginx:100000" | _
+ "my-_-nginx:100000" | _
+ }
+}

fix(artifacts): Fix broken Docker image name matching and add tests (#2442)

The changes in https://github.com/spinnaker/clouddriver/pull/2412
used the wrong pattern, and didn't match the beginning and end of
the string, so it allowed partial matches in invalid strings.

Adding some tests to make it less likely that this happens again.