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

Environment values not overriding #1168

Open
michaelajr opened this issue Apr 2, 2020 · 15 comments
Open

Environment values not overriding #1168

michaelajr opened this issue Apr 2, 2020 · 15 comments

Comments

@michaelajr
Copy link

michaelajr commented Apr 2, 2020

Something has broken in the last release around env values. I use to be able to do this:

environments:
  default:
    missingFileHandler: Error
    values:
    - values-file-1.yaml
    - values-file-2.yaml

Where values-file-2.yaml had some values that would override values in values-file-1.yaml. This is no longer working. It appears values-file-2.yaml is ignored (although I am not sure). It only has a single value override.

Also missingFileHandler: Error no longer works. It will not error on missing files.

Sorry if I missed a post about a breaking change. But I'm going to downgrade until this is resolved or I understand the issue.

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 2, 2020

@michaelajr Hey! I couldn't reproduce your issue with the following example:

$ tail *
==> helmfile.yaml <==
  default:
    missingFileHandler: Error
    values:
    - values.1.yaml
    - values.2.yaml

releases:
- name: example
  values:
  - {{ .Values | toYaml | nindent 4 }}

==> values.1.yaml <==
foo: FOO

==> values.2.yaml <==
bar: BAR
$ helmfile build
---
#  Source: helmfile.yaml

filepath: helmfile.yaml
helmBinary: helm
environments:
  default:
    values:
    - values.1.yaml
    - values.2.yaml
    missingFileHandler: Error
releases:
- name: example
  values:
  - bar: BAR
    foo: FOO
templates: {}

Could you clarify a bit more?

We did fix a bug that you couldn't override a value if it was nil as per #1153 and #1162.

Would it affect your use-case?

@michaelajr
Copy link
Author

michaelajr commented Apr 3, 2020

Hi. Thanks for the quick response. Here's our real world example.

  • We keep a list of charts to install in addons.yaml
  • Each cluster brings overrides - addons2.yaml in this example
$ tail -20 *
==> addons.yaml <==
addons:
  mychart:
    skip: true
    name: mychart
    namespace: kube-system
    chart: stable/mychart
    version: 1.0.0

==> addons2.yaml <==
addons:
  mychart:
    skip: false

==> helmfile.yaml <==
environments:
  default:
    missingFileHandler: Error
    values:
    - addons.yaml
    - addons2.yaml
releases:
{{- range .Values.addons }}
{{- if not .skip }}
- name: {{ .name }}
  namespace: {{ .namespace }}
  chart: {{ .chart }}
  version: {{ .version }}
{{- end }}
{{- end }}

In this example addons2.yaml is changing the skip to false. So it should show up in releases when I build - and prior to 0.106.2 it did.

$ helmfile build
---
#  Source: helmfile.yaml

filepath: helmfile.yaml
helmBinary: helm
environments:
  default:
    values:
    - addons.yaml
    - addons2.yaml
    missingFileHandler: Error
templates: {}
err: no releases found that matches specified selector() and environment(default), in any helmfile

Now, let's NOT skip the chart in addons.yaml.

==> addons.yaml <==
addons:
  mychart:
    skip: false
    name: mychart
    namespace: kube-system
    chart: stable/mychart
    version: 1.0.0

And build

$ helmfile build
---
#  Source: helmfile.yaml

filepath: helmfile.yaml
helmBinary: helm
environments:
  default:
    values:
    - addons.yaml
    - addons2.yaml
    missingFileHandler: Error
releases:
- chart: stable/mychart
  version: 1.0.0
  name: mychart
  namespace: kube-system

There it is. So addons2.yaml is not doing what it use to - overriding the skip value.

Also, if a file does not exist, I no longer get an error that stops the build.

$ tail -20 helmfile.yaml
environments:
  default:
    missingFileHandler: Error
    values:
    - does-not-exist.yaml
    - addons.yaml
    - addons2.yaml
releases:
{{- range .Values.addons }}
{{- if not .skip }}
- name: {{ .name }}
  namespace: {{ .namespace }}
  chart: {{ .chart }}
  version: {{ .version }}
{{- end }}
{{- end }}

Build.

$ helmfile build
in ./helmfile.yaml: error during helmfile.yaml.part.0 parsing: template: stringTemplate:9:17: executing "stringTemplate" at <.Values.addons>: map has no entry for key "addons

It looks like it fails internally but continues with no values , so it fails later. Pretty sure I use to get a missing file error immediately, but will confirm later in comments.

Btw, thanks for your work on this. Been following the repo, and you put in a lot of time. Great in depth comments - quick responses and turn arounds. So thank you. Really appreciate it.

M

@michaelajr
Copy link
Author

michaelajr commented Apr 3, 2020

Looks like other non-boolean values do override. Changing versions in this example, and it does work.

$ tail -20 *
==> addons.yaml <==
addons:
  mychart:
    skip: false
    name: mychart
    namespace: kube-system
    chart: stable/mychart
    version: 1.0.0

==> addons2.yaml <==
addons:
  mychart:
    version: 2.0.0

==> helmfile.yaml <==
environments:
  default:
    missingFileHandler: Error
    values:
    - addons.yaml
    - addons2.yaml
releases:
{{- range .Values.addons }}
{{- if not .skip }}
- name: {{ .name }}
  namespace: {{ .namespace }}
  chart: {{ .chart }}
  version: {{ .version }}
{{- end }}
{{- end }}

$ helmfile build
---
#  Source: helmfile.yaml

filepath: helmfile.yaml
helmBinary: helm
environments:
  default:
    values:
    - addons.yaml
    - addons2.yaml
    missingFileHandler: Error
releases:
- chart: stable/mychart
  version: 2.0.0
  name: mychart
  namespace: kube-system
templates: {}

@michaelajr
Copy link
Author

Also, can confirm missingFileHandler: Error in 0.104.0 behaves the same. I really thought it failed fast but I guess I was wrong. So disregard. Although it might be nice to make it fail fast - so if you think it should I can open a separate issue.

@michaelajr
Copy link
Author

Also here is confirmation that the boolean merge worked in 0.104.0.

$ brew switch helmfile 0.104.0
Cleaning /usr/local/Cellar/helmfile/0.104.0
Cleaning /usr/local/Cellar/helmfile/0.106.3
1 links created for /usr/local/Cellar/helmfile/0.104.0

$ tail -20 *
==> addons.yaml <==
addons:
  mychart:
    skip: true
    name: mychart
    namespace: kube-system
    chart: stable/mychart
    version: 1.0.0

==> addons2.yaml <==
addons:
  mychart:
    skip: false

==> helmfile.yaml <==
environments:
  default:
    missingFileHandler: Error
    values:
    - addons.yaml
    - addons2.yaml
releases:
{{- range .Values.addons }}
{{- if not .skip }}
- name: {{ .name }}
  namespace: {{ .namespace }}
  chart: {{ .chart }}
  version: {{ .version }}
{{- end }}
{{- end }}

$ helmfile build
---
#  Source: helmfile.yaml

filepath: helmfile.yaml
environments:
  default:
    values:
    - addons.yaml
    - addons2.yaml
    missingFileHandler: Error
releases:
- chart: stable/mychart
  version: 1.0.0
  name: mychart
  namespace: kube-system
templates: {}

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 3, 2020

Thanks a lot for your detailed report!

It does seem like a regression - So in nutshell it seems like an override with a zero values(like false for boolean) started to not work in 0.106.x, right?

Probably we need to revert a change or two regarding mergo, which we use under the hood for merging values.

@michaelajr
Copy link
Author

It does seem something in the mergo upgrade caused the behavior change. Could my use case be accomplished a different way? Is there a helmfile way to skip? I was just rolling my own.

mumoshu added a commit that referenced this issue Apr 4, 2020
This is a follow-up for #1169 and it also relates to #1168
mumoshu added a commit that referenced this issue Apr 4, 2020
This is a follow-up for #1169 and it also relates to #1168
mumoshu added a commit that referenced this issue Apr 4, 2020
This is a follow-up for #1169 and it also relates to #1168
@mumoshu
Copy link
Collaborator

mumoshu commented Apr 4, 2020

@michaelajr I managed to reproduce and fix this thanks to you and @elodani, in #1169.

Could you please confirm if it works for you now with Helmfile v1.106.4?

@mumoshu
Copy link
Collaborator

mumoshu commented Apr 4, 2020

Also - I've added a few tests to make this never happen again :) See #1170

@jalkjaer
Copy link
Contributor

jalkjaer commented Aug 29, 2020

The same problem exists with mergeOverwrite:
helmfile.yaml:

