feature(kustomize): add integration for kustomize

[ethanfrogers]

implements support for kustomize as a templating engine. one of the
interesting challenges with kustomize is that it depends on files
adjacent to a single kustomization file for it's templating. the
kustomization file refers to files surrounding it. these files are
collected and layered on top of eachother to produce a single, fully
rendered manifest. because of this we've had to implement a special
handling of kustomization files. essentially, before we attempt to
render a kustomization we parse the file and generate artifact requests
for any dependencies. we'll subsequently do this for any dependencies
that happen to be kustomization files themselves.

[ethanfrogers]

Submitted on behalf of @jorgebee65!

[emjburns]

Pretty cool!

[ezimanyi]

Overall this looks good, excited for this change! I've left a number of inline comments.

The one part I didn't fully read/understand yet is all the detailed logic around downloading files into a local directory structure. I get the overall idea, but was having a bit of trouble following the various functions handling parts of it. It think that logic can probably be simplified, but I'll take a closer look tomorrow and see if I have any ideas around that.

[ezimanyi]

Yaml objects should be created as new Yaml(new SafeConstructor()) to avoid code injection during deserialization.

[ethanfrogers]

SafeConstructor limits castable classes to Java natives. Do you know how I could use SafeConstructor but also add the Kustomization class it?

[ezimanyi]

I'm not sure---I thought it could also deserialize other objects? That's how we construct these all in clouddriver, but I'm not 100% the exact restrictions from SafeConstructor.

[ethanfrogers]

I think it works in Clouddriver because KubernetesManifest is just a primitive Map with methods on it.

[ethanfrogers]

I've addressed most of the simple feedback. I'll tackle the more complex stuff tomorrow morning! Thanks for the detailed feedback @ezimanyi. Feel free to ping me if you'd like to talk about the logic around gathering dependencies.

[plumpy]

In the interest of reducing the amount of Map<String, Object>s that get flung around, what if we made the interface look like this?

