FYI this issue's impact is quite wide on the post renderers code path. In Flux, this code path runs for 100% of the HelmReleases, so we forked Helm to ship a patch today.

We've identified an issue with this fix when it comes to multi-line string values in the YAML template, see fluxcd/helm-controller#1448 for an example.

The root cause is the combined behavior of kio.ParseAll + kio.StringAll. This behavior is probably present in all the helm-controller v1.5 versions (all patches), also in all Helm v4 versions. So this issue is unrelated to this PR.

I don't think this is related to the parsing behaviour. Rather, this is introduced directly by the use of SplitManifests. The slice returned by it only contains values without trailing whitespaces/newlines. The root cause appears to be judicious use of strings.TrimSpace in the implementation of SplitManifests.

So any user of SplitManifests would be affected.

@marvin-roesch I was just able to reproduce this in Helm CLI 4.0.0, but not in Helm CLI 3.20.0 using helm template --post-renderer:

helm-3.20.0 template test-chart . --post-renderer /home/matheus/.local/share/helm/plugins/label-injector/inject-labels.sh
---
# Source: demo-chart/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  labels:
    post-rendered: "true"
  name: my-config
data:
  value-a: |
    line1
    line2
  dummy-string: |
    line1
    line2

helm-4.0.0 template test-chart . --post-renderer label-injector
---
# Source: demo-chart/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  labels:
    post-rendered: "true"
  name: my-config
data:
  value-a: |
    line1
    line2
  dummy-string: |-
    line1
    line2

Ah, I think there indeed are two separate issues at play here. I was not able to reproduce your case with Helm 4 on the templates that exhibit the issue for us. Do your template files have a final newline? Ours do and Helm 4.1.3 renders it correctly (with post-renderer), but if I remove the final newline, I can see the same difference in behaviour that you do.

Arguably the new behaviour is more correct since it correctly renders the YAML from the template without trailing newline. Helm 3 would insert additional newlines where they weren't supposed to be. I'll raise a separate issue about this regression regardless, but this PR introduces additional breakage for files that do have a trailing newline. And that is due to the behaviour of SplitManifests.

Sorry if I caused confusion, my editor will automatically use a trailing newline by default, so this never showed in my testing. I do think this PR still needs adjustment, however.

I was not able to reproduce your case with Helm 4 on the templates that exhibit the issue for us. Do your template files have a final newline? Ours do and Helm 4.1.3 renders it correctly (with post-renderer), but if I remove the final newline, I can see the same difference in behaviour that you do.

If the configmap.yaml ends in a newline, Helm 4.0.0 and Helm 3.20.0 show the same behavior. If there's no newline at the end, only Helm 4.0.0 shows |-. This behavior comes from kio trying to guess what the line ending should be due to lost information (it loses the information of | versus |- when parsing).

Arguably the new behaviour is more correct since it correctly renders the YAML from the template without trailing newline.

I'm not sure, it looks like kio's intention is to preserve syntax information but it loses it and then tries to guess what should it be.

I do think this PR still needs adjustment, however.

What would you suggest for this PR? My only intention here is to align the post-renderer code path with non-post-renderer code path, nothing else. The fact that Helm 4 introduced kio in the post-renderer code path is unrelated.

By the way, I'm not saying Helm 4 should not fix this, just saying that it should be a separate PR.

I've separately raised #31948 to address this difference in behaviour. I personally think Helm 4 with a post-renderer is more accurate here as any YAML parser would agree with this interpretation of the block literal in the absence of a trailing newline. | indicates that newlines should be preserved, not that a trailing newline ought to be inserted.

What would you suggest for this PR?

I think it makes sense to adjust SplitManifests here because AFAICT that is the main difference between this version and the old fix attempt for document separators. I'm not sure what the correct implementation would look like, but it should preserve trailing newlines at any rate.

This PR fixes critical bugs in Helm post render that breaks most charts out there. I would not delay a patch release with this fix due to Helm v4 preserving final newlines, this newline issue originates in v4.0.0 and is not a related to this PR IMO.