{{- $v1 := dict "bool" true  "int" 2 "str" "v1" "str2" "v1" -}}
{{- $v2 := dict "bool" false "int" 0 "str" "v2" "str2" "" -}}
{{- $mo1 := mergeOverwrite (dict) $v1 $v2 }}
{{- $mo2 := mergeOverwrite (dict) $v2 $v1 }}{{/* just for sanity checking the order of merge args */}}
releases:
  - name: mo1
    chart: mo1
    values: 
      - {{- $mo1 | toYaml | nindent 8}}
  - name: mo2
    chart: mo2
    values:
      - {{- $mo2 | toYaml | nindent 8 }}

build output (helmfile version v0.126.0)

---
#  Source: helmfile.yaml

filepath: helmfile.yaml
helmBinary: helm
releases:
- chart: mo1
  name: mo1
  values:
  - bool: true
    int: 2
    str: v2
    str2: v1
- chart: mo2
  name: mo2
  values:
  - bool: true
    int: 2
    str: v1
    str2: v1
templates: {}
missingFileHandler: ""

in mo1 only str gets overwritten and the "falsy" values are ignored
Have been looking for helmfile v1.106.4 - where is that?

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 29, 2020

Have been looking for helmfile v1.106.4 - where is that?

I meant v0.106.4

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 29, 2020

@jalkjaer FWIW it seems a sprig issue.

Sprig's mergeOverwrite seems to be missing mergo.WithOverwriteWithEmptyValue.

func mergeOverwrite(dst map[string]interface{}, srcs ...map[string]interface{}) interface{} {
	for _, src := range srcs {
		if err := mergo.MergeWithOverwrite(&dst, src); err != nil {
			// Swallow errors inside of a template.
			return ""
		}
	}
	return dst
}

