roboll / helmfile

Deploy Kubernetes Helm Charts
MIT License
4.05k stars 564 forks source link

Prescriptive directory structure for "magic" values files #428

Closed mattparkes closed 5 years ago

mattparkes commented 5 years ago

I've written a helmfile containing abut 40 releases and I'm having to repeat myself a lot:

Example:

  - name: heapster
    namespace: kube-system
    chart: stable/heapster
    version: 0.3.2
    values:
      - "./values/logging/heapster/common.yaml"
      - "./values/logging/heapster/{{ .Environment.Name }}.yaml"
    secrets:
      - "./values/logging/heapster/common-secrets.yaml"
      - "./values/logging/heapster/{{ .Environment.Name }}-secrets.yaml"

  - name: kubernetes-dashboard
    namespace: kube-system
    chart: stable/kubernetes-dashboard
    version: 0.10.0
    values:
      - "./values/kube-system/kubernetes-dashboard/common.yaml"
      - "./values/kube-system/kubernetes-dashboard/{{ .Environment.Name }}.yaml"
    values:
      - "./values/kube-system/kubernetes-dashboard/common-secrets.yaml"
      - "./values/kube-system/kubernetes-dashboard/{{ .Environment.Name }}-secrets.yaml"

One feature I really like about Ansible is that it automatically looks for it's values files for each "playbook" in a certain directory structure, and looks in multiple locations in a known order, allowing easy environment specific overrides, etc.

I think it would be really nice if helmfile automatically looked for a values/<namespace>/<release-name> directory and then automatically loaded (in order):

  1. A common file regardless of environment (maybe values.yaml or common.yaml).
  2. An environment specific file (e.g production.yaml).
  3. Any files explicitly listed in the helmfile.yaml (in order).

And then a similar process for secrets (either in a secrets/<namespace>/<release-name> directory or in values/<namespace>/<release-name>, with files suffixed with -secrets treated as secrets e.g production-secrets.yaml.)

Another things is that if helmfile can't stat the file, it errors out (which is great in that it stops mistakes from missing files), but it would be nice if a a switch could be set so that a certain file and/or directory not existing is a warning, not an error.

Example Directory Tree:

values/
    kube-system/                      # 'kube-system' namespace
        dashboard/                    # 'dashboard' release
            production.yaml           #  production Environment specific values
            production-secrets.yaml   #  production Environment specific secrets
            staging.yaml              #  staging Environment specific values
            values.yaml               #  contains values common to all Environments
        heapster/
            values.yaml               #  only common values.yaml is provided, no environment specific values are needed.
    web/                              # 'web' namespace
        [..]                          # sub-directories similar to above

Then my helmfile.yaml could be simplified to:

  - name: heapster
    namespace: kube-system
    chart: stable/heapster
    version: 0.3.2

  - name: kubernetes-dashboard
    namespace: kube-system
    chart: stable/kubernetes-dashboard
    version: 0.10.0
vbehar commented 5 years ago

see https://github.com/roboll/helmfile/pull/408 for how to use the new valuesPathPrefix, which is a first step for what you want to do

sstarcher commented 5 years ago

I have similar boilerplate that I use over and over again

accounts/
  dev/
  stg
    prometheus/
       values.yaml
       secrets.yaml
releases/
  prometheus/
    values.yaml
mattparkes commented 5 years ago

408 Reduces the length of the lines, but not the number of lines, which is my ultimate goal with this suggestion.

I'll definitely updated my helmfile to make use of that feature though

royjs commented 5 years ago

I'm currently trying to do pretty much the same thing. I want to be able to specify override values for some environments. Right now it seems that I need to specify all files and create empty files for environments that have no overrides. Something like this would be really helpful.

mumoshu commented 5 years ago

