mirror of
https://github.com/cozystack/cozystack.git
synced 2026-10-27 23:27:58 +09:00
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 <noreply@anthropic.com>
Assisted-By: GPT-5 <noreply@openai.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
This commit is contained in:
3 files changed
+180
-37
No files matched your search
+38
-24
@@ -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: <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: <root> 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: <ref>` 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
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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 ':<chart-version>' 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.
|
||||
|
||||
Reference in new issue
Block a user