From 648eb42cae3f12bd367102048fa89089fc40407a Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 21 Jul 2026 21:08:15 +0500 Subject: [PATCH] test(ci): make the image-ref guards see split-host and unparseable files Both round-2 reviewers independently landed on the same two test-side holes, and both are the guard being blind in exactly the way the bug it guards was. The completeness oracle derived its expected set from a grep for a contiguous ghcr.io/cozystack/cozystack/...@sha256: string, so it could not see any split-host shape: cilium, keycloak-operator, kubeovn, the OCI artifact and seven shape-3 refs, 12 of 43 first-party refs in all. That includes keycloak-operator, which the test's own rationale names as the motivating profile. The fixture-discovery grep in the same file had already been widened for this exact reason; the fix was never carried to the oracle. The oracle now applies the same shape knowledge to an independently discovered file list, which is the axis that actually drifts -- what each consumer disagreed about historically is which FILES it visits, not how a ref is spelled inside one. To keep that sharing safe, shape coverage is now pinned by its own test, so deleting a shape still fails something even though both sides of the comparison would move together. Sharing required splitting collect_refs_from_file out of collect_image_refs. Behaviour is unchanged: same 67 refs collected before and after, multus and the packages artifact both still present. A YAML oracle cannot cover everything, though. A reviewer planted a split-host ref in a Helm template, which yq cannot parse at all, and it survived every test. A file-level marker check now backstops that: any file carrying a first-party marker must be enumerated or explicitly allowlisted. Two files are allowlisted today, each with its reason -- the talos-csr-signer Dockerfile placeholder, and the documented capi gzip gap. This also closes the secondary weakness that comparing on repo@digest masks an unenumerated occurrence when the same image sits at an enumerated path, which is live for that capi file. Second hole: the postcondition's key-position branch had no coverage. Replacing the whole pattern with the naive "${RC}@sha256:" -- the narrowing rejected because cilium keeps tag: and digest: under separate keys -- left all 22 tests green, so the branch that makes the split shape visible was asserted by nothing. Added a cilium-shaped fixture at an unenumerated path. Both previously-surviving mutations are now caught. The oracle was also pre-filtered by registry marker before parsing, cutting it from 32s to 5s. Assisted-By: Claude Assisted-By: GPT-5 Signed-off-by: Myasnikov Daniil --- hack/lib/image-refs.sh | 62 ++++++++----- hack/promote-rewrite-tags.sh | 23 +++-- hack/promote-rewrite-tags_test.bats | 132 ++++++++++++++++++++++++++-- 3 files changed, 180 insertions(+), 37 deletions(-) diff --git a/hack/lib/image-refs.sh b/hack/lib/image-refs.sh index 57e283059..abc5e3dd7 100644 --- a/hack/lib/image-refs.sh +++ b/hack/lib/image-refs.sh @@ -123,11 +123,46 @@ image_ref_files() { # coerces most scalars on `+`, so only a !!map aborts ("!!str () cannot be # added to a !!map") and takes the whole file's expression with it. Being a # select() rather than a test(), the guard drops just the offending map. +# Emit every ref found in ONE file. Split out from collect_image_refs so a test +# can apply the same shape knowledge to an independently discovered file list: +# what drifted historically was which FILES each consumer looked at, not how a +# ref is spelled inside one, so an oracle wants to vary the former and hold the +# latter. Shape coverage is pinned separately by per-shape tests, which is why +# sharing this half does not weaken the suite. +# +# YAML is parsed; anything else is scraped textually. A .tag file has no YAML +# structure to speak of, and a declared extra file may be a manifest whose ref +# yq would find anyway — running both on it would merely duplicate, and the +# callers dedup. +collect_refs_from_file() { + _ir_f="${1:?collect_refs_from_file: required}" + [ -f "$_ir_f" ] || return 0 + + case "$_ir_f" in + *.yaml|*.yml) + _collect_yaml_shapes "$_ir_f" + ;; + *) + # Select on the digest rather than emitting whole lines so a blank line + # or a comment a future file may carry cannot reach the caller as a bogus + # ref. grep's no-match exit 1 is swallowed for the same reason the yq + # calls swallow theirs: an unmatched file is normal, not an error. + grep -Eo '[^[:space:]]+@sha256:[0-9a-f]{64}' "$_ir_f" 2>/dev/null || true + ;; + esac + return 0 +} + collect_image_refs() { _ir_root="${1:?collect_image_refs: required}" + image_ref_files "$_ir_root" | while IFS= read -r _ir_each; do + collect_refs_from_file "$_ir_each" + done + return 0 +} - for _ir_f in "$_ir_root"/*/*/values.yaml; do - [ -f "$_ir_f" ] || continue +_collect_yaml_shapes() { + _ir_f="$1" # shape 1 yq -r '.. | select(tag == "!!str") | select(test("@sha256:[0-9a-f]{64}"))' "$_ir_f" 2>/dev/null || true # shape 2. The sub("^/"; "") is what makes `registry` optional: absent, it @@ -145,26 +180,5 @@ collect_image_refs() { yq -r '(.global.registry.address // "") as $reg | select($reg != "") | select($reg | tag == "!!str") | .global.images[] | select(tag == "!!map") | select(has("repository") and has("tag")) | select(.tag | tag == "!!str") | select(.tag | test("@sha256:[0-9a-f]{64}")) | select(.repository | tag == "!!str") | $reg + "/" + .repository + "@" + (.tag | sub(".*@"; ""))' "$_ir_f" 2>/dev/null || true # shape 5 yq -r '.. | select(tag == "!!map") | select(has("platformSourceUrl") and has("platformSourceRef")) | (.platformSourceUrl | sub("^oci://"; "")) + "@" + (.platformSourceRef | sub("^digest="; ""))' "$_ir_f" 2>/dev/null || true - done - - # Storage shape 2: a .tag file holds one ref as plain text. Select on the - # digest rather than emitting the whole file so a blank line or a comment a - # future file may carry cannot reach the caller as a bogus ref. grep's - # no-match exit 1 is swallowed for the same reason the yq calls swallow - # theirs: an unmatched file is normal, not an error. - for _ir_f in "$_ir_root"/*/*/images/*.tag; do - [ -f "$_ir_f" ] || continue - grep -Eo '[^[:space:]]+@sha256:[0-9a-f]{64}' "$_ir_f" 2>/dev/null || true - done - - # Storage shape 3: declared files that embed a ref in surrounding text. The - # same digest-anchored extraction applies — in a k8s manifest it lifts the - # ref out of an `image: ` line. Any third-party ref it also picks up is - # discarded by the caller's ownership filter, exactly as for the other shapes. - for _ir_f in $IMAGE_REF_EXTRA_FILES; do - [ -f "$_ir_root/$_ir_f" ] || continue - grep -Eo '[^[:space:]]+@sha256:[0-9a-f]{64}' "$_ir_root/$_ir_f" 2>/dev/null || true - done - - return 0 + return 0 } diff --git a/hack/promote-rewrite-tags.sh b/hack/promote-rewrite-tags.sh index 81ca1a080..602a66792 100755 --- a/hack/promote-rewrite-tags.sh +++ b/hack/promote-rewrite-tags.sh @@ -54,15 +54,21 @@ RC_ESC=$(printf '%s' "$RC_VERSION" | sed -e 's/\./\\./g') files_list=$(mktemp) image_ref_files "$ROOT" > "$files_list" +# Failures are recorded and acted on after the loop rather than exiting inside +# it: the loop reads from $files_list, so removing that file in the same +# construct is both a shellcheck SC2094 hit and needless subtlety (the open +# descriptor would survive the unlink, but nobody should have to know that). rewritten=0 +failed_file="" +failed_why="" while IFS= read -r f; do # An unreadable file is NOT "a file with no match". Treating the two alike is # how a ref goes unrewritten while the run still reports success — the same # silent-skip shape as the enumeration bug this script exists to fix. Fail. if [ ! -r "$f" ]; then - echo "::error::${f} is not readable; refusing to promote a tree that cannot be fully scanned" >&2 - rm -f "$files_list" - exit 1 + failed_file="$f" + failed_why="is not readable; refusing to promote a tree that cannot be fully scanned" + break fi if grep -q "$RC_ESC" "$f"; then sed -i "s/${RC_ESC}/${STABLE_VERSION}/g" "$f" @@ -73,14 +79,19 @@ while IFS= read -r f; do # an ordinary outcome. grep_rc=$? if [ "$grep_rc" -gt 1 ]; then - echo "::error::failed to scan ${f} (grep exit ${grep_rc})" >&2 - rm -f "$files_list" - exit 1 + failed_file="$f" + failed_why="could not be scanned (grep exit ${grep_rc})" + break fi fi done < "$files_list" rm -f "$files_list" +if [ -n "$failed_file" ]; then + echo "::error::${failed_file} ${failed_why}" >&2 + exit 1 +fi + # Postcondition: scan WIDER than the rewrite. The rewrite is deliberately # narrow — a blind walk of every file under $ROOT would rewrite vendored chart # defaults that `make update` regenerates — but a narrow rewrite is exactly diff --git a/hack/promote-rewrite-tags_test.bats b/hack/promote-rewrite-tags_test.bats index b550565f8..cb41c733f 100644 --- a/hack/promote-rewrite-tags_test.bats +++ b/hack/promote-rewrite-tags_test.bats @@ -225,16 +225,33 @@ # published nightly points at the private build registry — which the # contract calls the worst of the three failure modes. # - # Compare on repo@digest so the cosmetic tag does not matter, and discover - # the expected set by content, independent of the enumeration under test. + # The oracle applies the SAME shape knowledge (collect_refs_from_file) to an + # INDEPENDENTLY discovered file list. That is the right axis: what drifted + # historically is which files each consumer visits, not how a ref is spelled + # inside one. Shape coverage is pinned separately by the per-shape test + # below, so deleting a shape still fails something even though both sides of + # this comparison would move together. + # + # An earlier revision derived the expected set from a grep for a CONTIGUOUS + # 'ghcr.io/cozystack/cozystack/...@sha256:' string. That was blind to every + # split-host shape — cilium, keycloak-operator, kubeovn, the OCI artifact and + # seven others, 12 of 43 first-party refs — including keycloak-operator, + # which this test's own rationale names as the motivating profile. . hack/lib/image-refs.sh tmp=$(mktemp -d) - collect_image_refs packages | grep 'cozystack/cozystack' \ - | sed -E 's|:[^/@]*@|@|' | sort -u > "$tmp/collected" - grep -rIoh --exclude-dir=charts --exclude='*.md' -E \ - 'ghcr\.io/cozystack/cozystack/[A-Za-z0-9._/-]+(:[A-Za-z0-9._-]+)?@sha256:[0-9a-f]{64}' \ - packages/ | sed -E 's|:[^/@]*@|@|' | sort -u > "$tmp/present" + canon() { sed -E 's|^[^ ]*=||; s|:[^/@]*@|@|' | grep '^ghcr\.io/cozystack/cozystack/' | sort -u; } + + collect_image_refs packages | canon > "$tmp/collected" + # Pre-filter to files that mention the registry at all before parsing them. + # Parsing every YAML under packages/ costs ~30s; this is the same file set + # for the purpose of finding FIRST-PARTY refs (canon discards everything + # else anyway) and runs in about a second. The marker covers the split forms + # too, so keycloak-operator and kubeovn are not filtered out. + grep -rIlE --exclude-dir=charts --exclude='*.md' \ + -e 'cozystack/cozystack' packages/ \ + | grep -E '\.(yaml|yml|tag)$' \ + | while IFS= read -r f; do collect_refs_from_file "$f"; done | canon > "$tmp/present" missed=$(comm -13 "$tmp/collected" "$tmp/present") if [ -n "$missed" ]; then @@ -247,6 +264,78 @@ rm -rf "$tmp" } +@test "every file carrying a first-party ref marker is enumerated or allowlisted" { + # File-level backstop for the class no YAML oracle can reach: a ref inside a + # file yq cannot parse. Helm templates are full of {{ }} and fail to parse + # entirely, so a first-party ref planted in one is invisible to the + # shape-based completeness check above — that is exactly how a reviewer's + # split-host fixture survived every test. + # + # Markers cover both the contiguous host and the two split forms, so + # keycloak-operator (sibling `registry:`) and kubeovn + # (`global.registry.address`) are seen. + . hack/lib/image-refs.sh + tmp=$(mktemp -d) + + image_ref_files packages | sort > "$tmp/enumerated" + grep -rIlE --exclude-dir=charts --exclude='*.md' \ + -e 'ghcr\.io/cozystack/cozystack/' \ + -e 'repository:[[:space:]]*"?cozystack/cozystack/' \ + -e 'address:[[:space:]]*"?ghcr\.io/cozystack/cozystack' \ + packages/ | sort > "$tmp/marked" + + # Known carriers that are deliberately NOT enumerated. Each needs a reason; + # this list is the place a new exception has to be argued for, rather than + # discovered during a release. + # - talos-csr-signer/Dockerfile: a ':' placeholder in build + # instructions, not a runtime ref — nothing stamps a version there. + # - capi-providers-cpprovider/files/control-plane-components.yaml: the + # documented gzip gap. The chart ships files/components.gz built from it, + # so rewriting only this copy would diverge the two. kamaji is + # component-versioned, so no tag rewrite is owed. + cat > "$tmp/allow" <<'ALLOW' +packages/apps/kubernetes/images/talos-csr-signer/Dockerfile +packages/system/capi-providers-cpprovider/files/control-plane-components.yaml +ALLOW + sort -o "$tmp/allow" "$tmp/allow" + + unexplained=$(comm -13 "$tmp/enumerated" "$tmp/marked" | comm -13 "$tmp/allow" -) + if [ -n "$unexplained" ]; then + echo "files carry a first-party image ref but are neither enumerated nor allowlisted:" >&2 + printf '%s\n' "$unexplained" >&2 + echo "Declare each in IMAGE_REF_EXTRA_FILES (hack/lib/image-refs.sh), or add it to the" >&2 + echo "allowlist in this test WITH A REASON if it is not a runtime image ref." >&2 + rm -rf "$tmp" + return 1 + fi + rm -rf "$tmp" +} + +@test "collect_refs_from_file understands every YAML shape" { + # Pins shape coverage directly, so deleting a shape fails here even though + # the completeness oracle above shares this code and would move with it. + . hack/lib/image-refs.sh + tmp=$(mktemp -d) + D=$(printf 'a%.0s' $(seq 64)) + + printf 'image: ghcr.io/cozystack/cozystack/one:v1@sha256:%s\n' "$D" > "$tmp/s1.yaml" + printf 'image:\n repository: ghcr.io/cozystack/cozystack/two\n tag: v1\n digest: "sha256:%s"\n' "$D" > "$tmp/s2.yaml" + printf 'image:\n registry: ghcr.io\n repository: cozystack/cozystack/three\n tag: "v1@sha256:%s"\n' "$D" > "$tmp/s3.yaml" + printf 'global:\n registry:\n address: ghcr.io/cozystack/cozystack\n images:\n - repository: four\n tag: "v1@sha256:%s"\n' "$D" > "$tmp/s4.yaml" + printf 'x:\n platformSourceUrl: oci://ghcr.io/cozystack/cozystack/five\n platformSourceRef: digest=sha256:%s\n' "$D" > "$tmp/s5.yaml" + printf 'ghcr.io/cozystack/cozystack/six:v1@sha256:%s\n' "$D" > "$tmp/s6.tag" + + for n in one two three four five six; do + case "$n" in one) f=s1.yaml ;; two) f=s2.yaml ;; three) f=s3.yaml ;; + four) f=s4.yaml ;; five) f=s5.yaml ;; six) f=s6.tag ;; esac + if ! collect_refs_from_file "$tmp/$f" | grep -q "cozystack/$n"; then + echo "shape for '$n' ($f) was not collected" >&2 + rm -rf "$tmp"; return 1 + fi + done + rm -rf "$tmp" +} + @test "the cozystack-packages OCI artifact is collected" { # Shape 5 (platformSourceUrl/platformSourceRef). Deleting that rule left # every suite green, so stable promotion could silently stop retagging the @@ -297,6 +386,35 @@ rm -rf "$tmp" } +@test "postcondition fires on a split tag/digest ref, not just an @sha256 one" { + # Covers the postcondition's key-position branch specifically. Replacing the + # whole pattern with the naive "${RC}@sha256:" — the narrowing that was + # explicitly rejected because cilium keeps `tag:` and `digest:` under + # separate keys — previously left every test green, so the branch that makes + # the split shape visible was asserted by nothing. + tmp=$(mktemp -d) + mkdir -p "$tmp/packages/system/surprise/files" + { + echo 'image:' + echo ' repository: ghcr.io/cozystack/cozystack/surprise' + echo ' tag: v9.9.9-rc.9' + printf ' digest: "sha256:%s"\n' "$(printf 'a%.0s' $(seq 64))" + } > "$tmp/packages/system/surprise/files/x.yaml" + + rc=0 + hack/promote-rewrite-tags.sh 9.9.9-rc.9 9.9.9 "$tmp/packages" >"$tmp/log" 2>&1 || rc=$? + if [ "$rc" -eq 0 ]; then + echo "expected a non-zero exit for a split tag/digest ref at an unenumerated path, got 0" >&2 + cat "$tmp/log" >&2 + rm -rf "$tmp"; return 1 + fi + grep -q 'still carry the rc version' "$tmp/log" || { + echo "expected the postcondition diagnostic; got:" >&2; cat "$tmp/log" >&2 + rm -rf "$tmp"; return 1 + } + rm -rf "$tmp" +} + @test "malformed versions are rejected before any file is touched" { # RC_VERSION becomes a sed pattern and STABLE_VERSION its replacement, so an # unvalidated argument is an injection surface as well as a correctness one.