I had been considering this as an important problem that should be fixed. But I had no idea how we can universally fix it. It is possible if you use {{define in the template but I think that's not what you want.

What if we add helmfile "mixins"? A mixin is a reusable snippet of releases that can be included to releases. The mixin is evaluated before merged into the targeted release.

mixins:
  foo:
    # New configuration keys. The current behavior is 'Error'
    onMissingValuesFile: Warn
    onMissingSecretsFile: Warn
    values:
     # IMPORTANT: {{` {{ .Release.Name }} `}} is surrounded by {{` and `}} not to be executed on the loading time of helmfile.yaml.
     # We need to defer it until each release is actually processed by sync|diff|apply|etc.
      - "./values/logging/{{` {{ .Release.Name }} `}}/common.yaml"
      - "./values/logging/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}.yaml"
    secrets:
      - "./values/logging/{{` {{ .Release.Name }} `}}/common-secrets.yaml"
      - "./values/logging/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}-secrets.yaml"

releases:
- name: heapster
  namespace: kube-system
  chart: stable/heapster
  version: 0.3.2
  # Selectively include the mixin
  include:
  - foo
- name: kubernetes-dashboard
  namespace: kube-system
  chart: stable/kubernetes-dashboard
  version: 0.10.0
  include:
  - foo

The default include can be used to prevent repeating include in releases. BTW I'd like to rename helmDefaults to defaults for the sake of simpicity before this. Then you can write it even shorter:

mixins:
  foo:
    # New configuration keys. The current behavior is 'Error'
    onMIssingValuesFile: Warn
    onMIssingSecretsFile: Warn
    values:
      - "./values/logging/{{` {{ .Release.Name }} `}}/common.yaml"
      - "./values/logging/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}.yaml"
    secrets:
      - "./values/logging/{{` {{ .Release.Name }} `}}/common-secrets.yaml"
      - "./values/logging/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}-secrets.yaml"

defaults:
  include:
  - foo

releases:
- name: heapster
  namespace: kube-system
  chart: stable/heapster
  version: 0.3.2
- name: kubernetes-dashboard
  namespace: kube-system
  chart: stable/kubernetes-dashboard
  version: 0.10.0
sstarcher commented 5 years ago

That seems reasonable and I like the addition of onMissing

royjs commented 5 years ago

yeah I also like the onMissingValuesFile addition. I think that it's really what is missing right now. And the mixins idea would really help avoiding duplication

mattparkes commented 5 years ago

I love the idea , but I'm not sure about about the terminology "mixin". (I see it's popular for Vue, so maybe other people are more familiar with the term.)

Maybe "templates" with a special entry called "default" which is applied to all releases (if it exists) and then we can move all the stuff currently in helmDefaults into this.

This allows us to have most releases use this default template and then specify different templates when required and still override singular values on a per-release basis (like we currently can):

templates:
  default:
    # New configuration keys. The current behavior is 'Error'
    missingValues: Warn
    missingSecrets: Warn
    values:
      - "./values/{{` {{ .Release.Name }} `}}/common.yaml"
      - "./values/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}.yaml"
    secrets:
      - "./values/{{` {{ .Release.Name }} `}}/common-secrets.yaml"
      - "./values/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}-secrets.yaml"
    tillerNamespace: default
    verify: false
    recreatePods: true

  mustfail:
    missingValues: Error
    missingSecrets: Error
    verify: true
    recreatePods: false
releases:
- name: heapster
  namespace: kube-system
  chart: stable/heapster
  version: 0.3.2
  template: default     # 'default' is special and normally implicit, but I've explicitly defined it here as an example 

- name: kubernetes-dashboard
  namespace: kube-system
  chart: stable/kubernetes-dashboard
  version: 0.10.0
  template: mustfail    # mustfail is a combination of default with 'mustfail' (defined above) merged/replaced over the top

- name: nginx-ingress
  namespace: kube-system
  chart: stable/nginx-ingress
  version: 1.0.0
  template: mustfail
  verify: false         # here we use mustfail, but then also override verify to false

Also since Helm release names are currently global (not per namespace) I've changed the directory structure to just values/release-name (in this comment only) to avoid potential confusion.

mumoshu commented 5 years ago

Maybe "templates" with a special entry called "default" which is applied to all releases (if it exists) and then we can move all the stuff currently in helmDefaults into this.

Thanks for the feedback! I like the idea a lot 👍

Are you ok with suffixing missingValues with Policy or FilePolicy? So that it looks missingValuesFilePolicy: Warn or missingValuesPolicy: Warn.

The intention here is to make it extra clear that it decides how(=policy) to handle missing values/secrets "file". It would be ok to omit the "file" part if it is already clear that it works against values files, not inline values.

mumoshu commented 5 years ago

Also, I think "template" is a good terminology that follows "pod templates" in k8s.

One reason I chosen "mixins" and "include" was to allow including multiple mixins for maximum reusability and composability.

But YAGNI, right? Or do you think it's a good idea to start with mixins and include now?

Even in that case, I'd definitely borrow your idea of using default as the set of things included to releases by default, and rephrasing helmDefaults to mixins.default combied with implicit include: [default]

mattparkes commented 5 years ago

Yeah missingValuesPolicy or missingValuesAction looks good.

I'm personally happy with either mixins or templates, especially since I'm currently only doing the easy part and writing the suggestion!

mattparkes commented 5 years ago

Your {{` {{ .Release.Name }} `}} syntax inspired me to try just use YAML Anchors and Extends.

Because Helmfile doesn't like invalid/arbitrary keys in the helmfile, I stuffed my values in a fake environment:

environments:
  staging_defaults: &defaults
    values:
      - "./values/{{` {{ .Release.Name }} `}}/common.yaml"
      - "./values/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}.yaml"
    secrets:
      - "./values/{{` {{ .Release.Name }} `}}/common-secrets.yaml"
      - "./values/{{` {{ .Release.Name }} `}}/{{ .Environment.Name }}-secrets.yaml"
releases:
- name: heapster
  namespace: kube-system
  chart: stable/heapster
  version: 0.3.2
  <<: *defaults

(If you're not familiar with YAML anchors, &defaults names the block, then *defaults references it. The <<: syntax says to "extend" (merge) that reference into the current tree.)

This almost works, but complains that values/{{.Release.Name}}/common.yaml cannot be found. (Presumably it doesn't "double" render this block?)

I wonder if a simpler way to achieve all this is to:

  1. Add an extra key to the helfile spec (templates)
  2. Make sure rendering of {{` {{ .Release.Name }} `}} is deferred until sync|diff|apply|template
  3. Add a complex example to the documentation using YAML Anchors and Extends in a templates block.

I only got to spend 10 minutes on this and haven't checked the code yet, so the above may actually already be possible (other than 1.)?

mumoshu commented 5 years ago

This almost works, but complains that values/{{.Release.Name}}/common.yaml cannot be found. > (Presumably it doesn't "double" render this block?)

Yep that's correct. And it must be implemented as an another mechanism than the double-rendering.

  1. Add an extra key to the helfile spec (templates)
  2. Make sure rendering of {{{{ .Release.Name }}}} is deferred until sync|diff|apply|template
  3. Add a complex example to the documentation using YAML Anchors and Extends in a templates block.

You've figured out the current state of helmfile perfectly! Yes, these 3 steps must be the minimum delta necessary to support the feature.

I once discarded the idea of relying on YAML anchors instead of template or include due to that it introduces a hard dependency to yaml, and no existing tool that does X-to-YAML conversions support yaml anchors.

But I might be thinking too much - I now believe we should just start with your idea :)

mumoshu commented 5 years ago

@mattparkes @sstarcher @royjs Just submitted a PR that adds Release Template #439. I appreciate it if you could give it a shot :)

mattparkes commented 5 years ago

Wow this is awesome! Thanks so much @mumoshu!

fzyzcjy commented 3 years ago

Hi I am a bit confused about the syntax.

I do see:

     # IMPORTANT: {{` {{ .Release.Name }} `}} is surrounded by {{` and `}} not to be executed on the loading time of helmfile.yaml.
     # We need to defer it until each release is actually processed by sync|diff|apply|etc.

but why does that happen? Is it a special syntax? I have tried searching without results :(

mumoshu commented 3 years ago

@fzyzcjy Hey. That's because helmfile.yaml itself is a go template file that should render a YAML document. So, it renders in two phases. The first phase renders the whole helmfile.yaml. The second phase renders go template expressions embedded in releases[].values and so on.

{{` {{ .Release.Name }} `}}

This is a go template comment. This makes the first phase to render

{{ .Release.Name }}

which is then rendered by the second phase.

fzyzcjy commented 3 years ago

@mumoshu Thanks!