MergeWithOverwrite is basically Merge(dst, src, mergo.WithOverride) where it MUST be , Merge(dst, src, mergo.WithOverride, mergo.WithOverwriteWithEmptyValue)` to enable override with falsey values.

@jalkjaer
Copy link
Contributor

jalkjaer commented Sep 2, 2020

Latest version (v3.1.0) of sprig does the expected. (see https://play.golang.org/p/r_IgI3Tqb1A )
If I bump sprig to v3.1.0 and explicitly add mergo v0.3.11 then mergeOverwrite works in Helmfile as documented in sprig

But those deps cause a test to fail

--- FAIL: TestVisitDesiredStatesWithReleasesFiltered_EnvironmentValueOverrides (0.00s)
panic: reflect: reflect.flag.mustBeAssignable using value obtained using unexported field [recovered]
	panic: reflect: reflect.flag.mustBeAssignable using value obtained using unexported field

goroutine 13 [running]:
testing.tRunner.func1.1(0x13567e0, 0xc0002a5ce0)
	/home/jalkjaer/sdk/go1.14.4/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc000457440)
	/home/jalkjaer/sdk/go1.14.4/src/testing/testing.go:943 +0x3f9
panic(0x13567e0, 0xc0002a5ce0)
	/home/jalkjaer/sdk/go1.14.4/src/runtime/panic.go:969 +0x166
reflect.flag.mustBeAssignableSlow(0x1b9)
	/home/jalkjaer/sdk/go1.14.4/src/reflect/value.go:244 +0x1b9
reflect.flag.mustBeAssignable(...)
	/home/jalkjaer/sdk/go1.14.4/src/reflect/value.go:234
reflect.Value.Set(0x1455e60, 0xc000010018, 0x1b9, 0x1455e60, 0xc000010018, 0x1b9)
	/home/jalkjaer/sdk/go1.14.4/src/reflect/value.go:1526 +0x3b
github.com/imdario/mergo.deepMerge(0x1455e60, 0xc000010018, 0x1b9, 0x1455e60, 0xc000010018, 0x1b9, 0xc0000a28c8, 0x2, 0xc000555dc0, 0x0, ...)
	/home/jalkjaer/git/roboll/helmfile/vendor/github.com/imdario/mergo/merge.go:99 +0x308c
github.com/imdario/mergo.deepMerge(0x15cdd60, 0xc00037db10, 0x1b6, 0x15cdd60, 0xc000487610, 0x1b6, 0xc0000a28c8, 0x1, 0xc000555dc0, 0x0, ...)
	/home/jalkjaer/git/roboll/helmfile/vendor/github.com/imdario/mergo/merge.go:246 +0x206e
github.com/imdario/mergo.deepMerge(0x15e31a0, 0xc00037d900, 0x199, 0x15e31a0, 0xc000487400, 0x199, 0xc0000a28c8, 0x0, 0xc000555dc0, 0x199, ...)
	/home/jalkjaer/git/roboll/helmfile/vendor/github.com/imdario/mergo/merge.go:93 +0x2f82
github.com/imdario/mergo.merge(0x15f1240, 0xc00037d900, 0x15f1240, 0xc000487400, 0xc0000a2aa8, 0x1, 0x1, 0xd, 0x201)
	/home/jalkjaer/git/roboll/helmfile/vendor/github.com/imdario/mergo/merge.go:361 +0x32a
github.com/imdario/mergo.Merge(...)
	/home/jalkjaer/git/roboll/helmfile/vendor/github.com/imdario/mergo/merge.go:291
github.com/roboll/helmfile/pkg/app.(*desiredStateLoader).renderAndLoad(0xc0002f3050, 0x0, 0xc00074c7a0, 0x15fb2f5, 0x1, 0xc0003a0f00, 0xd, 0xc0001c1800, 0x384, 0x400, ...)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/desired_state_file_loader.go:225 +0x58c
github.com/roboll/helmfile/pkg/app.(*desiredStateLoader).loadFileWithOverrides(0xc0002f3050, 0x0, 0xc00074c7a0, 0x15fb2f5, 0x1, 0x16074ea, 0xd, 0x1, 0x199, 0x203000, ...)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/desired_state_file_loader.go:117 +0x1ec
github.com/roboll/helmfile/pkg/app.(*desiredStateLoader).Load(0xc0002f3050, 0x16074ea, 0xd, 0x25e24e8, 0x0, 0x0, 0xc00074c3a0, 0x2, 0x2, 0x0, ...)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/desired_state_file_loader.go:65 +0x2f2
github.com/roboll/helmfile/pkg/app.(*App).loadDesiredStateFromYaml(0xc0004578c0, 0x16074ea, 0xd, 0xc0000a36b8, 0x1, 0x1, 0x0, 0x0, 0x0)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:575 +0x26d
github.com/roboll/helmfile/pkg/app.(*App).visitStates.func1(0x16074ea, 0xd, 0xc0003a09b0, 0x8, 0x0, 0x0)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:627 +0x176
github.com/roboll/helmfile/pkg/app.(*App).visitStateFiles.func1(0x0, 0x7fae4c245108)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:542 +0x9d
github.com/roboll/helmfile/pkg/app.(*App).within(0xc0004578c0, 0x15fb2f5, 0x1, 0xc000017ad0, 0xc00007ba90, 0x2)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:484 +0x602
github.com/roboll/helmfile/pkg/app.(*App).visitStateFiles(0xc0004578c0, 0x16074ea, 0xd, 0x25e24e8, 0x0, 0x0, 0xc00074c1c0, 0x2, 0x2, 0x0, ...)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:536 +0x2f2
github.com/roboll/helmfile/pkg/app.(*App).visitStates(0xc0004578c0, 0x16074ea, 0xd, 0x25e24e8, 0x0, 0x0, 0xc00074c1c0, 0x2, 0x2, 0x0, ...)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:620 +0x10b
github.com/roboll/helmfile/pkg/app.(*App).visitStatesWithSelectorsAndRemoteSupport(0xc0004578c0, 0x16074ea, 0xd, 0xc00007bdf0, 0xc00007be70, 0x1, 0x1, 0xc00007be01, 0xc0003a09a0)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:875 +0x4c1
github.com/roboll/helmfile/pkg/app.(*App).ForEachState(0xc0004578c0, 0xc00007bec8, 0xc00007be70, 0x1, 0x1, 0xc0001c0800, 0x384)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app.go:756 +0xb2
github.com/roboll/helmfile/pkg/app.TestVisitDesiredStatesWithReleasesFiltered_StateValueOverrides.func1(0xc000457440)
	/home/jalkjaer/git/roboll/helmfile/pkg/app/app_test.go:1155 +0x703
testing.tRunner(0xc000457440, 0xc00074e4c0)
	/home/jalkjaer/sdk/go1.14.4/src/testing/testing.go:991 +0xdc
created by testing.(*T).Run
	/home/jalkjaer/sdk/go1.14.4/src/testing/testing.go:1042 +0x357

That test is rather daunting (for a go / Helmfile noob like me) so this is as far I was able to get.
If you want, I can create a PR

@mumoshu
Copy link
Collaborator

mumoshu commented Sep 3, 2020

@jalkjaer Thanks for trying. Would u mind submitting the PR so that I can review and see what's going on?

@jalkjaer
Copy link
Contributor

jalkjaer commented Sep 3, 2020

PR #1456 created

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

No branches or pull requests

3 participants