refactor(manifest): Some refactoring of manifest bakery classes (#444)

* refactor(manifest): Encapsulate paths in BakeManifestEnvironment

Manifest bakes (helm, customize) create a BakeManifestEnviornment
that encapsulates a staging directory. Instead of exposing the
staging directory to consumers and requiring them to do the path
operations, hide the actual staging path from consumers and instead
expose operations to resovle paths relative to that path.

A nice advantage of this is that we can pull the logic to ensure
consumers don't break out of the staging path into the class.

* fix(manifest): Clean up temporary directories in tests

The tests I added in my last PR should have created the
BakeManifestEnvironment in a try-with-resources construction (as
happens in the real code) so that we clean up the temporary
directories that are created.

* refactor(manifest): Use a UUID as the file name

When generating a file name for helm bakes, we take a hash of the
artifact's reference (or if absent the name). Since we just need
the name to be unique, let's just use a UUID to make the name
generation simpler.

* refactor(manifest): A few minor style fixes

* Replace a loop over the enum TemplateRenderer's values with
a call to valueOf
* Remove unecessary null initialization of bakeStatus
* TemplateUtils does not need to be a component, as it is just a
superclass for concrete utils classes
* Simplify the logic in removeTestsDirectoryTemplates

* refactor(manifest): Update some instances of File to Path

We're passing around a mix of Path and File objects; to clean up
the code a bit, just standardize on Path so we don't need to
interconvert between the two.

* refactor(manifest): Avoid passing around a byte[]

We're returning the result of a manifest bake as a byte[] because
we'll eventually need to get the bytes to encode the result as
base64. Instead of having doBake() return the byte[] directly, have
it return a String and only convert to bytes right before encoding as
base64.

This reduces the number of functions that need to accept/return a
byte[]. It also removes the extra conversion that needed to happen in
removeTestsDirectoryTemplates.

* refactor(manifest): Don't inherit artifact downloader

We have a base class TemplateUtils from which both the Helm* and
Kustomize* template utils inherit. The only functionality here is
an artifact download function; instead of having helm and kustomize
inherit from this base class, just call it ArtifactDownloader and
compose it into the other utils classes.

This also cleans up some of the mocking/stubbing in the utils classes
(as now they can just stub the result from downloadArtifact instead
of the clouddriver response) and also allows us to add some tests
on the new ArtifactDownloader.

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/ArtifactDownloader.java

@@ -0,0 +1,25 @@
+/*
+ * Copyright 2019 Google, LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.rosco.manifests;
+
+import com.netflix.spinnaker.kork.artifacts.model.Artifact;
+import java.io.IOException;
+import java.nio.file.Path;
+
+public interface ArtifactDownloader {
+  void downloadArtifact(Artifact artifact, Path targetFile) throws IOException;
+}

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/TemplateUtils.java → rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/ArtifactDownloaderImpl.java

@@ -3,28 +3,28 @@
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.rosco.services.ClouddriverService;
-import java.io.File;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.io.IOUtils;
 import org.springframework.stereotype.Component;
 import retrofit.client.Response;
 
 @Component
 @Slf4j
-public class TemplateUtils {
+public final class ArtifactDownloaderImpl implements ArtifactDownloader {
   private final ClouddriverService clouddriverService;
-  private RetrySupport retrySupport = new RetrySupport();
+  private final RetrySupport retrySupport = new RetrySupport();
 
-  public TemplateUtils(ClouddriverService clouddriverService) {
+  public ArtifactDownloaderImpl(ClouddriverService clouddriverService) {
     this.clouddriverService = clouddriverService;
   }
 
-  protected void downloadArtifact(Artifact artifact, File targetFile) throws IOException {
-    try (OutputStream outputStream = new FileOutputStream(targetFile)) {
+  public void downloadArtifact(Artifact artifact, Path targetFile) throws IOException {
+    try (OutputStream outputStream = Files.newOutputStream(targetFile)) {
       Response response =
           retrySupport.retry(() -> clouddriverService.fetchArtifact(artifact), 5, 1000, true);
       if (response.getBody() != null) {

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/BakeManifestEnvironment.java

@@ -22,9 +22,10 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import javax.annotation.ParametersAreNonnullByDefault;
 
+@ParametersAreNonnullByDefault
 public final class BakeManifestEnvironment implements AutoCloseable {
-
   private final Path stagingPath;
 
   private BakeManifestEnvironment(Path stagingPath) {
@@ -36,12 +37,23 @@ public static BakeManifestEnvironment create() throws IOException {
     return new BakeManifestEnvironment(stagingPath);
   }
 
-  public Path getStagingPath() {
-    return stagingPath;
+  public Path resolvePath(String fileName) {
+    return checkPath(stagingPath.resolve(fileName));
+  }
+
+  public Path resolvePath(Path fileName) {
+    return checkPath(stagingPath.resolve(fileName));
   }
 
   @Override
   public void close() throws IOException {
     MoreFiles.deleteRecursively(stagingPath, ALLOW_INSECURE);
   }
+
+  private Path checkPath(final Path path) {
+    if (!path.normalize().startsWith(stagingPath)) {
+      throw new IllegalStateException("Attempting to create a file outside of the staging path.");
+    }
+    return path;
+  }
 }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/BakeManifestRequest.java

@@ -1,8 +1,8 @@
 package com.netflix.spinnaker.rosco.manifests;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
-import java.util.Arrays;
 import java.util.Map;
+import javax.annotation.Nullable;
 import lombok.Data;
 
 @Data
@@ -17,17 +17,16 @@ public enum TemplateRenderer {
     KUSTOMIZE;
 
     @JsonCreator
-    public TemplateRenderer fromString(String value) {
+    @Nullable
+    public TemplateRenderer fromString(@Nullable String value) {
       if (value == null) {
         return null;
       }
-      return Arrays.stream(values())
-          .filter(v -> value.equalsIgnoreCase(v.toString()))
-          .findFirst()
-          .orElseThrow(
-              () ->
-                  new IllegalArgumentException(
-                      "The value '" + value + "' is not a supported renderer"));
+      try {
+        return TemplateRenderer.valueOf(value.toUpperCase());
+      } catch (IllegalArgumentException e) {
+        throw new IllegalArgumentException("The value '" + value + "' is not a supported renderer");
+      }
     }
   }
 }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/BakeManifestService.java

@@ -39,8 +39,7 @@ public BakeManifestService(JobExecutor jobExecutor) {
 
   public abstract boolean handles(String type);
 
-  protected byte[] doBake(BakeRecipe recipe) {
-    BakeStatus bakeStatus = null;
+  protected String doBake(BakeRecipe recipe) {
     JobRequest jobRequest =
         new JobRequest(
             recipe.getCommand(),
@@ -50,7 +49,7 @@ protected byte[] doBake(BakeRecipe recipe) {
             false);
 
     String jobId = jobExecutor.startJob(jobRequest);
-    bakeStatus = jobExecutor.updateJob(jobId);
+    BakeStatus bakeStatus = jobExecutor.updateJob(jobId);
     while (bakeStatus == null || bakeStatus.getState() == BakeStatus.State.RUNNING) {
       try {
         Thread.sleep(1000);
@@ -63,6 +62,6 @@ protected byte[] doBake(BakeRecipe recipe) {
     if (bakeStatus.getResult() != BakeStatus.Result.SUCCESS) {
       throw new IllegalStateException("Bake failed: " + bakeStatus.getLogsContent());
     }
-    return bakeStatus.getOutputContent().getBytes();
+    return bakeStatus.getOutputContent();
   }
 }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/helm/HelmBakeManifestService.java

@@ -33,13 +33,11 @@ public Artifact bake(HelmBakeManifestRequest bakeManifestRequest) throws IOExcep
     try (BakeManifestEnvironment env = BakeManifestEnvironment.create()) {
       BakeRecipe recipe = helmTemplateUtils.buildBakeRecipe(env, bakeManifestRequest);
 
-      byte[] bakeResult = doBake(recipe);
+      String bakeResult = helmTemplateUtils.removeTestsDirectoryTemplates(doBake(recipe));
       return Artifact.builder()
           .type("embedded/base64")
           .name(bakeManifestRequest.getOutputArtifactName())
-          .reference(
-              Base64.getEncoder()
-                  .encodeToString(helmTemplateUtils.removeTestsDirectoryTemplates(bakeResult)))
+          .reference(Base64.getEncoder().encodeToString(bakeResult.getBytes()))
           .build();
     }
   }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/helm/HelmTemplateUtils.java

@@ -1,35 +1,30 @@
 package com.netflix.spinnaker.rosco.manifests.helm;
 
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
-import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException;
 import com.netflix.spinnaker.rosco.jobs.BakeRecipe;
+import com.netflix.spinnaker.rosco.manifests.ArtifactDownloader;
 import com.netflix.spinnaker.rosco.manifests.BakeManifestEnvironment;
-import com.netflix.spinnaker.rosco.manifests.TemplateUtils;
-import com.netflix.spinnaker.rosco.services.ClouddriverService;
-import java.io.File;
 import java.io.IOException;
 import java.nio.file.Path;
-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
+import java.util.UUID;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
-import javax.annotation.Nonnull;
-import javax.xml.bind.DatatypeConverter;
 import org.springframework.stereotype.Component;
-import org.yaml.snakeyaml.util.ArrayUtils;
 
 @Component
-public class HelmTemplateUtils extends TemplateUtils {
-
+public class HelmTemplateUtils {
   private static final String MANIFEST_SEPARATOR = "---\n";
-  private static final String REGEX_TESTS_MANIFESTS = "# Source: .*/templates/tests/.*";
+  private static final Pattern REGEX_TESTS_MANIFESTS =
+      Pattern.compile("# Source: .*/templates/tests/.*");
+
+  private final ArtifactDownloader artifactDownloader;
 
-  public HelmTemplateUtils(ClouddriverService clouddriverService) {
-    super(clouddriverService);
+  public HelmTemplateUtils(ArtifactDownloader artifactDownloader) {
+    this.artifactDownloader = artifactDownloader;
   }
 
   public BakeRecipe buildBakeRecipe(BakeManifestEnvironment env, HelmBakeManifestRequest request) {
@@ -75,66 +70,30 @@ public BakeRecipe buildBakeRecipe(BakeManifestEnvironment env, HelmBakeManifestR
       }
       String overrideOption = request.isRawOverrides() ? "--set" : "--set-string";
       command.add(overrideOption);
-      command.add(overrideList.stream().collect(Collectors.joining(",")));
+      command.add(String.join(",", overrideList));
     }
 
     if (!valuePaths.isEmpty()) {
       command.add("--values");
-      command.add(
-          String.join(",", valuePaths.stream().map(Path::toString).collect(Collectors.toList())));
+      command.add(valuePaths.stream().map(Path::toString).collect(Collectors.joining(",")));
     }
 
     result.setCommand(command);
 
     return result;
   }
 
-  public byte[] removeTestsDirectoryTemplates(byte[] input) {
-
-    final String inputString = new String(input);
-    final List<String> inputManifests =
-        ArrayUtils.toUnmodifiableList(inputString.split(MANIFEST_SEPARATOR));
-
-    final List<String> outputManifests =
-        inputManifests.stream()
-            .filter(
-                manifest ->
-                    !manifest.trim().isEmpty()
-                        && !Pattern.compile(REGEX_TESTS_MANIFESTS).matcher(manifest).find())
-            .collect(Collectors.toList());
-
-    final String manifestBody =
-        MANIFEST_SEPARATOR
-            + outputManifests.stream().collect(Collectors.joining(MANIFEST_SEPARATOR));
-    return manifestBody.getBytes();
-  }
-
-  private String nameFromReference(String reference) {
-    try {
-      MessageDigest md = MessageDigest.getInstance("MD5");
-      return DatatypeConverter.printHexBinary(md.digest(reference.getBytes()));
-    } catch (NoSuchAlgorithmException e) {
-      throw new RuntimeException("Failed to save bake manifest: " + e.getMessage(), e);
-    }
-  }
-
-  @Nonnull
-  private String uniqueKey(Artifact artifact) {
-    if (artifact.getReference() != null) {
-      return artifact.getReference();
-    }
-    if (artifact.getName() != null) {
-      return String.format(
-          "%s-%s", artifact.getName(), Optional.ofNullable(artifact.getVersion()).orElse(""));
-    }
-    throw new InvalidRequestException("Input artifact has empty 'name' and 'reference' fields.");
+  public String removeTestsDirectoryTemplates(String inputString) {
+    return Arrays.stream(inputString.split(MANIFEST_SEPARATOR))
+        .filter(manifest -> !REGEX_TESTS_MANIFESTS.matcher(manifest).find())
+        .collect(Collectors.joining(MANIFEST_SEPARATOR));
   }
 
-  protected Path downloadArtifactToTmpFile(BakeManifestEnvironment env, Artifact artifact)
+  private Path downloadArtifactToTmpFile(BakeManifestEnvironment env, Artifact artifact)
       throws IOException {
-    String uniqueKey = uniqueKey(artifact);
-    File targetFile = env.getStagingPath().resolve(nameFromReference(uniqueKey)).toFile();
-    downloadArtifact(artifact, targetFile);
-    return targetFile.toPath();
+    String fileName = UUID.randomUUID().toString();
+    Path targetPath = env.resolvePath(fileName);
+    artifactDownloader.downloadArtifact(artifact, targetPath);
+    return targetPath;
   }
 }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeBakeManifestService.java

@@ -53,11 +53,11 @@ public Artifact bake(KustomizeBakeManifestRequest kustomizeBakeManifestRequest)
     try (BakeManifestEnvironment env = BakeManifestEnvironment.create()) {
       BakeRecipe recipe = kustomizeTemplateUtils.buildBakeRecipe(env, kustomizeBakeManifestRequest);
 
-      byte[] bakeResult = doBake(recipe);
+      String bakeResult = doBake(recipe);
       return Artifact.builder()
           .type("embedded/base64")
           .name(kustomizeBakeManifestRequest.getOutputArtifactName())
-          .reference(Base64.getEncoder().encodeToString(bakeResult))
+          .reference(Base64.getEncoder().encodeToString(bakeResult.getBytes()))
           .build();
     }
   }

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java

@@ -19,10 +19,9 @@
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException;
 import com.netflix.spinnaker.rosco.jobs.BakeRecipe;
+import com.netflix.spinnaker.rosco.manifests.ArtifactDownloader;
 import com.netflix.spinnaker.rosco.manifests.BakeManifestEnvironment;
-import com.netflix.spinnaker.rosco.manifests.TemplateUtils;
 import com.netflix.spinnaker.rosco.manifests.kustomize.mapping.Kustomization;
-import com.netflix.spinnaker.rosco.services.ClouddriverService;
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
@@ -40,13 +39,14 @@
 
 @Component
 @Slf4j
-public class KustomizeTemplateUtils extends TemplateUtils {
+public class KustomizeTemplateUtils {
   private final KustomizationFileReader kustomizationFileReader;
+  private final ArtifactDownloader artifactDownloader;
 
   public KustomizeTemplateUtils(
-      KustomizationFileReader kustomizationFileReader, ClouddriverService clouddriverService) {
-    super(clouddriverService);
+      KustomizationFileReader kustomizationFileReader, ArtifactDownloader artifactDownloader) {
     this.kustomizationFileReader = kustomizationFileReader;
+    this.artifactDownloader = artifactDownloader;
   }
 
   public BakeRecipe buildBakeRecipe(
@@ -64,7 +64,7 @@ public BakeRecipe buildBakeRecipe(
       throw new IllegalArgumentException("The inputArtifact should be a valid kustomization file.");
     }
     String referenceBaseURL = extractReferenceBase(artifact);
-    Path templatePath = env.getStagingPath().resolve(artifact.getName());
+    Path templatePath = env.resolvePath(artifact.getName());
 
     try {
       List<Artifact> artifacts = getArtifacts(artifact);
@@ -90,19 +90,10 @@ protected void downloadArtifactToTmpFileStructure(
       throw new InvalidRequestException("Input artifact has an empty 'reference' field.");
     }
     Path artifactFileName = Paths.get(extractArtifactName(artifact, referenceBaseURL));
-    Path artifactFilePath = env.getStagingPath().resolve(artifactFileName);
-    // ensure file write doesn't break out of the staging directory ex. ../etc
+    Path artifactFilePath = env.resolvePath(artifactFileName);
     Path artifactParentDirectory = artifactFilePath.getParent();
-    if (!pathIsWithinBase(env.getStagingPath(), artifactParentDirectory)) {
-      throw new IllegalStateException("attempting to create a file outside of the staging path.");
-    }
     Files.createDirectories(artifactParentDirectory);
-    File newFile = artifactFilePath.toFile();
-    downloadArtifact(artifact, newFile);
-  }
-
-  private boolean pathIsWithinBase(Path base, Path check) {
-    return check.normalize().startsWith(base);
+    artifactDownloader.downloadArtifact(artifact, artifactFilePath);
   }
 
   private List<Artifact> getArtifacts(Artifact artifact) {