Error instead of silently truncating split set values

[northbymidwest]

Rollback Plan

If a change needs to be reverted, we will publish an updated version of the library.

Changes to Security Controls

Yes, two, both narrowing what reaches user-visible output:

• The parse-failure diagnostics no longer include the failing value. They previously interpolated it verbatim (Failed parsing key %q with value %s: %s), so a set_sensitive or set_wo value was printed in plan output whenever parsing it failed. The underlying parser error is withheld for those entries too, since it quotes the fragment it choked on.
• The set_sensitive loop logged its whole element at debug level; %v on that model renders the value. It now logs the key name alone.

Non-sensitive entries keep the underlying parser error, which is the more useful diagnostic where nothing is at stake.

Description

set, set_sensitive and set_list values are parsed by strvals, which treats , as a separator between assignments. A value containing an unescaped comma is therefore split: the key keeps only the text before it, and the remainder is applied as further, unrelated keys.

There are two outcomes, and only one is visible:

• When the remainder contains no =, strvals returns key "…" has no value and the apply fails. This is how the behaviour is usually noticed, and is the subject of most existing reports.
• When the remainder does contain an = — a PEM bundle, a base64 blob, a comment header — it parses cleanly. No error is returned. The release is applied with a truncated value plus some junk keys, and helm exits 0. Nothing in the plan or the apply output indicates that anything was dropped.

The second case is what this fixes. A CA bundle passed through set_sensitive reached the chart as a single certificate, because one of its comment headers contained a comma and the base64 payload after it happened to contain =.

What changed

A single set entry is expected to produce exactly one leaf, and a set_list entry as many elements as it was given. Both are now checked against a probe parse before the value is merged into the release config, and a mismatch is reported with the escape that fixes it:

Error: Value would be truncated

The value for "secrets.ca" contains an unescaped ",", which helm's value parser
treats as a separator between assignments. Only the text before it would reach
the chart, and the remainder would be applied as unrelated keys.

Escape it as "\," to pass it through unchanged, or supply the value through the
`values` attribute instead, which is parsed as YAML and needs no escaping.

Brace list syntax ({a,b}) still parses to a single leaf, so passing a list through set keeps working, as does an already-escaped x\,y. An empty set_list is skipped, since {} parses as one empty element and that is pre-existing behaviour rather than a split.

The diagnostics deliberately name only the key, never the value or elements, for the reason given under Changes to Security Controls.

Compatibility

This is breaking for a configuration that relies on the split — value = "1,b=2" under name = "a" to set two keys from one entry. Such a configuration does work today, so this is a real break rather than a no-op, but it is not a documented behaviour: the docs describe name as the full name of the variable and value as its value, and instruct that , be escaped. Nothing in the repository's tests or examples relies on it. The multi-assignment form is also never written by the user — name and value are supplied separately and the provider joins them — so a second assignment can only arise from a comma inside a value, which is the case this fixes.

Checked against the patterns that are documented, all still accepted: escaped dots in name, brace list syntax ({a,b}), and an already-escaped x\,y.

Known gap

A brace-wrapped value containing commas is still parsed as a list rather than a string — value = "{\"timeout\": \"30s\", \"x\": \"y\"}" becomes a two-element list. That is unchanged behaviour, not a regression, and this guard does not catch it because a list is a single leaf. Rejecting it would require either refusing brace values, which breaks the documented list syntax, or requiring type = "string" to yield a string, which would reject brace lists that configurations may rely on. Left alone deliberately.

Acceptance tests

• Have you added an acceptance test for the functionality being added?

Unit tests rather than acceptance tests, since the behaviour is entirely in value parsing and needs no cluster:

• both split outcomes, for type = "string" and for auto
• the escaped form round-tripping unchanged, and nothing being written to the config on failure
• brace list syntax, escaped commas, nested keys, multiline values and values containing = all still accepted
• set_list gaining elements, and the empty-list edge case
• a set_sensitive parse failure keeping the value out of the diagnostic, while a non-sensitive one keeps the parser error

Verified end to end outside the test suite as well, against a real chart driven with helm: a 248KB, 146-certificate bundle containing 49 commas and 24 backslashes renders byte-identical when escaped, and the same bundle unescaped reproduces the truncation. Stubbing the guard out makes the new tests fail with the original bug rather than passing vacuously.

Release Note

resource/helm_release: Report an error instead of silently truncating a `set`, `set_sensitive` or `set_list` value that helm's value parser would split on an unescaped comma.
resource/helm_release: Stop `set_sensitive` and `set_wo` values from being printed in plan output when parsing them fails, and from being written to debug logs.

References

Adjacent, none overlapping:

• set_sensitive fails to redact values when key name contains escaped dots (\.) #1788 / Fix/set sensitive escaped dots (fixes #737 #1788) #1866 — set_sensitive failing to redact when the key contains an escaped dot. Same underlying theme of strvals escaping being handled inconsistently, but at the other end of the pipeline: that is redaction of a parsed key path, this is correctness of the value being parsed. Fix/set sensitive escaped dots (fixes #737 #1788) #1866 adds splitKeyPath(), which lookupPath() here should use instead of its own strings.Split once that merges — noted in a comment on the function.
• helm_release does not respect sensitive attribute anymore #1221 / the provider continues to reveal sensitive variables during destroy or update in-place #1287 / Fix #1287, #1221: Mark values and metadata as sensitive to prevent sensitive data leakage in plan output #1817 — metadata.values being non-sensitive, so values-supplied secrets appear in plan output. Also unaddressed here.

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[hashicorp-cla-app]

All committers have signed the CLA.