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

Make Helm values field templatable using Kustomize #2936

Closed
carlosjgp opened this issue Jan 2, 2020 · 15 comments · Fixed by #11538
Closed

Make Helm values field templatable using Kustomize #2936

carlosjgp opened this issue Jan 2, 2020 · 15 comments · Fixed by #11538
Labels
component:config-management Tools specific issues (helm, kustomize etc) enhancement New feature or request type:usability Enhancement of an existing feature

Comments

@carlosjgp
Copy link

Summary

Use a YAML structure for argoproj.io/v1alpha1/Application.spec.source.helm.values instead of string to allow Kustomize to change properties per environment

Motivation

Helm allows to template charts using YAML files typically called values.yaml files

I found myself repeating code across environments for charts that include their configuration on these values.yaml files, like Prometheus chart.
It happens that loads of apps are using YAML files themselves to be configured

ArgoCD currently supports Kustomize which is a great tool to template YAML files and could solve this problem easily but since argoproj.io/v1alpha1/Application.spec.source.helm.values is a string Kustomize can't change few properties.

In this particular case, it could be even better since Prometheus configuration itself is expressed on YAML format so I could tweak tiny bits thanks to Kustomize

- prometheus-kustomize
  |- base
  |  |- kustomization.yaml
  |  |- prometheus.yaml
  |- ci
  |  |- kustomization.yaml
  |  |- patch.yaml
...

prometheus.yaml would contain all the common configuration

  • chart repo
  • chart version
  • release name
  • Alertmanager global webhook shared across all environments but without receivers
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: prometheus
  namespace: argocd
spec:
  source:
    chart: prometheus
    helm:
      releaseName: prometheus
      values:
        alertmanagerFiles:
          alertmanager.yml:
            global:
              slack_api_url: https://hooks.slack.com/services/xxxxxxx

patch.yaml little changes like which channel on Slack will receive the notifications

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: prometheus
  namespace: argocd
spec:
  source:
    helm:
      values:
        alertmanagerFiles:
          alertmanager.yml:
            receivers:
              - name: prometheus-alert
                slack_configs:
                  - channel: '#ci-alerts'

Proposal

Change the field spec.source.helm.values to be a generic YAML structure (map?) to allow Kustomize to template these values

@carlosjgp carlosjgp added the enhancement New feature or request label Jan 2, 2020
@jannfis jannfis added component:config-management Tools specific issues (helm, kustomize etc) type:usability Enhancement of an existing feature labels May 14, 2020
@lienerieksta
Copy link

I second this request. The benefits of having yaml values (just as flux does them) are more than just ability to kustomize them.

Yaml syntax highlighting would be preserved - Less errors and easier to debug.

Easier to manage by various CI tools - One could just edit the application definition directly, commit it back to the repository and be done with it. As it is now it's a hassle to get the values variable first and then parse it again and then write the variable and then write it back to Application.

The values are not even parsed and merged with chart defaults in the UI. This leads to incorrect app parameters shown in the UI and it's just asking for erroneous troubleshooting decisions.

Forcing user to split the values and define some as parameters, just to have an accurate depiction in UI and cleaner CI code, complicates the manifest and makes it harder to read. In a gitops situation I see that as a workaround to get the same results that could be achieved by values alone.

Tbh, I'm surprised that this issue has so little support. Though I account it to poor documentation - it does not even hint in the docs that you can define values in a variable. It's just shows in the example Application yaml. This and the huge amount of issues that beg for ability to fetch a values file from a separate location indicates that a good part of them either have not found the feature even exists or are not happy with the implementation. I'm sure that changing the values string to a yaml format and documenting it properly would appease most of those requests.

@jacobscunn07
Copy link

I am currently working on a POC for Argo CD and this is something that I've run into. I'd love to be able to define the base values for a helm chart, but then override them in a kustomize overlay. As mentioned, I'm doing a POC over ArgoCD and Flux v2 and a couple of core features I am looking for is to declaratively define helm chart releases and support for kustomize. Ideally, the two would work together as well.

@carlosjgp
Copy link
Author

carlosjgp commented Apr 13, 2021

As a workaround, my previous company did develop a custom plugin using Kustomize to render the expected ArgoCD Application of type helm

I hope they open source it soon but it's not crazy complicated to bake your own... they did use Python but any programming language could be used for this purpose

It'd be a winning hand for Argo to have this when it's time to compare with other GitOps solutions

@mcanevet
Copy link
Contributor

Would it be possible to use an array of maps instead of a map so that we can pass multiple values as the helm binary supports it?

helm template --help
...
  -f, --values strings               specify values in a YAML file or a URL (can specify multiple)
...

i.e.

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: prometheus
  namespace: argocd
spec:
  source:
    chart: prometheus
    helm:
      releaseName: prometheus
      values:
        - alertmanagerFiles:
            alertmanager.yml:
              global:
                slack_api_url: https://hooks.slack.com/services/xxxxxxx
        - alertmanagerFiles:
            alertmanager.yml:
              receivers:
                - name: prometheus-alert
                  slack_configs:
                    - channel: '#ci-alerts'

@hamza3202
Copy link

Has there been any progress on this?

I am currently working on a POC for Argo CD and this is something that I've run into. I'd love to be able to define the base values for a helm chart, but then override them in a kustomize overlay. As mentioned, I'm doing a POC over ArgoCD and Flux v2 and a couple of core features I am looking for is to declaratively define helm chart releases and support for kustomize. Ideally, the two would work together as well.

@jacobscunn07 How did you progress with this? I am in exactly the same situation as you

@tiagomeireles
Copy link

I'm also being burned by this. Is this something that the project would consider?

@puckettgw
Copy link

Wow, I thought I was doing it wrong. This feature really should be implemented... pretty please?
I was so excited to use ArgoCD instead of the various other tools I had been using. But I need to be able to feed an array, map, SOMETHING besides a string to my Helm chart.

@mcbenjemaa
Copy link

mcbenjemaa commented Feb 20, 2022

Hey guys,

I know most of you are dealing with this issue!

I have already tackle around to find a solution, so created an enhancement proposal #8579
and submitted a PR #8580 I hope it will be accepted and merged soon.

Please support this enhancement by (+1) in the proposal.

You can also check my demo repo: https://github.com/mcbenjemaa/argocd-apps

if anyone is interested to test it please use this image medchiheb/argocd-extra-values:alpha.

@crenshaw-dev
Copy link
Member

I think the best possible version of this proposal is to make the values field flexibly-typed (maybe JSON type would be appropriate?). That would preserve backwards-compatibility (people could pass stringified YAML) and allow people to use an arbitrary YAML structure if they want.

PR #8580 would instead add a new extraValues string (or string array) field, which I think undercuts the main advantage people are looking for (a single, simple source of truth for values). With multiple fields you have to think about precedence to understand the final values used in the chart.

I'm not opposed to a new field if it really solves the problems people are encountering. But I'm not sure how a new field would be preferable to either of these workarounds:

  1. Use kustomize patches to write individual parameter overrides. In other words, patch this:
apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  source:
    helm:
      parameters:
      - name: "nginx-ingress.controller.service.annotations.external-dns\\.alpha\\.kubernetes\\.io/hostname"
        value: mydomain.example.com
      - name: "ingress.annotations.kubernetes\\.io/tls-acme"
        value: "true"
        forceString: true # ensures that value is treated as a string
  1. Use the valueFiles field to simply pass in overriding values file paths:
apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  source:
    helm:
      valueFiles:
      - values-prod.yaml

Am I correct that those workarounds are at least as good as adding a new extraValues field? If so, I think we should keep looking for a way to make values flexibly-typed (while maintaining backwards compatibility).

@alexec
Copy link
Contributor

alexec commented Mar 3, 2022

In workflows we tend to introduce a new field when we change type because it is both much simpler implemenatation wise, and easier to use.

manifest: |
  a manifest as long string
manifestFrom:
  url: "the url to download from"
  value:
    a: "structued YAML value"

@wmiller112
Copy link
Contributor

wmiller112 commented Jun 15, 2022

I think the best possible version of this proposal is to make the values field flexibly-typed

Definitely think this would be best case, but any field that allows arbitrary yaml passed would get the job done.