@stefanprodan, unfortunately due to the interplay with #31948 this fix (from the Helm fork shipped with the Flux Helm controller) has also caused issues for us already and could potentially cause way more charts to be broken due to trailing newlines being eliminated.

As for how to address the issue in this PR: The simplest would be to change the use of strings.TrimSpace in releaseutils.SplitManifests to strings.TrimLeft(..., unicode.IsSpace). That would only trim leading whitespace but leave trailing whitespace such as newlines alone.

The regex used by SplitManifests is still problematic since it would gobble up trailing newlines of all but the final document in a multi-document file/template, but I'd argue that the vast majority of Helm templates out there contain only a single document anyway. The other cases would have to be covered by a resolution of #31948.

As for how to address the issue in this PR: The simplest would be to change the use of strings.TrimSpace in releaseutils.SplitManifests to strings.TrimLeft(..., unicode.IsSpace). That would only trim leading whitespace but leave trailing whitespace such as newlines alone.

This would not fix the example I posted above, where there's no newline at the end of configmap.yaml. There's no empty line in either ends, not at the beginning, not at the end. So both TrimSpace and TrimLeft would not change the text bytes and Helm 4's behavior still differs from Helm 3's.

Hmm but I guess | and |- makes no difference if there's no newline at the end?

This would not fix the example I posted above, where there's no newline at the end of configmap.yaml.
Yes, I am aware of this. But it would avoid further breakage for templates that do have a trailing newline, like we were facing after upgrading to the latest Helm Controller.

A proper and full resolution for this situation would need to be addressed in #31948. Right now I am only concerned with not breaking things further with this PR, however.

I've locally played with the changes in this PR and using TrimLeftFunc instead avoids this additional breakage. See the following modified test case from the PR:

		{
			name: "mixed glued and proper separators",
			files: map[string]string{
				"templates/mixed.yaml": `
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm1
data:
  multi-line-1: |
    test
    test1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm2
data:
  multi-line-2: |
    test
    test1
---apiVersion: v1
kind: ConfigMap
metadata:
  name: cm3
data:
  multi-line-3: |
    test
    test1
`,
			},
			expected: `apiVersion: v1
kind: ConfigMap
metadata:
  name: cm1
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-1: |-
    test
    test1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm2
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-2: |-
    test
    test1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm3
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-3: |
    test
    test1
`,
		},

The cm1 and cm2 ConfigMaps still have the erroneous |- instead of | like in the input, which is due to the splitting regex eating any whitespace before. But crucially, cm3 is correctly having its trailing whitespace preserved. This would be the same for single-document files.

And now that I think about it, the changes from this PR would further meddle with currently working documents that are properly formatted. Before these changes, the above test case (if the separator before cm3 is properly on its own line) would have produced the following output:

apiVersion: v1
kind: ConfigMap
metadata:
  name: cm1
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-1: |
    test
    test1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm2
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-2: |
    test
    test1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm3
  annotations:
    postrenderer.helm.sh/postrender-filename: 'templates/mixed.yaml'
data:
  multi-line-3: |
    test
    test1

That is everything would have had its whitespace correctly preserved.

One resolution that would also address #31948 in the sense of restoring Helm 3 behaviour is the following: Trim all whitespace in SplitManifests (i.e. keep that as is) but parse each of the documents but then append a newline to each split document before passing it into kio.ParseAll. That would also "fix" files that originally did not have a trailing newline. Like I said earlier, though, I think strictly speaking Helm 4's behaviour with a post-renderer is more "correct" here, so I'm not sure that's a desirable solution.

@marvin-roesch I opened #31952 to address your concerns (and hence fix #31948) using this PR as base. I asked the maintainers to review and merge this one first, as is.

I got to the bottom of how |, |- and |+ work in YAML and added test cases for all scenarios. I think your suggestion of just replacing TrimSpace with TrimLeftFunc fixes everything. Please take a look in the test cases added in the PR.

Thanks @matheuscscp! I was holding off on submitting something myself since I wanted to wait for Helm maintainers to chime in on the issue itself, but having a PR to discuss on is also good.

@banjoh @TerryHowe @matheuscscp can we get this backported to v4.1? Or is that minor EOL already?