feat(kustomize): support git/repo artifact in kustomize bake manifest #449
Conversation
...ts/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
Outdated
Show resolved
Hide resolved
|
|
||
| try { | ||
| extractArtifact(tarPath, env.resolvePath("")); |
There was a problem hiding this comment.
Currently, we're doing the following:
- Requesting the artifact from clouddriver, then writing that
InputStreamto a file inartifactDownloader.downloadArtifact() - Opening the file as an
InputStreamand 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good--updated.
...ts/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
Outdated
Show resolved
Hide resolved
|
|
||
| ArchiveEntry archiveEntry = null; | ||
| while ((archiveEntry = tarArchiveInputStream.getNextEntry()) != null) { | ||
| Path archiveEntryOutput = outputPath.resolve(archiveEntry.getName()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Yep, that makes a lot of sense. 👍
rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/ArtifactDownloaderImpl.java
Show resolved
Hide resolved
...ts/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java
Outdated
Show resolved
Hide resolved
|
Linking to the PR in clouddriver which added git/repo: spinnaker/clouddriver#4049 |
| if (response.getBody() == null) { | ||
| throw new IOException("Failure to fetch artifact: empty response"); | ||
| } else { | ||
| try (InputStream inputStream = response.getBody().in()) { |
There was a problem hiding this comment.
I don't think we'll want a try-with-resources in this function, as it's directly returning the InputStream which should remain open for the caller (whereas try-with-resources will close it).
I think what this function should do is just return response.getBody().in() after the null check. Then callers (including the below downloadArtifactToFile) should call this function as:
try (InputStream inputStream = downloadArtifact(artifact)) {
// ...use inputStream
}
That means we'll have nested try-with-resources blocks in downloadArtifactToFile but that's fine and the usual way of solving this.
There was a problem hiding this comment.
Ah, that makes sense--updated. I have the catches nested as well, to preserve the logic of the error messages Maggie recently added.
…spinnaker#449) * feat(kustomize): support git/repo artifact in kustomize bake manifest * address code review comments * address code review comments * address code review comments
…spinnaker#449) * feat(kustomize): support git/repo artifact in kustomize bake manifest * address code review comments * address code review comments * address code review comments
This adds support for
git/repoas an artifact type for a kustomize bake. This is a work in progress involving changes to other services as well, so we're leaving in the old logic (baking from a github/file artifact) until the rest of the PRs are merged.@ezimanyi PTAL?