GoogleContainerTools / GoogleContainerTools/skaffold

skaffold filter drops the trailing newline from the last value in a ConfigMap (regression in v2.6.0)

Open
#10,159 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
15.9k
Forks
1.7k
Avg merge
3d 9h
Merged PRs (30d)
10

Description

### Expected behaviour

`skaffold filter` should not change the *content* of the manifests it passes through. A ConfigMap value that ends with a newline should still end with a newline afterwards.

### Actual behaviour

The value under the **alphabetically last key** of a ConfigMap's `data` map loses its trailing newline. Values under earlier keys are unaffected.

```
$ helm template t ./chart | grep 'zzz:'
zzz: |

$ helm template t ./chart | skaffold filter | grep 'zzz:'
zzz: |-
```

The block chomping indicator changes from `|` to `|-`, and the parsed string really does differ — `'x\n# EOF\n'` in, `'x\n# EOF'` out. This is not a formatting nit: for any file format with a required terminator, the file becomes invalid.

### Reproduction

```sh
mkdir -p /tmp/nl/chart/templates && cd /tmp/nl
printf 'apiVersion: skaffold/v4beta11\nkind: Config\nmetadata:\n name: t\n' > skaffold.yaml
printf 'apiVersion: v2\nname: t\nversion: 0.1.0\n' > chart/Chart.yaml
printf 'apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: t\ndata:\n zzz: |\n x\n # EOF\n' > chart/templates/cm.yaml

helm template t ./chart | grep 'zzz:' # zzz: | <-- correct
helm template t ./chart | skaffold filter | grep 'zzz:' # zzz: |- <-- newline lost
```

The stub `skaffold.yaml` is only there because `filter` requires a config in the working directory; its contents are irrelevant — a bare config with no `deploy:` section reproduces it exactly as a full one does.

### The rule

The value appearing **last in the mapping, in document order** loses its trailing newline. Everything before it survives untouched. Key names are irrelevant.

Earlier notes here said "alphabetically last" — wrong, and it made it into the first version of #10159 (corrected by this edit). Every test had used key names whose alphabetical order matched their write order, so the two were indistinguishable. Setting them in opposition settles it:

| `data` mapping | doc-last | alpha-last | altered |
|---|---|---|---|
| `zzz_plain`, then `aaa_block: \|` | `aaa_block` | `zzz_plain` | **`aaa_block`** |
| `aaa_block: \|`, then `zzz_plain` | `zzz_plain` | `zzz_plain` | none |
| `zzz_block: \|`, then `aaa_block: \|` | `aaa_block` | `zzz_block` | **`aaa_block`** |

It follows document order every time.

| `data` map shape | result |
|---|---|
| block scalar first, plain scalar last | unaffected |
| plain scalar first, block scalar last | **last value altered** |
| two block scalars | **only the last altered** |
| `\|-` last (no trailing newline to begin with) | unaffected |
| `\|+` last (ends `\n\n`) | **altered** (`'a\n'` → `'a'`) |
| a second YAML document following it | still **altered** |

Worth flagging for anyone writing a test: a `data` map whose last key happens to hold a `|-` value reports a false pass on every version.

### Regression range

Bisected to a single release. v2.5.1 is clean; v2.6.0 and every release since is not.

| version | released | result |
|---|---|---|
| v2.0.17 | | pass |
| v2.2.0 | | pass |
| v2.4.1 | | pass |
| **v2.5.1** | 2023-06-12 | **pass — last good** |
| **v2.6.0** | 2023-06-27 | **fail — first bad** |
| v2.6.1, v2.6.2, v2.6.3 | | fail |
| v2.7.0, v2.7.1 | | fail |
| v2.11.1 | | fail |
| v2.14.2, v2.15.0, v2.16.1, v2.17.3, v2.18.3, v2.19.0 | | fail |
| v2.21.0, v2.22.1, v2.23.0, v2.24.0 | 2026-07-23 | fail |

v2.0.17's pass is not a pass-through: it re-serialises like the later versions do (output differs from input, redundant quotes dropped — `"1.7.0"` becomes `1.7.0`), it just preserves the trailing newline while doing so.