public interface BakeManifestService<T> {
  boolean handles(String type);
  Class<T> requestType();
  Artifact bake(T bakeManifestRequest);

And pushed the deserialization out to the BakeryController?

[ethanfrogers]

That's actually something I was going to ask about! I prefer that!

[ethanfrogers]

👍

[plumpy]

isWithinParent("/foo", "/foo2") will incorrectly return true here

Luckily for you Path has its own startsWith method that handles this in a more correct way. Like this:

Path artifactDirectory = artifactPath.getParent();
if (!artifactDirectory.startsWith(env.getStagingPath()) {
  throw new IllegalStateException(...)
}

In general, you usually don't want to convert Path objects to Strings to manipulate them. Path has methods to do these operations more safely. So, also (below) try this:
File newFile = env.getStagingPath().resolve(artifactPath).toFile()

[ethanfrogers]

🤦‍♂ And that's why we do code review 😄 . Thanks @plumpy!

[ezimanyi]

I'm a bit confused by the logic in this function...where is the possibleName coming from? It looks like we're trying all of the KUSTOMIZATION_FILENAMES but using an input String as a hint for what to try first?

[ethanfrogers]

Correct. A Kustomization file can have 3 names, .yml, .yaml and one w/o an extension. Initially, the file name comes from the artifact that you supply as an input. We're making the assumption that if you use, say kustomization.yml, you're likely to use kustomization.yml for your subsequent kustomizations. Since we have no way of knowing that the actual file name is before we try to fetch it we try to be smart and make a best guess before falling back to the other options.

[ezimanyi]

I'm not an expert, but does the current form prevent deserialization code injection? I think that from a security perspective we'll need to solve this before merging (if this version hasn't already solved it).

[ethanfrogers]

I'm honestly not sure. I know SafeConstructor only lets you deserialize to Java primitives so using it here will cause an error.

[ethanfrogers]

For reference - Yes, using a new Constructor(Kustomization.class) will prevent code injection according to this.

[ezimanyi]

OK, gave it another pass, mostly looking good. I think from my perspective the main blocker is just ensuring that we're safely deserializing YAML so we don't create any security issues.

The logic to download files looks a lot cleaner now. Is there a design document or some artifact describing how this is designed? It thing seeing it at a higher level might make it easier for me to understand the code. I've left a few questions still on parts I don't fully understand.

[ethanfrogers]

@ezimanyi I can certainly document this in a more permanent location but will describe the design here.

Generally, a "kustomization" is a folder, in a repo, and is made up of 2 things:

1. A kustomization file which describes how the resulting manifest(s) should be rendered. This includes things like files to include (resources), patch files to modify those files (various patch keys), configMapGenerators which take various files and turn them into a ConfigMap, etc.
2. All of the files that the kustomization file depends on.

It's also worth pointing out that a kustomization file can depend on other kustomizations in sibling or child directories e.x ../sibling or ./child.

Kustomize is typically used in CI and meant to be run against a full repo where all of the file dependencies are checked out alongside the kustomization file that defines them. However, since Spinnaker doesn't have an artifact that can represent this (a git/repo artifact, for example) we needed to come up with a way to ensure all of the file dependencies are present before we can run kustomize build.

Our solution is to treat each kustomization file as a BOM, essentially. For every kustomization we fine, we collect the files we know aren't references to other kustomizations and then attempt to fetch subsequent kustomizations and do the same thing. Once we have a full list of dependencies, we can reconstruct the file tree, write each file to disk and then run kustomize build.

[ethanfrogers]

@ezimanyi I'd love to see this land in 1.16 so LMK if there's anything else we need to do.

[ezimanyi]

I just did another pass and added some comments. I spent a bunch of time trying to parse through and understand the logic for downloading all dependencies by parsing the kustomize files; I think I understand now and can see how it works, though I have to admit I still find the logic reasonably confusing and the amount of string concatenation and matching seems very fragile.

As one example of this being fragile, when I run the tests I see this in the logs:

downloading file https:/api.github.com/repos/org/repo/contents/base/kustomization.yml

It seems like somehow all the path matching logic is dropping one of the slashes from the https://? It's not clear to me how this would be working at all in that case, unless this is somehow only a test issue. What examples were used to test this logic?

I feel like in retrospect this is something that would have benefited from a design document and more discussion, particularly around the question of how to handle getting all dependencies from a git repo into Spinnaker. A few other solutions I could think of:

• We expect the input artifact to be a tar file that already contains the dependencies and all Spinnaker needs to do is un-tar it
• We add a more general artifact type that represents not just a single file in git but represents a folder structure
  I probably would have liked more discussion around what the right solution is; this solution here is reasonable though does have the disadvantage that it's somewhat breaking the artifact abstraction.

I guess overall this seems fine, but I can't say I'm fully confident that all the downloading logic is correct given the complexity and the reasonably low test coverage. I've left a few comments about places where I think there might be edge cases, but I'm sure I didn't catch everything and would feel way more comfortable if the logic were more encapsulated and better tested.

[ezimanyi]

Here we're assuming the file name is artifact.getName() whereas on line 58 we're calling FilenameUtils.getName(artifact.getReference()) to get the name. Are these guaranteed to return the same result, or will this break in some cases?

[ezimanyi]

I'm not a big fan of the fact that we're breaking the artifact extraction here; the point of an artifact is that is should be possible to use artifacts of different types interchangably (as long as the actual file type matches). If we start adding logic that only works for some types of artifacts but not others, we're regressing back to the special case-logic that artifacts were intended to avoid.

[ethanfrogers]

What if we limited the input artifact types to Github and Bitbucket artifacts? I agree that it isn't ideal but maybe we can use this as an experiment to determine what artifact types people use this with most often. If people use it with more than Github or Bitbucket we can revisit.

[ezimanyi]

Based on the tests for this, and from how this is implemented, is this just assuming that any file that does not contain a . is a folder? It seems like child/kustomize would return true, but that's a common format for kustomize files. Might people have patch files that don't have extensions?

[ethanfrogers]

Moved to just checking for an extension on the file via FilenameUtils.getExtension.

[ezimanyi]

Would feel better if this logic were exercised by a unit test

[ezimanyi]

Actually I'm fairly certain this doesn't work as is; the following will return true:

    Path test = Paths.get("/tmp/mydir");
    Path other = Paths.get("/tmp/mydir/../../etc/passwd");
    boolean matches = other.startsWith(test);

[ethanfrogers]

I see what we need to do here - we need a call to normalize before we to the assertion.

[ethanfrogers]

@ezimanyi agreed that this isn't least confusing solution out there. We opted for this solution for a couple of reasons:

1. It's as close to the normal kustomize experience as we can get with what we have today. Your suggestion about expecting a .tar is reasonable, however, as a user of kustomize I wouldn't see much value in using it within Spinnaker if I had to go the extra step and package up my code before I could. That would happen in CI and, if I already have Kustomize there I might as well do it at that stage.
2. Adding a git/repo or folder structure artifact would have increased the scope of this change dramatically. I agree that we should have something that represents that in the future but this seemed like a reasonable solution until we can agree on what that would look like.

I'm happy to call this feature "beta" for the 1.16 release. I've already put it behind a feature flag in Deck. Would this solution be reasonable if the logic were hardened/tested more? If so, is this something you'd feel comfortable merging or are we calling this a "no-go"?

[ezimanyi]

I think the solution is reasonable; while it might have been good to think a bit more broadly, I don't think this is a bad solution, I just wonder a bit about the tradeoffs between the options noted above. That was more of a general comment about wishing we'd have a more open design document as I read the complexity of the implementation, not something I think we need to address/fix here.

From a more tactical perspective about this in 1.16, my main concern is just that I can't really convince myself this is correct---I've read it through a few times and am still having trouble understanding exactly what it's doing (or finding enough tests to convince me). At a minimum I'd want to see the few bugs noted in the latest review fixed before merging this...but in particular the one about dropping a slash from all URLs (https:/my-url/) makes me wonder how much this has been tested. Until that stuff is fixed, I'd say this isn't even at something I'd call "beta" yet, but with that and some more tests to validate the fixes to those bugs then this could be mergeable.