Skip to content

Harden BOM version-freshness gate (bidirectional parse, docs-only PRs, type-safety) #1576

Description

@yuanchen8911

Summary

Follow-up hardening for the BOM version-freshness gate introduced in PR #1572 (which resolved #1424). The gate's core path works — a registry version bump that forgets make bom-docs now fails CI — but three edge cases remain where it can false-pass or misbehave. None block #1572; tracking them here.

F1 — Freshness test is one-way and duplicate-unsafe

TestCommittedBOMVersionsMatchRegistry (tools/bom/freshness_test.go) only checks registry → document. It never compares the reverse direction, so:

  • Stale/removed rows go undetected. Removing a component from recipes/registry.yaml while leaving its row in docs/user/container-images.md still passes.
  • Duplicate rows, last-wins. parseBOMVersionTable walks every pipe table matching the Component/Pinned Version headers and does out[name] = ver, so a later duplicate row silently overwrites the generated value. It parses the whole doc, not just the generated markers.

Fix: parse only the generated BOM section (between its markers), reject duplicate component rows, and compare the registry and document component sets bidirectionally before comparing versions.

F2 — Freshness test never runs on docs-only PRs

.github/workflows/merge-gate.yaml classifies a PR as code=false when every changed file is **.md/docs/**/LICENSE, routing it to tests-skip. A PR that edits only docs/user/container-images.md therefore never executes the freshness test, despite docs/contributor/recipe.md stating version freshness is gated. The intended registry-bump path is covered (it touches registry.yamlcode=true), so this is the narrower docs-only hand-edit hole.

Fix: add a lightweight path-filtered job that runs the freshness test when docs/user/container-images.md or recipes/registry.yaml change, or soften the doc's "gated" wording to match reality.

F4 — Not deployment-type-safe

  • (a) Latent: the BOM generator (tools/bom/main.go) emits Version only from Helm.DefaultVersion and never from Kustomize.DefaultTag, but the freshness test's expected value falls back to Kustomize.DefaultTag. The first Kustomize component carrying a defaultTag would fail the freshness test even after make bom-docs. No such component exists today, so it cannot trigger yet.
  • (b) Hardening: assertExemptionDefaultsInstalled (pkg/recipe/version_pin_guard_test.go) accepts ref.Version == def || ref.Tag == def. For a Helm component it should compare only the active field; an inactive Tag matching def could spoof the sole-consumer proof.

Fix: select the active version field by effective component type and reject the incompatible field, in both the generator/test expectation and the exemption proof.

F3 — Example recipes are guarded by neither gate

The hand-maintained example recipes in examples/recipes/*.yaml bypass overlay resolution, and neither new guard examines them: TestOverlayVersionPinsMatchRegistry scans only store.Base/store.Overlays/store.Mixins, and TestCommittedBOMVersionsMatchRegistry compares only the registry against docs/user/container-images.md. PR #1572 refreshed the examples by hand (nvsentinel, gpu-operator, AKS kube-prometheus-stack pins, and a stale driver.version override), so they are consistent today — but a future registry or overlay bump can leave the examples advertising a stale chart version while both gates stay green, reintroducing the exact drift class #1424 targets, on the examples surface.

Fix: cover examples/recipes/* with a guard or a regeneration/consistency check (e.g. resolve each example against the registry and fail on undeclared divergence), or explicitly document them as an unguarded illustrative snapshot boundary if that is the intended contract.

F5 — Base is not considered a standalone consumer in the sole-consumer proof

assertExemptionDefaultsInstalled (pkg/recipe/version_pin_guard_test.go) resolves only overlays with non-nil Spec.Criteria; base.yaml pins kube-prometheus-stack to the 84.4.0 registry default but carries no criteria, so it is excluded from the resolved results. The AKS exemption therefore stays satisfied only because a criteria-bearing non-AKS overlay inherits base's default. This holds today (and the test fails loudly if no overlay carries criteria), but the sole-consumer guarantee is coupled to overlay structure rather than stated explicitly.

Fix: either count store.Base as a standalone consumer in the proof, or add a code comment documenting why base is intentionally excluded and what invariant keeps the exemption honest. Related to F4(b) (active-field selection in the same proof).

Context

Metadata

Metadata

Assignees

Type

Fields

No fields configured for Task.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions