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

.Release.Labels are not available within values templates #435

Closed
sisrael-dn opened this issue Oct 13, 2022 · 31 comments · Fixed by #604 or #609
Closed

.Release.Labels are not available within values templates #435

sisrael-dn opened this issue Oct 13, 2022 · 31 comments · Fixed by #604 or #609
Labels
help wanted Extra attention is needed

Comments

@sisrael-dn
Copy link

Operating system

macOS Monterey Version 12.5

Helmfile Version

0.147.0

Helm Version

3.10.1

Bug description

Since 0.147.0 it seems like .Release.Labels are no longer available to use within values templates.
Same example works fine with previous releases.

Example helmfile.yaml

helmfile.yaml:

repositories:
  - name: jenkinsci
    url: https://charts.jenkins.io
 
templates:
  jenkins: &jenkins
    name: jenkins{{`{{.Release.Labels.jenkinsInstance}}`}}
    chart: jenkinsci/jenkins
    version: 4.1.16
    labels:
      group: jenkins
      privateRegistry: 123456789.dkr.ecr.us-west-1.amazonaws.com
    namespace: jenkins{{`{{.Release.Labels.jenkinsInstance}}`}}
    values:
      - jenkins.yaml.gotmpl

releases:
  - labels:
      jenkinsInstance: "1"
      zone: us-west-1b
      branch: master
      jobsTags: ""
    <<: *jenkins

jenkins.yaml.gotmpl:

agent:
  image: '{{ .Release.Labels.privateRegistry }}/jenkins-agent'

Error message you've seen (if any)

in ./helmfile.yaml: failed to render values files "jenkins.yaml.gotmpl": failed to render [jenkins.yaml.gotmpl], because of template: stringTemplate:2:21: executing "stringTemplate" at <.Release.Labels.privateRegistry>: map has no entry for key "privateRegistry"

Steps to reproduce

helmfile -f ./helmfile.yaml -l name=jenkins1 template

Working Helmfile Version

0.146.0

Relevant discussion

No response

@yxxhero yxxhero added the help wanted Extra attention is needed label Oct 13, 2022
@yxxhero
Copy link
Member

yxxhero commented Oct 13, 2022

@sisrael-dn I will work on this.

@yxxhero
Copy link
Member

yxxhero commented Oct 13, 2022

see: #394, we upgrade the yaml library into v3. it allows key overrides.

@mumoshu what do you think? this should be an incompatible change, should we keep the new behavior, or fall back to v2?
I think this v3 upgrade is normal, and this override behavior should be in line with the current yaml logic.

@yxxhero
Copy link
Member

yxxhero commented Oct 13, 2022

@sisrael-dn

repositories:
  - name: jenkinsci
    url: https://charts.jenkins.io
 
templates:
  jenkins: &jenkins
    name: jenkins{{`{{.Release.Labels.jenkinsInstance}}`}}
    chart: jenkinsci/jenkins
    version: 4.1.16
    labels:
      group: jenkins
      privateRegistry: 123456789.dkr.ecr.us-west-1.amazonaws.com
    namespace: jenkins{{`{{.Release.Labels.jenkinsInstance}}`}}
    values:
      - jenkins.yaml.gotmpl

releases:
  - labels:
      group: jenkins
      privateRegistry: 123456789.dkr.ecr.us-west-1.amazonaws.com
      jenkinsInstance: "1"
      zone: us-west-1b
      branch: master
      jobsTags: ""
    <<: *jenkins

@yxxhero
Copy link
Member

yxxhero commented Oct 13, 2022

@sisrael-dn upgrade your helmfile.yaml as shown above. It will be work correctly.

@yxxhero
Copy link
Member

yxxhero commented Oct 13, 2022

https://yaml-online-parser.appspot.com/ we can test YAML on it. it will do the same thing as YAML v3.

@yxxhero
Copy link
Member

yxxhero commented Oct 14, 2022

@sisrael-dn ping

@yxxhero
Copy link
Member

yxxhero commented Oct 16, 2022

@mumoshu WDYT about this issue? Thanks very much.

@mumoshu
Copy link
Contributor

mumoshu commented Oct 18, 2022

@sisrael-dn @yxxhero If we were to allow Release.Labels as a template parameter for other fields like the release name and release namespace, how would one do the opposite? For instance, how can we enable folks who want to use Release.Labels as a parameter, while enabling others to use Release.Namespace or Release.Name as a template parameter to dynamically render the Release.Labels..?
We're basically supporting the latter pattern today I think. Fundamentally it's impossible to support both at the same time without parsing Go template expressions embedded in each release field...?

@tdaniely-dn
Copy link

@sisrael-dn @yxxhero If we were to allow Release.Labels as a template parameter for other fields like the release name and release namespace, how would one do the opposite? For instance, how can we enable folks who want to use Release.Labels as a parameter, while enabling others to use Release.Namespace or Release.Name as a template parameter to dynamically render the Release.Labels..? We're basically supporting the latter pattern today I think. Fundamentally it's impossible to support both at the same time without parsing Go template expressions embedded in each release field...?

Our error is in the values render, not in the template render, that works fine.

@sisrael-dn
Copy link
Author

@yxxhero thank you for your explanation & suggestion, I have tested it and it seems to work with latest helmfile.
However, I understand I now need to duplicate all labels for each release and can't trust they will be taken from template labels as before if missing.

@yxxhero
Copy link
Member

yxxhero commented Oct 19, 2022

This may be an incompatible change, but it conforms to the yaml specification. @sisrael-dn

@mumoshu
Copy link
Contributor

mumoshu commented Oct 20, 2022

@tdaniely-dn Thanks, gotcha. But it should be made available via this part of code

Labels: release.Labels,
Not yet sure what's happening there 🤔

@stale
Copy link

stale bot commented Nov 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 3, 2022
@yxxhero yxxhero removed the wontfix This will not be worked on label Nov 3, 2022
@stale
Copy link

stale bot commented Nov 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 17, 2022
@yxxhero yxxhero removed the wontfix This will not be worked on label Nov 17, 2022
@stale
Copy link

stale bot commented Dec 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 1, 2022
@yxxhero yxxhero removed the wontfix This will not be worked on label Dec 1, 2022
@yxxhero
Copy link
Member

yxxhero commented Dec 1, 2022

ping pong

@VladStarr
Copy link

Same here, since 0.147.0, parent template is unable to get labels values which are specified in child template.

templates:
  exchange: &exchange
    secrets:
      - secrets/redis.yaml
      - secrets/rabbitmq.yaml
      - secrets/{{`{{ .Release.Labels.app }}`}}.yaml
      - secrets/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml
      - secrets/{{`{{ .Release.Name }}`}}.yaml
    valuesTemplate:
      - ../common/values/{{`{{ .Release.Labels.app }}`}}.yaml.gotmpl
      - ../common/values/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Labels.app }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Name }}`}}.yaml.gotmpl
  private-exchange-svc: &private-exchange-svc
    <<: *exchange
    labels:
      app: private-exchange-svc
      
releases:
  - name: private-exchange-svc-test-0
    <<: *private-exchange-svc
    labels:
      exchange: test

in ./helmfile.yaml: in .helmfiles[2]: in pa/helmfile.yaml: failed executing release templates in "helmfile.yaml": failed executing templates in release "helmfile.yaml"."private-exchange-svc-test-0": failed executing template expressions in release "private-exchange-svc-test-0".values[8] = "[104 101 108 109 102 105 108 101 76 97 98 101 108 115 58 10 32 32 97 112 112 58 32 39 123 123 32 46 82 101 108 101 97 115 101 46 76 97 98 101 108 115 46 97 112 112 32 125 125 39 10]": template: stringTemplate:2:19: executing "stringTemplate" at <.Release.Labels.app>: map has no entry for key "app"

@voron
Copy link
Contributor

voron commented Dec 7, 2022

The whole idea of labels usage for us is a deep merge of labels keys&values between templates and releases , similar to the release values from valuesTemplate + values, the case @VladStarr is showed above. We use tens of releases with just a single difference - one label, while the release template is the same. And this release template is nested from another template to keep things "dry".

@stale
Copy link

stale bot commented Dec 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 21, 2022
@mumoshu
Copy link
Contributor

mumoshu commented Dec 21, 2022

@VladStarr @voron Thanks a lot for clarifying! Let met first say that your use case alone looks totally valid to me.

What's complicating the resolution here is that your use relied on a behavior of a specific yaml parser implementation(go-yaml v2) which does not conform to the yaml spec.

We should keep backward compatibility. But is treating a bug like a spec to keep the existing behavior the right thing to do...?

I'd be happy if we could still consider it a bug. Instead of reverting the parser to the prev version, can we add another feature to support your use case?

What if we add a new field, inherit or base or whatever that makes sense, to inherit fields from a template?

With it, I'd assume @VladStarr's previous example could be written like this:

templates:
  exchange: &exchange
    secrets:
      - secrets/redis.yaml
      - secrets/rabbitmq.yaml
      - secrets/{{`{{ .Release.Labels.app }}`}}.yaml
      - secrets/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml
      - secrets/{{`{{ .Release.Name }}`}}.yaml
    valuesTemplate:
      - ../common/values/{{`{{ .Release.Labels.app }}`}}.yaml.gotmpl
      - ../common/values/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Labels.app }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Labels.app }}`}}-{{`{{ .Release.Labels.exchange }}`}}.yaml.gotmpl
      - values/{{`{{ .Release.Name }}`}}.yaml.gotmpl
  private-exchange-svc: &private-exchange-svc
    <<: *exchange
    labels:
      app: private-exchange-svc
      
releases:
  - name: private-exchange-svc-test-0
    # THE ONLY CHANGE YOU NEED
    inherit: private-exchange-svc
    labels:
      exchange: test

@VladStarr's example needs a deep merge on labels only once, so as the use of inherit.

I presume @voron's example might look a bit more complex. Hey @voron, how many "labels" fields across the templates and the release needs to be deep merged in your configuration? For Vlad's it was two. If you need three or more, it it would look like this:

templates:
  exchange:
    labels:
      foo
  private-exchange-svc:
    # Let helmfile merge the whole release spec and the labels field
    inherit: exchange
    labels:
      app: private-exchange-svc
      
releases:
  - name: private-exchange-svc-test-0
    # Let helmfile merge the whole release spec and the labels field
    inherit: private-exchange-svc
    labels:
      exchange: test

@stale stale bot removed the wontfix This will not be worked on label Dec 21, 2022
mumoshu added a commit that referenced this issue Dec 27, 2022
This is a successor to #596. We need a smooth migration path from `gopkg.in/yaml.v2`, and this pull request moves it forward with `goccy/go-yaml` instead of `gopkg.in/yaml.v3`. Merging this unblocks users stuck in Helmfile v0.146.x or earlier due to #435, so that they can upgrade to 0.147.x or greater without updating their helmfile configs.

