Support for multiple yaml documents in stdin/file

[jkroepke]

What this PR does / why we need it:

Fixes #10866

Special notes for your reviewer:

Tested stdin yaml:

---
a:
  b: c
  c: d
  e: f
---
e: value
a:
  b: f
  e:
  
test: >-
  hello
  ---
  world
---
a:
  c: 4

Before (main branch):

% cat values.yaml | ./bin/helm template --repo https://jkroepke.github.io/helm-charts/ values -f - 
---
# Source: values/templates/values.yaml
a:
  b: c
  c: d
  e: f

After (this PR):

% cat values.yaml | ./bin/helm template --repo https://jkroepke.github.io/helm-charts/ values -f - 
---
# Source: values/templates/values.yaml
a:
  b: f
  c: 4
  e: null
e: value
test: hello --- world

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[joejulian]

Can I suggest a different approach to Unmarshalling multi-document yaml? What about using this example? https://go.dev/play/p/MZNwxdUzxPo

[joejulian]

I've tried the method you're suggesting, in the past, and something caused it to break mid-file. I can't remember what it was though.

[jkroepke]

What about using this example?

It uses a different golang yaml library (sigs.k8s.io/yaml vs goyaml "github.com/go-yaml/yaml") and uses the v1?. No idea, if this is wanted

Kubernetes uses the same separater ( https://github.com/kubernetes/kubernetes/blob/a750d8054a6cb3167f495829ce3e77ab0ccca48e/staging/src/k8s.io/apimachinery/pkg/util/yaml/decoder.go#L199). I got this from here: kubernetes-sigs/yaml#37 (comment)

[jkroepke]

Based on kubernetes-sigs/yaml#37 (comment), I push a new version here.

I coped some code from here:

helm/pkg/lint/rules/template.go

Line 131 in 49819b4

decoder := yaml.NewYAMLOrJSONDecoder(strings.NewReader(renderedContent), 4096)

I extract the relevant merge code to a new function and add unit tests.

[joejulian]

This looks great, to me!

I am not a core maintainer, though. This will still need their approval.

[jkroepke]

@thomastaylor312 @adamreese What did you think?

[jkroepke]

@joejulian How can I help here? Looks stale here.

[joejulian]

@jkroepke I've been wondering that, too. Maybe the helm mailing list or, if you can attend, the helm developer call?

[jkroepke]

@joejulian thanks. https://lists.cncf.io/g/cncf-helm/message/450

[jkroepke]

@mattfarina What did you think?

[yxxhero]

@jkroepke I think you shuld write a HIP for this.

[jkroepke]

@yxxhero helm/community#253 - lets go.

[jkroepke]

Any news here @yxxhero ?

[yxxhero]

@jkroepke I think the code is good.

[yxxhero]

/lgtm

[yxxhero]

@hickeyma @marckhouzam @scottrigby Can you have a review on this. Thanks very much.

[iverberk]

@hickeyma @marckhouzam @scottrigby If you have some time, can you please review and (if ok) merge this addition? It unlocks a nice workflow to pass secrets securely to Helm (no intermediate disk storage needed).

[iverberk]

Friendly reminder to review this PR. It's fine (but slightly disappointing) ff there is no time or urgency to review this PR but it would be nice to get some kind of an update.

[yxxhero]

@iverberk I added this to this 3.10.0

[iverberk]

Wonderful @yxxhero! Thanks for the update and putting this on a milestone, looking forward to start using it.

[hickeyma]

@jkroepke There is feedback in the HIP (helm/community#253 (review)) from @bacongobbler. Do you mind addressing the comments?

[jkroepke]

Hi @hickeyma,

I read the feedback and there are currently no plan to more forward on the HIP. This PR add 30 lines of code and remove 9 lines of code excluding code for tests. From my point of view, a HIP feels too over engineered for this kind of change. I'm not going to document the existing values merge logic in detail or discuss about security implications which are not in scope of the helm project.

At least, the HIP feedback does not criticism the current implementation draft. I'm not sure, if there is an alternative solution to implement this feature from user perspective.

From code perspective, there are 2 approvals from 2 triage maintainers. All questions are resolved on this PR expect the lastest one. Unit tests are included. I would really like to move forward on the PR here.

[hickeyma]

@jkroepke If this feature requires a HIP then the HIP needs to be agreed upon before the feature code can be merged as the feature implementation needs to follow the design.

@bacongobbler reviewed the HIP and provided feedback and it is good practice to address these comments first.

[jkroepke]

I saw the comments, and in some points I do not agree with him. I hope someone else from the community has enough motivation for this process. It's a pain to go through the comments and wait 2 months again for the next feedback loop.

Can you explain, why a HIP is strictly need?

[iverberk]

I agree with @jkroepke that this process seems very heavy-handed for the proposed change. It's basically achieving parity with something that can already be done on the CLI (providing multiple values files). It's also reusing a lot of the existing code. Of course, it's your prerogative to mandate following the HIP process but I think no one will be motivated enough to follow through, especially with the long feedback cycles. Maybe that's also indicative that this is a 'niche' change that doesn't affect a lot of people. That being said, I think the use-case is very valid and that this should be part of core Helm.

@jkroepke I've decided to go ahead and just merge the values files myself before presenting them to Helm. I've taken the mergeMaps function and applied it to the values files that I have loaded, after that it's just a matter of marshalling to bytes and feeding it via stdin. Maybe you can take a similar approach to get what you want (secure passing of values via stdin). Thanks for your efforts on this so far.

[jkroepke]

I'm also still confused, why other features does not require a HIP, while this feature does.

#10693

I may not see the logic behind this.

[joejulian]

Just to be clear, triage folks are not code maintainers. We triage issues. Our feedback on PRs has exactly as much weight as anybody else in the community.