Nested null values don't remove keys as expected

[aeijdenberg]

#2648 allows for deletion of keys by setting null values in a values.yaml file. However it doesn't work for nested values, e.g.:

web:
  livenessProbe:
    httpGet: null
    exec:
      command:
      - curl
      - -f
      - http://localhost:8080/api/v1/info

Will not remove web.livenessProbe.httpGet from the original values, rather it just overrides the value with null and prints the warning:

2019/01/18 11:30:07 Warning: Merging destination map for chart 'concourse'. Cannot overwrite table item 'httpGet', with non table value: map[path:/api/v1/info port:atc]

Ironically, above is a small variant on the example given in the docs, which I'm pretty sure does not actually do what it claims to: https://docs.helm.sh/chart_template_guide/#deleting-a-default-key

I suspect that since the template rendering doesn't seem to differentiate much between a null value and a value not being specified, that this warnings are being ignored and not having much effect.

Output of helm version:

Client: &version.Version{SemVer:"v2.12.1", GitCommit:"02a47c7249b1fc6d8fd3b94e6b4babf9d818144e", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.12.1", GitCommit:"02a47c7249b1fc6d8fd3b94e6b4babf9d818144e", GitTreeState:"clean"}

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.1", GitCommit:"eec55b9ba98609a46fee712359c7b5b365bdd920", GitTreeState:"clean", BuildDate:"2018-12-13T10:39:04Z", GoVersion:"go1.11.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"11+", GitVersion:"v1.11.5-eks-6bad6d", GitCommit:"6bad6d9c768dc0864dab48a11653aa53b5a47043", GitTreeState:"clean", BuildDate:"2018-12-06T23:13:14Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.):

EKS

[bacongobbler]

@scottrigby, do you by any chance want to take a gander at this one?

[bacongobbler]

Marking as question/support until it's been confirmed that this is indeed a bug according to the original author.

[cprivitere]

I'm seeing this as well on 2.12.3. I'm seeing while trying to set the value to null via both -f and --set. Either way it just sets it to "null" as a string (without the quotes) rather than removing the default value.
My situation:

> helm get values grafana
admin:
  existingSecret: ""
chownDataImage:
  pullPolicy: null
  repository: null
  tag: null
<...>

> helm upgrade grafana stable/grafana --tls --reuse-values --set chownDataImage.pullPolicy=null
<output indicating it works>

> helm get values grafana
admin:
  existingSecret: ""
chownDataImage:
  pullPolicy: null
  repository: null
  tag: null
<...>

Use case is to reset these values to use the defaults from the template instead of the defaults now stuck in helm.

[cdenneen]

stable/filebeat

With null or nil I'm seeing the same thing... the key isn't removed... it's overwritten as null/nil but not removed and when you are replacing it with different key it causes problem:

---values.yml in the Chart
output.file:
  path: /var/log/foo.log

---my overrides
output.elasticsearch:
   hosts:
      - 'http://localhost:9200'
output.file: null

or

---my overrides
output:
  elasticsearch:
     host:
      - 'http://localhost:9200'
  file: null

or

---my overrides
output:
  elasticsearch:
     hosts:
      - 'http://localhost:9200'
output.file: null

» k get secret filebeat -o jsonpath="{.data.filebeat\.yml}" | base64 -D
    filebeat.config:
      modules:
        path: ${path.config}/modules.d/*.yml
        reload.enabled: false
      prospectors:
        path: ${path.config}/prospectors.d/*.yml
        reload.enabled: false
    filebeat.prospectors:
    - enabled: true
      fields:
        apenv: dev
        app: kubernetes
        log_category: kubernetes
      fields_under_root: true
      paths:
      - /var/log/*.log
      - /var/log/messages
      - /var/log/syslog
      type: log
    - containers.ids:
      - '*'
      fields:
        apenv: dev
        app: kubernetes
        log_category: kubernetes
      fields_under_root: true
      processors:
      - add_kubernetes_metadata:
          in_cluster: true
      - drop_event:
          when:
            equals:
              kubernetes.container.name: filebeat
      type: docker
    http.enabled: true
    http.port: 5066
    output:
      elasticsearch:
        hosts:
        - http://localhost9200
    output.file: null
    processors:
    - add_cloud_metadata: null

pod errors as:

Exiting: error unpacking config data: more than one namespace configured accessing 'output' (source:'filebeat.yml')

[PosicubeBeege]

I'm also encountering this exact issue with the filebeat chart.

@cdenneen You can get around file output specifically with config.output.file.enabled=false.

I'm encountering an issue where I want to use the new inputs key for filebeat but cannot remove the prospectors key.

Existing values:

config:
  filebeat.prospectors:
    - type: log
      enabled: true
      paths:
        - /var/log/*.log
        - /var/log/messages
        - /var/log/syslog

My override: --set config.filebeat.prospectors=null

Result: (config is used to set a Secret value)

filebeat:
  prospectors: null

[pjanuario]

I also encountered this problem with stable/kibana and stable/filebeat the keys will default to the chart values even when !!null is specified.

[cdenneen]

@aeijdenberg believe your PR just requires labels updated to be reviewed

[scottrigby]

Hey @bacongobbler sorry I missed your question in #5184 (comment). I can verify it is a bug. In #2648 I really should have added a test that matched the documented example. I had meant to get back to this but #5185 looks promising! 👏 will respond over there

[wxdao]

Hi @scottrigby . May I know which release will include #5185's fix? I just encountered this problem when trying to remove resources definitions on istio chart.

[cc-stjm]

This is supposed to be in 2.14.2 - but I can't get it to work.

I'm following a very similar example to that at the top of the issue - the only difference I can see is that I'm trying to delete a value set in a subchart (in my case, logstash).

I have tried:

• setting logstash.livenessProbe.httpGet to null, ~ and {} in the main chart's values.yaml, or on the command line. In both cases, "helm template" shows that the original value is still there (i.e. I'm still getting httpGet set) - and this is consistent with what I see for "helm install"
• setting logstash.livenessProbe.httpGet to "nil" - in this case the value does get overwritten, but to the string "nil" which isn't a valid value to use here, so kubernetes rejects the template.

Have I misunderstood and this didn't make it into 2.14.2 - or is there still an issue?

(Chart demonstrating this: demo.zip)

[bacongobbler]

The patch was cherry-picked into 2.14.2 according to the release notes so it should be available.

[cc-stjm]

In which case, I'm not sure this bug is fixed - should it be re-opened or a new issue created? Can someone else have a look at the chart I attached to the above and see if they see different?

[aeijdenberg]

@cc-stjm @bacongobbler - I am able to reproduce the issue, and I think this test demonstrates it:

func TestSubchartCoaleseWithNullValue(t *testing.T) {
	v, err := CoalesceValues(&chart.Chart{
		Metadata: &chart.Metadata{Name: "demo"},
		Dependencies: []*chart.Chart{
			{
				Metadata: &chart.Metadata{Name: "logstash"},
				Values: &chart.Config{
					Raw: `livenessProbe: {httpGet: {path: "/", port: monitor}}`,
				},
			},
		},
		Values: &chart.Config{
			Raw: `logstash: {livenessProbe: {httpGet: null, exec: "/bin/true"}}`,
		},
	}, &chart.Config{})
	if err != nil {
		t.Errorf("Failed with %s", err)
	}
	result := v.AsMap()
	expected := map[string]interface{}{
		"logstash": map[string]interface{}{
			"global": map[string]interface{}{},
			"livenessProbe": map[string]interface{}{
				"exec": "/bin/true",
			},
		},
	}
	if !reflect.DeepEqual(result, expected) {
		t.Errorf("got %+v, expected %+v", result, expected)
	}
}

The problem, so far as I can tell, is that CoalesceValues() results in more than one call to the underlying function that coaleces the values:

helm/pkg/chartutil/values.go

Lines 164 to 180 in e04fa72

	func CoalesceValues(chrt *chart.Chart, vals *chart.Config) (Values, error) {
	cvals := Values{}
	// Parse values if not nil. We merge these at the top level because
	// the passed-in values are in the same namespace as the parent chart.
	if vals != nil {
	evals, err := ReadValues([]byte(vals.Raw))
	if err != nil {
	return cvals, err
	}
	cvals, err = coalesce(chrt, evals)
	if err != nil {
	return cvals, err
	}
	}
	return coalesceDeps(chrt, cvals)
	}

ie line 173 above is called with the httpGet set to null, but the value it returns has deleted that key from the map (as intended). But then that output is passed a 2nd time as input to the 2nd set of coalescing (line 179), and then since key no longer exists, it defaults to the value in the chart.

Unfortunately I'm unlikely to have time available in the near future to go any further - I've changed roles and am not currently using Helm, and the answer on how to solve isn't obvious to me. Hopefully the above is helpful in resolving.

[sgillespie]

In fact, I just upgraded 2.14.0 => 2.14.2. Not only does the nulled key still exist, but it also contains the previous value. The previous behavior just made it null, so I believe this has actually regressed.

[bacongobbler]

@aeijdenberg can you please look into @sgillespie's inquiry? If there's a regression, then it's probably safer to back that PR out unless you can determine a fix. If you're unable to help out then it's probably safer to revert your commit and go back to square 1. Let me know how you'd like to proceed.

[aeijdenberg]

@bacongobbler , while I haven't explictly tested 2.14.0, what @sgillespie matches the behaviour I referred to in #5184 (comment). Sadly I think it's a case of the unit tests passing, but the end to end failing - as the individual components are being used in series, which makes removing a key in an earlier stage makes it have no effect in later stages (and is why the original values are coming through).

I agree it's a regression, though backing it out is also a regression for anyone relying on the new behaviour.

I've had a quick go at a relatively minor patch which I think will reduce the impact (and fixes the test I added above) in #6146 - I'm sorry we got this wrong the in last attempt.

[bacongobbler]

This might've been fixed in #6080. That PR fixes the case where coalesceDeps is called twice. Can someone confirm with master?

If so, can #6146 be closed, or is it attempting to fix a separate issue? I'm trying to grasp my head around the problem space here but it's unclear what these PRs are attempting to solve. Sorry.

[sgillespie]

This does indeed seem to fix the issue. A quick naive test:

Subchart values:

prop:
  nested:
    val: true

Parent chart values:

sub:
  prop:
    nested: null

Output as expected is {}

[arnaud-deprez]

Hi,

I'm using helm 2.15.0 and I've still met this issue.

Given a subchart with values:

securityContext:
  runAsUser: 65534
  fsGroup: 65534

Where the values are used with toYaml

And in a parent chart values:

sub:
  securityContext:
  runAsUser: null

Then the actual output is:

securityContext:
  runAsUser: 65534
  fsGroup: 65534

While it should have been:

securityContext:
  fsGroup: 65534

[MatteoInfi]

still bugged in 3.5.4

[armandleopold]

Still exist in

HELM_VERSION="v3.6.3"

Can someone reopen the issue and fix it please ?

[kineta-Jiang]

Still exist in

HELM_VERSION="v3.7.1"
Can someone reopen the issue and fix it please ?

[sebastianblunt]

The reopened issue is #9136

[scalalang2]

Still exist in v3.7.2

[hosswald]

Still exists in v3.9.2

[kopylash]

Still exists in 3.11.1
Reproducible only with subcharts

[korniltsev]

still exists in 3.12.3

[korniltsev]

repro:
pyroscope-folder.zip
fsGroup and runAsUser null values are not removed

helm upgrade release-1 pyroscope-folder  --dry-run  --install > run1   
helm upgrade release-1 pyroscope-folder --values pyroscope-folder/values.yaml --dry-run  --install > run2 

diff -u run1 run2
--- run1        2023-09-12 18:26:31.238211712 +0700
+++ run2        2023-09-12 18:27:28.071648302 +0700
@@ -1,6 +1,6 @@
 Release "release-1" does not exist. Installing it now.
 NAME: release-1
-LAST DEPLOYED: Tue Sep 12 18:26:26 2023
+LAST DEPLOYED: Tue Sep 12 18:26:51 2023
 NAMESPACE: default
 STATUS: pending-install
 REVISION: 1
@@ -194,9 +194,7 @@
     spec:
       serviceAccountName: release-1-pyroscope
       securityContext:
-        fsGroup: 10001
         runAsNonRoot: true
-        runAsUser: 10001
       containers:
         - name: "pyroscope"
           securityContext:

[ccmcbeck]

still exists in 3.12.3

While I was unable to override a default subcharted value with foo: nil, I was able to override with foo: "".

[bauerjs1]

Still having this issue in Helm v3.13.2. Unfortunately I cannot use @ccmcbeck's workaround, because the particular value is not fenced with an if in the template and the value must be of type integer.

Overwriting the default value with null/nil (which does not work atm) is the only option I see here.

[rodrigoechaide]

I have created a new issue because this problem persists in the latest version of Kubernetes.