### Suspected cause

`v2.5.1...v2.6.0` changes no YAML dependency — `gopkg.in/yaml.v3 v3.0.1`, `sigs.k8s.io/yaml v1.3.0` and `sigs.k8s.io/kustomize/kyaml v0.10.17` are identical either side.

The change appears to be #8902 ("chore: port apply-setter krm function over to skaffold", `ce682a53`), which added an apply-setters pass to `runFilter` in `cmd/skaffold/app/cmd/filter.go`:

```go
+var ass applysetters.ApplySetters
+manifestOverrides := pkgutil.EnvSliceToMap(opts.ManifestsOverrides, "=")
+for k, v := range manifestOverrides {
+ ass.Setters = append(ass.Setters, applysetters.Setter{Name: k, Value: v})
+}
+manifestList, err = ass.Apply(ctx, manifestList)
+if err != nil {
+ return err
+}
```

`ApplySetters` is a `kio.Filter` built on `sigs.k8s.io/kustomize/kyaml`, so `Apply` parses and re-emits the entire manifest through a kyaml pipeline, and kyaml's serialiser normalises the block scalar.

The call is **unguarded**: it runs on every `skaffold filter` invocation, while `ass.Setters` is empty unless manifest overrides were supplied. So a deploy that passes no overrides still pays a full kyaml round-trip, and still loses the newline.

### Suggested fix

Skip the pass when there is nothing to apply, which restores v2.5.1 behaviour for the common case:

```go
if len(ass.Setters) > 0 {
manifestList, err = ass.Apply(ctx, manifestList)
if err != nil {
return err
}
}
```

That alone would not help users who *do* pass overrides, so preserving the value verbatim (or at least carrying over the input's chomping indicator) in the re-emit is the more complete fix.

### Why it matters in practice

Skaffold's Helm deployer runs `filter` as Helm's post-renderer, so this reaches anything deployed that way. Helm stores the post-rendered output, so the damaged value is what lands in the cluster.

The case that led us here: the `grafana/k8s-monitoring` chart mounts a `self-reporting-metric.prom` file into Grafana Alloy and scrapes it with the node_exporter textfile collector. That file is OpenMetrics, whose `# EOF` marker must be newline-terminated. With the newline stripped, Alloy logs this on every scrape interval, indefinitely:

```
level=error msg="failed to collect textfile data" component_path=/
component_id=prometheus.exporter.unix.kubernetes_monitoring_telemetry collector=textfile
file=self-reporting-metric.prom err="failed to parse textfile data from
\"/etc/alloy/self-reporting-metric.prom\": text format parsing error in line 12:
unexpected end of input stream"
```

It took a while to find because the stored release manifest differs from a plain `helm template` by exactly one byte, on one key.

### Helm is not involved

For completeness — the same value through Helm v4's postrenderer-plugin plumbing with a no-op plugin is preserved, so the transformation is skaffold's:

```sh
mkdir -p /tmp/nl/noop
printf 'name: "noopplug"\nversion: "0.1.0"\ntype: postrenderer/v1\napiVersion: v1\nruntime: subprocess\nruntimeConfig:\n platformCommand:\n - command: /usr/bin/cat\n' > /tmp/nl/noop/plugin.yaml
helm plugin install /tmp/nl/noop
helm template t ./chart --post-renderer noopplug | grep 'zzz:'
# zzz: | <-- preserved
```

### Information

- **skaffold version**: v2.24.0 (also reproduced on v2.6.0 through v2.23.0; see table)
- **helm version**: v4.2.3
- **operating system**: Ubuntu 26.04 LTS, x86_64
- **installed via**: released binaries from this repo

Contributor guide

Open the contributing guide

Research direction

Reproduce the trailing-newline loss with the provided Helm manifest, then inspect cmd/skaffold/app/cmd/filter.go, especially runFilter and its apply-setters pass. Verify behavior with and without manifest overrides, and consider the existing applysetters.Apply path; done means filter preserves the final value's newline while still handling overrides correctly.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, helm, kubernetes
Domain
cli, devops, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.