roboll / helmfile

Deploy Kubernetes Helm Charts
MIT License
4.04k stars 565 forks source link

Deprecate `set` in helmfile.yaml #243

Open mumoshu opened 6 years ago

mumoshu commented 6 years ago

Ref https://github.com/roboll/helmfile/issues/227#issuecomment-416465961

You should use values files templates not to bloat up your helmfile.yaml

goruha commented 6 years ago

--set-file in difference to readFile allow to use urls like

  set:
  - name: "configMaps.dashboards.files.deployment\\.json"
    file: https://raw.githubusercontent.com/cloudposse/grafana-dashboards/feature-kube-prometheus-dashboards/kube-prometheus/deployment-dashboard.json

Very useful.

But if readFile will be allow to fetch url endpoints - no problem with deprecation.

Stono commented 6 years ago

Eekk this would be a really breaking change for us in our CI/CD environment. Take this example, which sets the tag for the image to be equal to the build pipeline number (the previous step pushes the image tagged with that number).

We do this a lot

releases:
- name: sauron-web
  namespace: sauron-web
  chart: aws/platform-helm-at-service
  version: 0.1.{{ requiredEnv "GO_DEPENDENCY_LABEL_PLATFORM" }}
  values:
  - environments/values.yaml
  - environments/{{ requiredEnv "HELMFILE_ENV" }}/values.yaml
  secrets:
  - environments/{{ requiredEnv "HELMFILE_ENV" }}/secrets.yaml
  set:
  - name: deployment.containers[0].image.tag
    value: {{ requiredEnv "GO_PIPELINE_COUNTER" }}
mumoshu commented 6 years ago

@goruha Thanks! I'm aware of the feature. Good to know about someone who actually use the feature!

I'll add a dedicated template function to fetch a remote url, and will ask for your confirmation before I actually deprecate or even remove the set: thing.

mumoshu commented 6 years ago

@Stono Your use-case is completely valid!

Unfortunately, I have no idea how we could cleanly support it with the template values file feature. Don't worry. I won't remove the set: feature without your confirmation.

sstarcher commented 6 years ago

@Stono any reason you don't put that

set:
  - name: deployment.containers[0].image.tag
    value: {{ requiredEnv "GO_PIPELINE_COUNTER" }}

In your values.yaml?

goruha commented 6 years ago

@mumoshu thanks!

Stono commented 6 years ago

@sstarcher i'm confused, do you mean in the charts values.yaml? That'd go to helm and helm doesn't support requiredEnv?

sstarcher commented 6 years ago

@Stono If you rename - environments/values.yaml to values.yaml.gotmpl helmfile will template that and it supports everything that can be done in the primary helmfile.

mumoshu commented 6 years ago

@Stono More concretely, you could add values.yaml.gotmpl like:

deployment:
  containers:
  - image:
      tag: {{ requiredEnv "GO_PIPELINE_COUNTER" }}

And add the filename to values: in helmfile.yaml:

releases:
- name: sauron-web
  namespace: sauron-web
  chart: aws/platform-helm-at-service
  version: 0.1.{{ requiredEnv "GO_DEPENDENCY_LABEL_PLATFORM" }}
  values:
  - environments/values.yaml
  - environments/{{ requiredEnv "HELMFILE_ENV" }}/values.yaml
  - values.yaml.gotmpl
  secrets:
  - environments/{{ requiredEnv "HELMFILE_ENV" }}/secrets.yaml

Note that the gotcha here is that the values.yaml generated from values.yaml.gotmpl would replace the entire deployment.containers[] yaml array. So in case you have a secondary container defined by default in preceding values.yaml files, modify your chart to change deployment.containers from an array to a hash.

After that your values.yaml.gotmpl becomes:

deployment:
  containers:
    primary:
      tag: {{ requiredEnv "GO_PIPELINE_COUNTER" }}
blissd commented 6 years ago

I find set to be very convenient when using helmfiles for testing. I'd hate to see it go.