We previously tried to upgrade to `yaml.v3` (#394) in Helmfile v0.x, presuming it won't break anything. Apparently, it broke use-cases where you want to layer release's `values` field over three or more release templates and releases (#435).

We then tried to bring back `yaml.v2` for Helmfile v0.x and keep `yaml.v3` for the upcoming Helmfile v1. However, it failed due to incompatibility in the Unmarshaller interface between `yaml.v2` and `yaml.v3` (#596).

`goccy/go-yaml` is, from my observation, a well-maintained alternative to `yaml.v2`. One of its premises is that it enables us to swap the implementation from `gopkg.in/yaml.v2` to `goccy/go-yaml` just by replacing the import directive. It seems to use the same `Unmarshaller` interface as yaml.v2 too.

Once this PR gets merged, I'd like to follow-up with adding a new build-time variable and an envvar to set the proper default for the yaml parser Helmfile uses and the ability to switch the parser at runtime. All in all, the next Helmfile release, v0.150.0 will get reverted to use `gopkg.in/yaml.v2` by default which resolves #435.

New users who started using Helmfile since any of v0.148.0, v0.148.1, and v0.149.0 might be already relying on the new behavior, They might need to specify a new envvar to enable `goccy/go-yaml`.

Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Co-authored-by: yxxhero <aiopsclub@163.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu reopened this Dec 27, 2022
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 27, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
* feat: `inherit` field for release template inheritance

Ref #435 (comment)

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Fix wording

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Comment on releaseWithInheritedTemplate

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Update Release Template doc with the new `inherit` feature

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Fix a typo in code comment

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu
Copy link
Contributor

mumoshu commented Dec 28, 2022

@VladStarr @voron FYI, #606 has been merged. You can try building helmfile from our main branch to give inherit a shot today 🎉

mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu added a commit that referenced this issue Dec 28, 2022
Ref #435

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@dennybaa
Copy link

dennybaa commented Dec 29, 2022

@mumoshu @voron Hello great to see you all here in active discussion 👍 I would though like to clarify if we really dump labels pass-through or not. It's really cool to see a new feature such as inherit which tends to improve using helmfile templates. Though I do really depend on .Release data inside valuesTemplates (

I'd like to know if it will be still possible to use .Release.Labels and other .Release data inside valuesTemplate files during rendering. Since my usage is slightly different and I start using a lot more templating to achieve D.R.Y. Here's some examples:

  1. tenant-app.yaml.gotmpl (multiple tenant applications are driven by common values template). As can be seen all Release data is used.

    ## Common values for all tenant application deployments (dysnix/app v0.99.x)
    ##
    app:
      name: {{ index .Release.Labels "app" | default .Release.Name }}
    
    podLabels:
      app/logs_json: "true"
    
    image:
      registry: eu.gcr.io
      repository: {{ .Values.google.images | default .Values.google.project }}/{{ index .Release.Labels "app" | default .Release.Name }}
      pullPolicy: Always
      pullSecrets:
        - name: gcr-pull-default
    
  2. eval.yaml.gotmpl - evaluation helper which combines helmfile values context with some data (can be even ingested from a dynamic helmfile):

    {{/* Evaluated expressions */}}
    {{- $tenant := index .Release.Labels "tenant" -}}
    
    environment: {{ .Environment.Name }}
    tenant: {{ $tenant }}
    
    {{/* SecretManager paths */}}
    SMPath:
      infra: {{ printf "%s/%s-infra" .Values.google.project .Values.google.namespace }}
      namespace: {{ printf "%s/%s" .Values.google.project .Values.google.namespace }}
      tenant: {{ printf "%s/%s-%s" .Values.google.project .Values.google.namespace $tenant }}
    
  3. values.yaml.gotmpl which can combine dynamic data with the helmfile values environment values (forgot to post this)

    {{- $e := tpl (readFile "../eval.yaml.gotmpl") . | fromYaml -}}
    
    env:
      PUSHGATEWAY_URL: http://pushgateway-prometheus-pushgateway.monitoring:9091
      SENTRY_URL: https://aaa
      SENTRY_DEBUG: 1
      SENTRY_SAMPLE_RATE: 0.5
      NOTIONS_URL: http://notions:8080
      SCHEDULING_URL: http://scheduling:8080
      ENVIRONMENT: {{ $e.environment }}
      DB_ENCRYPT_PASSWORD: ref+gcpsecrets://{{ $e.SMPath.tenant }}-database#/encrypt_password
      DB_ENCRYPT_SALT: ref+gcpsecrets://{{ $e.SMPath.tenant }}-database#/encrypt_salt
      HMAC_SHA256_SECRET: ref+gcpsecrets://{{ $e.SMPath.infra }}-common-secrets#/workersTailApiToken
    

If it goes I'm afraid lots from the values "dryness" perspective goes missing too :(

I mean I didn't clearly get, imo it's bad if we loose either:

  1. evaluation inside the helmfile
    valuesTemplate:
      - ../common/values/{{`{{ .Release.Labels.app }}`}}.yaml.gotmpl
  1. evaluation inside the gotmpl values files render contexts.
  2. @voron

arrays-/maps-based field are deep-merged from all the upper levels up to the root template or up to mentioning inside except. Using the field inside a new except field switches this field to override/cleanup mode on the current level w/o merging from any upper levels

How do you see a deep merge of an array which is basically an "order-matters" data?

@dennybaa
Copy link

dennybaa commented Jan 3, 2023

Hello, sorry for buzz. Didn't read carefully enough( So this is about {{ .Release.Labels }} interpolation in template name and namespace as well as the new inherit feature which merges the labels, values and other parameters available in the template for a release. While labels can be successfully merged via inherit feature, would you please clarify what the behavior of the bellow is:

templates:
  exchange:
    values:
    - default.yaml
    labels:
      foo
  private-exchange-svc:
    inherit:
      template: exchange
    labels:
      app: private-exchange-svc
    values:
    - exchange.yaml
releases:
  - name: private-exchange-svc-test-0
    inherit:
      template: private-exchange-svc
    labels:
      exchange: test

I.e. how the array (values array) is supposed to be merged/overwritten/concatenated in this case, so what is the resulting values array for private-exchange-svc-test-0 release. Thanks.

@yxxhero
Copy link
Member

yxxhero commented Jan 4, 2023

@mumoshu we need more docs for inherit feature.

mumoshu added a commit that referenced this issue Jan 4, 2023
This should fix #435 for Helmfile v0.x releases since the next v0.150.0.
We introduce a new envvar to opt-in to the new YAML library, so that you can give it a shot before upgrading your Helmfile to v1. The same envvar can be used to opt-out of the new YAML library after you upgrade to Helmfile v1, giving you a more flexible migration story.

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
7 participants