Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(kustomize): support git/repo artifact in kustomize bake manifest #449

Merged
merged 12 commits into from
Oct 29, 2019
Merged

feat(kustomize): support git/repo artifact in kustomize bake manifest #449

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
24adbbb
feat(kustomize): support git/repo artifact in kustomize bake manifest
KathrynLewis Oct 17, 2019
df933b1
Merge branch 'master' into gitrepo-kustomize
Oct 18, 2019
9f128c4
address code review comments
KathrynLewis Oct 23, 2019
ec74473
Merge branch 'master' into gitrepo-kustomize
KathrynLewis Oct 23, 2019
e26c64a
Merge remote-tracking branch 'upstream/master' into gitrepo-kustomize
KathrynLewis Oct 23, 2019
caf7502
Merge branch 'master' into gitrepo-kustomize
Oct 25, 2019
38fa3f6
address code review comments
KathrynLewis Oct 26, 2019
e1eeab3
Merge branch 'master' of https://github.com/spinnaker/rosco into gitr…
KathrynLewis Oct 26, 2019
3fef299
Merge branch 'gitrepo-kustomize' of github.com:armory-io/rosco into g…
KathrynLewis Oct 26, 2019
bec468a
Merge branch 'master' into gitrepo-kustomize
Oct 28, 2019
be530e2
Merge branch 'master' into gitrepo-kustomize
KathrynLewis Oct 29, 2019
95330b6
address code review comments
KathrynLewis Oct 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rosco-manifests/rosco-manifests.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ dependencies {
implementation "com.netflix.spinnaker.kork:kork-exceptions"
implementation "com.netflix.spinnaker.kork:kork-security"
implementation "commons-io:commons-io"
implementation "org.apache.commons:commons-compress:1.14"
implementation "org.yaml:snakeyaml:1.25"

implementation "com.squareup.retrofit:retrofit"
Expand Down
1 change: 1 addition & 0 deletions ...in/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeBakeManifestRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@
@EqualsAndHashCode(callSuper = true)
public class KustomizeBakeManifestRequest extends BakeManifestRequest {
private Artifact inputArtifact;
private String kustomizeFilePath;
}
82 changes: 81 additions & 1 deletion ...src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.rosco.manifests.ArtifactDownloader;
import com.netflix.spinnaker.rosco.manifests.BakeManifestEnvironment;
import com.netflix.spinnaker.rosco.manifests.kustomize.mapping.Kustomization;
import java.io.BufferedInputStream;
import java.io.File;
import java.io.IOException;
import java.net.URI;
Expand All @@ -31,9 +32,13 @@
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.compress.archivers.ArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.apache.commons.compress.compressors.gzip.GzipCompressorInputStream;
import org.apache.commons.io.FilenameUtils;
import org.springframework.stereotype.Component;

Expand All @@ -50,13 +55,28 @@ public KustomizeTemplateUtils(
}

public BakeRecipe buildBakeRecipe(
BakeManifestEnvironment env, KustomizeBakeManifestRequest request) {
BakeManifestEnvironment env, KustomizeBakeManifestRequest request) throws IOException {
BakeRecipe result = new BakeRecipe();
result.setName(request.getOutputName());
Artifact artifact = request.getInputArtifact();
if (artifact == null) {
throw new IllegalArgumentException("Exactly one input artifact must be provided to bake.");
}

String artifactType = Optional.of(artifact.getType()).orElse("");
switch (artifactType) {
KathrynLewis marked this conversation as resolved.
Show resolved Hide resolved
case "git/repo":
return buildBakeRecipeFromGitRepo(env, request, artifact);
default:
return oldBuildBakeRecipe(env, request, artifact);
}
}

// Keep the old logic for now. This will be removed as soon as the rest of the git/repo artifact
// PRs are merged
private BakeRecipe oldBuildBakeRecipe(
BakeManifestEnvironment env, KustomizeBakeManifestRequest request, Artifact artifact) {

String kustomizationfilename = FilenameUtils.getName(artifact.getReference());
if (kustomizationfilename == null
|| (kustomizationfilename != null
Expand All @@ -79,11 +99,71 @@ public BakeRecipe buildBakeRecipe(
command.add("kustomize");
command.add("build");
command.add(templatePath.getParent().toString());

BakeRecipe result = new BakeRecipe();
result.setCommand(command);
return result;
}

private BakeRecipe buildBakeRecipeFromGitRepo(
BakeManifestEnvironment env, KustomizeBakeManifestRequest request, Artifact artifact)
throws IOException {
// This is a redundant check for now, but it's here for when we soon remove the old logic of
// building from
// a github/file artifact type and instead, only support the git/repo artifact type
if (!"git/repo".equals(artifact.getType())) {
throw new IllegalArgumentException("The inputArtifact should be of type \"git/repo\".");
}

String kustomizeFilePath = request.getKustomizeFilePath();
if (kustomizeFilePath == null) {
throw new IllegalArgumentException("The bake request should contain a kustomize file path.");
}

Path tarPath = env.resolvePath("repo.tar.gz");
try {
artifactDownloader.downloadArtifact(artifact, tarPath);
} catch (IOException e) {
throw new IOException("Failed to download git/repo artifact: " + e.getMessage(), e);
}

try {
extractArtifact(tarPath, env.resolvePath(""));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we're doing the following:

  • Requesting the artifact from clouddriver, then writing that InputStream to a file in artifactDownloader.downloadArtifact()
  • Opening the file as an InputStream and decompressing the archive

This would be simpler (and likely more efficient) if we directly consumed the InputStream we got back from clouddriver and decrypted from that, rather than needing the intermediate write to disk.

Perhaps we could rename ArtifactDownloader.downloadArtifact to ArtifactDownloader.downloadArtifactToFile and then have ArtifactDownloader.downloadArtifact just return an InputStream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch--changed! But I'm not sure how we want to handle an empty response body in the downloadArtifact() method. Right now, I just have it logging an error and returning null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I'd suggest throwing an IOException (with the text you're currently logging) if the response body is empty. I realize that's a change from what we currently do, where we just silently don't write out the artifact to a file if there's no input, but it seems like an improvement as the error would be surfaced earlier and more clearly. (As opposed to currently where it would just manifest as a missing file somewhere downstream.)

On the topic of errors, @maggieneterval just improved the error handling in this file in #453 so that errors actually propagate back to the user (per the screen shots on that PR). So you'll probably have a minor merge conflict when you merge in her changes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good--updated.

} catch (IOException e) {
throw new IOException("Failed to extract git/repo artifact: " + e.getMessage(), e);
}

List<String> command = new ArrayList<>();
command.add("kustomize");
command.add("build");
command.add(env.resolvePath(kustomizeFilePath).getParent().toString());

BakeRecipe result = new BakeRecipe();
result.setCommand(command);
return result;
}

// This being here is temporary until we find a better way to abstract it
public static void extractArtifact(Path inputTar, Path outputPath) throws IOException {
KathrynLewis marked this conversation as resolved.
Show resolved Hide resolved
TarArchiveInputStream tarArchiveInputStream =
KathrynLewis marked this conversation as resolved.
Show resolved Hide resolved
new TarArchiveInputStream(
new GzipCompressorInputStream(new BufferedInputStream(Files.newInputStream(inputTar))));

ArchiveEntry archiveEntry = null;
KathrynLewis marked this conversation as resolved.
Show resolved Hide resolved
while ((archiveEntry = tarArchiveInputStream.getNextEntry()) != null) {
Path archiveEntryOutput = outputPath.resolve(archiveEntry.getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this code behave if the archive entry's name references the parent directory? Could this break out of the temporary directory we're trying to extract to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out and you're right that if I force an entry to reference a parent, it does break out. Is this something that we want to support and handle in some way? Or should it be considered an error if an entry at the repo level tries to reference a parent? Because in theory, we wouldn't even have access to those files, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd want to immediately throw an exception and abort if the entry tries to reference something outside of the repo directory. This would avoid security vulnerabilities where someone could send a specially-crafted repo artifact that unpacks on top of system files. (You're right that in theory important files shouldn't be accessible by the rosco process unless it's running as root which it shouldn't, but I still think it's better to fail fast before depending on these permissions being set correctly.)

A good example where we do this check elsewhere and fail fast is here. (That example is not in a re-usable place, but you could do something similar in the function you wrote.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that makes a lot of sense. 👍

if (archiveEntry.isDirectory()) {
if (!Files.exists(archiveEntryOutput)) {
Files.createDirectory(archiveEntryOutput);
}
} else {
Files.copy(tarArchiveInputStream, archiveEntryOutput);
}
}

tarArchiveInputStream.close();
}

protected void downloadArtifactToTmpFileStructure(
BakeManifestEnvironment env, Artifact artifact, String referenceBaseURL) throws IOException {
if (artifact.getReference() == null) {
Expand Down