mumoshu commented 6 years ago

@blissd You seem to be talking about the --set flag, right? That's another story than this. I'm not going to remove the flag at all.

blissd commented 6 years ago

No, I mean set in the helmfile.yaml itself. We've been using v0.19, which I think predates the gotmpl files, but I'd like to upgrade to a newer version.

However, I'd be hesitant to use the gotmpl files because I like my values files to be the same as what would be passed to helm without using helmfile.

mumoshu commented 6 years ago

@blissd Thanks for clarifying! Makes sense, but do you know inline values in helmfile.yaml? That's basically better alternative to set:

blissd commented 6 years ago

Thanks, the inline values work fine for my use case. I'll have the team here migrate to that.

philomory commented 4 years ago

If we do end up deprecating set in favor of inline values, could we also merge the values and secrets sections in some way? Right now, it's impossible to specify a values file that will have higher priority than a secrets file, e.g., this command line cannot be replicated: helm upgrade --install -f values.yaml -f secrets.yaml -f values.prod.yaml -f secrets.prod.yaml.

If you write it like this:

values:
  - values.yaml
  - values.{{ .Environment.Name }}.yaml
secrets:
  - secrets.yaml
  - secrets.{{ .Environment.Name }}.yaml

... you'll end up with the following command: helm upgrade --install -f values.yaml -f values.prod.yaml -f secrets.yaml -f secrets.prod.yaml; that gives items from secrets.yaml higher priority than those in values.prod.yaml during the recursive merge.

mumoshu commented 4 years ago

@philomory Thanks! Yeah that's a valid point. I was thinking if we could just use remote secrets though. WDYT?

philomory commented 4 years ago

That's great if you've got your secrets in Vault, it's a bit more awkward when each chart your deploying has a chart-specific helm-secrets YAML file, and some of those files for specific microservices might have the same key with different values.

philomory commented 4 years ago

Actually, I suppose ref+sops:// would solve that case? It'd be nice if vals and go-getter could join forces to read a sops-encrypted file from a remote git repo, but maybe that's asking too much (and obviously that's unrelated to whether the set section is deprecated or not).

mumoshu commented 4 years ago

@philomory Yeah I thought ref+sops:// in values could replace secrets.

Re: reading a sops encrypted file from a remote git repo, that sounds like an interesting use-case indeed :) Created https://github.com/variantdev/vals/issues/35 to track it.

philomory commented 4 years ago

@mumoshu So... does ref+sops:// actually work? I tried the following:

# helmfile.yaml
values:
  - ref+sops://secrets.yaml?format=yaml

releases:
  - name: dummy
    chart: incubator/raw
    values:
      - {{ .Values | toYaml | nindent 8 | trimPrefix " " }}
# secrets.yaml (before encrypting with sops)
foo: bar

Results:

$ helmfile template
in ./helmfile.yaml: environment values file matching "ref+sops://secrets.yaml?format=yaml" does not exist in "."

There's not actually any examples of how to use ref+sops:// anywhere in the Helmfile documentation (not even the PRs linked from the documentation), they only show ref+vault://; but using the ref+sops:// syntax from the vals documentation doesn't seem to work for me.

EDIT: Note, using vals directly does work as expected, if I strip out the templating:

$ vals eval -f helmfile.yaml
releases:
  - chart: incubator/raw
    name: dummy
    values:
      - null
values:
  - |
    foo: bar

(Well, it at least loads the secrets file, decrypts it and gets the values out; it renders it as raw rather than embedding it in the YAML document but I can't even get that far with Helmfile)

philomory commented 4 years ago

Ok, I realized what my mistake was: you can't use ref+sops:// (or, presumably, any other vals lookup) at the top level to load an entire values "file" (array entry) from an encrypted file, you can only use it to load individual values within a set. Unfortunately, I think until something like the syntax I presented above is available, I don't think it's really feasible to trade the secrets section for ref+sops://, which, in turn, means the ordering issue of secrets vs vanilla values remains a problem.