Am I correct that those workarounds are at least as good as adding a new extraValues field? If so, I think we should keep looking for a way to make values flexibly-typed (while maintaining backwards compatibility).

Option 1 would theoretically work, the drawback being that its ugly and tedious to write out every yaml path, rather than just defining the yaml. I also found that I cannot use strategicMergePatch for the parameters field because the field/crd is not supported (at least by default, I think theres a way to declare the crd fields with transformers or something, but that seems excessive). This means we'd have to use json patch, which adds to the ugly/tediousness for each parameter.

Option 2 would work in some cases, but only when the chart is in the git repo where the app is defined, which most of the time is not the case (packaged internally managed charts or 3rd party charts).

@appellod
Copy link

This is the only thing keeping me from switching from Flux to Argo CD, and trust me, I want to use Argo CD very badly.

@jsinme
Copy link

jsinme commented Nov 9, 2022

also running into this issue requiring us to be very careful with promoting changes between envs. would love to see this added

@conman2305
Copy link

Any update on this? I see #8579 got closed, so are we back to square 1 here?

@crenshaw-dev
Copy link
Member

@conman2305 yep I think we're back to square 1. I don't think I was far from getting it to work, but I just don't have the time to dedicate to it right now.

If anyone wants to pick up the PR, I'm happy to help guide them.

crenshaw-dev added a commit that referenced this issue Jun 26, 2023
* feat: values can be either a string or a map

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Allow viewing and editing values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix golang lint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen make build green

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add special cases for handling nil/empty string. strip newline at end of generated yaml. fix unittests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add tests. Fix e2e marshalJSON

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen. make codegen and make codegen-local give different results to me

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Update helm_test.go

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix UI - validate input

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* change implementation; introduce valuesObject instead of values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* More generated files. Fix tests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix eslint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: use sigs.k8s.io/yaml

In the same vein as #13292, since `github.com/ghodss/yaml` is no longer
maintained.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: compact values

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make manifests

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add to helm user guide

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: remove any deprecation mentions

Remove any mentions that `Values` is deprecated, for now.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: add assertion

verify that valuesobject overrides values by checking the number of
replicas.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: remove assertion

This wasn't meant to be in there, was a typo.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: minor nit fix

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

---------

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this issue Aug 9, 2023
… (argoproj#11538)

* feat: values can be either a string or a map

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Allow viewing and editing values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix golang lint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen make build green

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add special cases for handling nil/empty string. strip newline at end of generated yaml. fix unittests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add tests. Fix e2e marshalJSON

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen. make codegen and make codegen-local give different results to me

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Update helm_test.go

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix UI - validate input

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* change implementation; introduce valuesObject instead of values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* More generated files. Fix tests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix eslint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: use sigs.k8s.io/yaml

In the same vein as argoproj#13292, since `github.com/ghodss/yaml` is no longer
maintained.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: compact values

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make manifests

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add to helm user guide

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: remove any deprecation mentions

Remove any mentions that `Values` is deprecated, for now.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: add assertion

verify that valuesobject overrides values by checking the number of
replicas.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: remove assertion

This wasn't meant to be in there, was a typo.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: minor nit fix

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

---------

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
… (argoproj#11538)

* feat: values can be either a string or a map

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Allow viewing and editing values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix golang lint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen make build green

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add special cases for handling nil/empty string. strip newline at end of generated yaml. fix unittests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Add tests. Fix e2e marshalJSON

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* fix codegen. make codegen and make codegen-local give different results to me

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Update helm_test.go

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix UI - validate input

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* change implementation; introduce valuesObject instead of values

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* More generated files. Fix tests

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* Fix eslint

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: use sigs.k8s.io/yaml

In the same vein as argoproj#13292, since `github.com/ghodss/yaml` is no longer
maintained.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: compact values

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make manifests

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add to helm user guide

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: remove any deprecation mentions

Remove any mentions that `Values` is deprecated, for now.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: make codegen

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: add assertion

verify that valuesobject overrides values by checking the number of
replicas.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: remove assertion

This wasn't meant to be in there, was a typo.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* chore: minor nit fix

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

---------

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment