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.

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

Add include'able sub-helmfiles #96

Closed
gtaylor opened this issue Apr 6, 2018 · 29 comments
Closed

Add include'able sub-helmfiles #96

gtaylor opened this issue Apr 6, 2018 · 29 comments

Comments

@gtaylor
Copy link

gtaylor commented Apr 6, 2018

  • We are managing the full state of our clusters with helmfile at Reddit. Some of our helmfiles are getting quite large.
  • This makes it difficult for our less Kubernetes-familiar folks to wade through and find what they're looking for.
  • We also can't use GitHub CODEOWNERS on any granular level.

It would be really nice if there was a notion of an include'able sub-helmfile. This would help us segment a cluster's workload definition by department, topic, or whatever else.

I am not at all tied to any particular implementation, but here's an example:

# helmfile.yaml
repositories:
  - name: roboll
    url: http://roboll.io/charts
    certFile: optional_client_cert
    keyFile: optional_client_key

context: kube-context					 # kube-context (--kube-context)

# Not sure what to name this. Could also be 'helmfiles'?
includes:
  - team_a_helmfile.yaml
  - team_b_helmfile.yaml

releases:
  # Published chart example
  - name: vault                            # name of this release
    namespace: vault                       # target namespace
    labels:                                  # Arbitrary key value pairs for filtering releases
      foo: bar
    chart: roboll/vault-secret-manager     # the chart being installed to create this release, referenced by `repository/chart` syntax
    version: ~1.24.1                       # the semver of the chart. range constraint is supported
    values: [ vault.yaml ]                 # value files (--values)

How this might work:

  1. Repositories in helmfile.yaml are added
  2. Context is set from helmfile.yaml
  3. We parse and load each helmfile's (in order of definition) releases and repositories (I think we have to ignore context, as that could muck things up a bit).
  4. If we encounter a release name that we've already parsed/stored, we replace it with the last defined instance (based on ordering in instances). You still can't have multiple releases of the same name within a single hemlfile, as that'd be invalid yaml.
  5. Once all of the includes helmfiles are parsed and loaded into memory, we return to the main helmfile.yaml and load its releases. Again, since this is the last in line, the root helmfile can override any releases in the includes helmfiles.

Other notes:

  • I don't think we'd need to get as fancy as trying to more gracefully merge releases of the same name. We could start by replacing/overriding them wholesale. This would allow us to do neat things like have two clusters with very similar workloads (and a common set of sub-helmfiles), overridable as needed.
  • This wouldn't be necessary to have an answer for in v1, but perhaps you could also override a previous helmfile's release, but in a way that leads the release to not be installed. IE: Use this common base helmfile, but disable this particular release in my cluster-specific helmfile.

cc @foklepoint

@manics
Copy link
Contributor

manics commented Apr 6, 2018

There's a similar discussion with more use-cases happening on #59

@gtaylor
Copy link
Author

gtaylor commented Apr 6, 2018

Thanks for the tip. Though, I think we'd be more interested in splitting up the actual releases as opposed to defining default values (as per #59).

@sstarcher
Copy link
Contributor

@gtaylor Thoughts on instead of doing an include being able to glob the files

files in the folder

  • main_helmfile.yaml
  • team_a_helmfile.yaml
  • team_b_helmfile.yaml

helm sync -f *helmfile.yaml

or maybe loading all helmfiles in a directory helm sync -d helmfile where helmfile is a directory

The complexity of the overriding and collisions concerns me. Instead of allowing things to be override I would prefer they collide and error out. You might be able to handle your overriding between environments if my proposal here is taken into account #59 (comment) and this PR #98

@gtaylor
Copy link
Author

gtaylor commented Apr 8, 2018

Since we'd like the ordering to be significant (to allow for the similar-but-separate cluster overrides), that makes a globbing approach clumsy to deal with.

What collisions are we talking about? We have release names to key off of. If a release has been seen before, we blow the existing struct away and replace it with the most recently seen. That can be managed with something like a map[string]X.

@sstarcher
Copy link
Contributor

sstarcher commented Apr 8, 2018

ya, exactly I find out odd to blow away a previous structure for collisions. Why do you want ordering to be significant? I think it's a misnomer to want to consider releases to be significant or do you mean the merging of includes? I think the merging could be better handled by values files that are loaded per environment.

With your example why would you ever want team b to be able to override the release from team a?

@osterman
Copy link
Contributor

Also, to add a use-case, we're creating a distribution of helm charts that install out-of-the-box on kops. Our helmfile.yaml is currently 500 lines and will probably double as soon as we add prometheus.

https://github.com/cloudposse/geodesic/pull/124/files

@sstarcher
Copy link
Contributor

@osterman adding support for templated values files looks like it would dramatically reduce the size of your helm chart. #97

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 26, 2018

I'm going to suggest something possibly terrible today!

How about adding dependencies section to helmfile.yaml:

dependencies:
- prod/org.yaml
- prod/team.yaml
- _helpers.tpl
  • Every dependency prefixed with _ like _helpers.tpl can contain {{ define "helpername" }}s only.
  • prod/org.yaml, prod/team.yaml and the main helmfile.yaml is merged in this order to support @gtaylor's use-case

But I'd recommend not to do too much in prod/*.yaml due to its implied complexity. Hopefully you make prod/*.yaml to contain fewer {{env directives to make it straightforward. Composition over inheritance is also encouraged.

_helpers.tpl is there for everyone prefers composition over inheritance. You can leverage any sprig func to programmatically generate your final helmfile.yaml in a DRY way, without inheritance.

The source of inspiration for this is helm's named templates and chart dependencies.

So, hopefully this makes sense to helm users, while supporting all the use-cases gathered until now...

WDYT?

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 26, 2018

Also see #59 for default values templating.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 26, 2018

The implementation should be simple. Probably it is a matter of some extra file loading through template.ParseFiles, render YAMLs in the order, mergo rendered YAMLs in the order. So, helmfile's implementation shouldn't be that bloated.

But beware, the requested helmfile lint(#58) and dry-run(#118) features would be your best friends...

I also suggest jsonnet or else if you get to find the life in this helm'ish way of reusability too hard. If you go that way, you can just remove almost all the sprig expressions from your sub-helmfiles and apply templating externally.

@gtaylor
Copy link
Author

gtaylor commented Apr 27, 2018

That would be great! Though, I'm not sure we should use the word "dependency", since that has existing (and different) meaning in Helm. include was originally suggested (since we're including/merging other files in), but imports is another idea if that's no good.

@sstarcher
Copy link
Contributor

I'm still interested in the ability to load values that can be used for interpolation as I view that as alot more powerful than merging yamls together. Especially for complex charts that don't follow the normal hierarchy.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 27, 2018

@sstarcher I guess you're talking about an alternative to envvars referenced via {{env - something like a values.yaml for helmfile itself - right?

@sstarcher
Copy link
Contributor

ya, I believe that would solve the issues with reusable data more cleanly then merging values files together as they are limited in structure.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 28, 2018

@gtaylor Good point! I originally took it inspired by helm's requirements.yaml thinking it would be easier for helm users to understand.

But I now realize that it is rather confusing(obvious?) as helmfile.yaml isn't version-controlled yet, hence it is different than helm chart's dependencies.

Some more thoughts:

  • imports sounds clean. But if I go nit-picky, it sounds like it isn'y merged automatically to the main helmfile.yml.
  • include sounds clean. But if I go nit-picky, it sounds like it is overriding the keys and values in main helmfile.yaml. In reality it is the main helmfile.yaml who overrides included sub-helmfiles but include sounds like doing the opposite of that.

Forgive me about I'm not yet sure what the best name would be. dependencies would be the worst idea anyway!

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 28, 2018

@sstarcher I believe it would be very nice to have.

Several questions came up in my mind:

  • Do you want to encrypt it, probably with helm-secrets if present?
  • Do you need templating in it?
  • Do you want to always specify the values.yaml of helmfile one? Or multiple times while the latter overrides values present in previous ones?
  • Do you need a convention that the file named like helmfile-values.yaml(terrible naming 😉) to be automatically loaded when you have not specified one?

@sstarcher
Copy link
Contributor

sstarcher commented Apr 28, 2018

@mumoshu if that's in response to my statement we should likely split that off into another ticket. Which might be #59

@AssafKatz3
Copy link

Allow recursive include - prod_helmfile.yaml includes stage_helmfile.yaml includes dev_helmfile.yaml - will be good (see #29).

@sstarcher
Copy link
Contributor

I'm very concerned about the complexity of inclusion and recursive inclusions.

@osterman
Copy link
Contributor

@gtaylor what do you think about this proposal? #151

@gtaylor
Copy link
Author

gtaylor commented May 22, 2018

@osterman if the helmfiles are executed in order by filename (alphanumeric?), this seems like it'd work. Consistent ordering would be very important to us.

I think this would still play nicely with Github code owners as well.

@osterman
Copy link
Contributor

Yea, I think they should be executed lexicographically. To influence the order of execution, one could organize files like this:

helmfiles.d/10.kube2iam.yaml
helmfiles.d/20.chartmuseum.yaml
helmfiles.d/30.externaldns.yaml

@mumoshu
Copy link
Collaborator

mumoshu commented May 25, 2018

@gtaylor Just to make sure that I don't miss your use-case - do you still need the "include" feature on top of #151?

Regarding your original motivation, my guess is that with #151 you can separate helmfiles and apply it selectively according to the department and the topic and whatever else?

@AssafKatz3
Copy link

@mumoshu I don't know about @gtaylor, but we need :-) since we will also have central HELM file of application level (HELM file should be per micro service)

@gtaylor
Copy link
Author

gtaylor commented May 27, 2018

@mumoshu #151 should cover our usage case.

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 29, 2018

@AssafKatz3 Hey! First of all, #227 allows you to import arbitrary yaml files into helmfile.yaml via templates. In case your use-case is not fully supported with that and the helmfile.d feature #151 #160, PTAL at my next proposal #247.

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 30, 2018

@AssafKatz3 PTAL at #254 too. It proposes a feature that is very similar to includable helmfiles.

@AssafKatz3
Copy link

@mumoshu We put this effort on hold due to more other issues. #247 and #254 seem to cover what we need but it will take sometime until we will be able to test it.

@gtaylor
Copy link
Author

gtaylor commented Sep 13, 2018

This issue is satisfied by the new helmfile directory support. Thanks to all who contributed to make that happen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants