Skip to content

Commit 73b4b77

Browse files
lexfreilexfrei
committed
fix(cdi): stop make update from overwriting the first-party CDI object
templates/cdi-cr.yaml is ours. Against upstream's bare CDI object it adds Helm templating for cloneStrategyOverride, uploadProxyURL and the worker-pod podResourceRequirements, the ExpandDisks feature gate, a CriticalAddonsOnly toleration, and the two RBAC documents that let a tenant clone from the golden-image namespace. I diffed the two to write that: upstream ships 283 bytes carrying one feature gate, two nodeSelectors and an imagePullPolicy, so the list is complete today and reads loosely only because it moves as people edit the file. Each entry arrived by hand. `wget -O templates/cdi-cr.yaml` therefore did not refresh a vendored file, it overwrote a first-party one -- the same loss as the `rm -rf templates` this recipe stopped doing, arriving through the fetch instead of the wipe. What replaces it: the release the object is derived from is pinned rather than resolved from GitHub's releases/latest redirect, and `update` fetches it only to confirm the bytes still hash to a measured sha256. Nothing is written under templates/. The next CDI bump fails that check, which is the point -- upstream moving its own defaults is when the local copy needs a human to reconcile it, not an overwrite. The download survives a mismatch, because reconciling one starts by reading it. RELEASE names v1.66.1, which is what ../kubevirt-cdi-operator vendors now. Nothing but a comment in each file ties the two together, so a pin left at the older release would be a check that keeps passing against a version the tree no longer carries. The `test:` target this package needed arrived separately, so what it gains here is the `.PHONY` line that came without: hack/package.mk declares only the targets it defines itself, so a file or directory named test or update beside this Makefile would otherwise make that target up to date and skip the recipe in silence. The suite pins the clone-strategy override, the ExpandDisks gate, the toleration and the clone RBAC, and renders the upload-proxy Ingress and TLSRoute, each under the gateway setting that selects it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 653e6fd commit 73b4b77

3 files changed

Lines changed: 139 additions & 7 deletions

File tree

‎packages/system/kubevirt-cdi/.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# `update` fetches here to check the pin. The file is removed once it has
2+
# matched, and deliberately kept when it has not so the mismatch can be read.
3+
cdi-cr.yaml.download

‎packages/system/kubevirt-cdi/Makefile‎

Lines changed: 57 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,69 @@
11
export NAME=kubevirt-cdi
22
export NAMESPACE=cozy-$(NAME)
33

4+
# The CDI release templates/cdi-cr.yaml is reconciled against, and the one
5+
# ../kubevirt-cdi-operator vendors its manifest from. Moving to a newer CDI is
6+
# an edit to this line. Nothing ties the two packages together, so a pin left
7+
# behind keeps passing against a release the tree no longer carries: move both.
8+
RELEASE = v1.66.1
9+
10+
# sha256 of the cdi-cr.yaml release asset as published. Upstream publishes no
11+
# checksum for it, so the pin is taken from a download rather than fetched
12+
# alongside one: it detects an asset that moved under its tag, and it is not
13+
# authenticity verification.
14+
CR_SHA256 = a497f90de608c1df26f9ee4095289373f17132d74a2042ca03e00c17c964f8a7
15+
16+
# sha256sum is not everywhere: current macOS carries a native one in /sbin,
17+
# older releases carry only shasum(1). Both print "<digest> <path>", so field
18+
# 1 is the digest whichever gets picked.
19+
SHA256SUM := $(shell command -v sha256sum >/dev/null 2>&1 && echo sha256sum || echo 'shasum -a 256')
20+
421
include ../../../hack/package.mk
522

623
# hack/package.mk defines no `test` target and hack/helm-unit-tests.sh only runs
724
# a package whose `make -n test` resolves, so a tests/ directory without this
825
# line is collected by nothing and the suite is skipped in silence.
26+
#
27+
# Phony because hack/package.mk's .PHONY line lists only the targets that file
28+
# defines: without either name here, a path named `test` or `update` next to
29+
# this Makefile would make that target up to date and skip the recipe.
30+
.PHONY: test update
931
test:
1032
helm unittest .
1133

12-
# templates/ holds first-party manifests as well as the vendored CR: the
13-
# upload-proxy Ingress and TLSRoute, which nothing below refetches. So this
14-
# recipe must never clear the directory -- wget -O truncates the file it
15-
# fetches and leaves the rest alone.
34+
# templates/cdi-cr.yaml is ours, not upstream's. Against upstream's bare CDI
35+
# object it carries Helm templating for cloneStrategyOverride, uploadProxyURL
36+
# and the worker-pod podResourceRequirements, the ExpandDisks feature gate, a
37+
# CriticalAddonsOnly toleration, and the two RBAC documents that let a tenant
38+
# clone from the golden-image namespace. I diffed the two at this release to
39+
# write that, so the list is complete today: upstream ships 283 bytes with one
40+
# feature gate, two nodeSelectors and an imagePullPolicy, and we are a strict
41+
# superset of it. The wording stays loose because the list moves as people edit
42+
# the file, not because it went unchecked. At v1.64.0 the divergence also ran
43+
# the other way, upstream enabling WebhookPvcRendering where we do not; v1.66.1
44+
# no longer ships that gate, so read this as a map to re-derive at each bump
45+
# rather than a fact about CDI in general.
46+
#
47+
# So nothing here writes under templates/: fetching the release the object is
48+
# derived from confirms the bytes still hash to the pin, and stops there. At
49+
# the next CDI bump this fails, which is the point -- upstream moving its own
50+
# defaults is when the local copy needs a human to reconcile it rather than an
51+
# overwrite. RELEASE is checked by `update`, not by `test`: the object carries
52+
# no version string for a rendered-output assertion to compare against.
1653
update:
17-
mkdir -p templates
18-
export VERSION=$$(basename $$(curl -s -w %{redirect_url} https://github.com/kubevirt/containerized-data-importer/releases/latest)) && \
19-
wget https://github.com/kubevirt/containerized-data-importer/releases/download/$$VERSION/cdi-cr.yaml -O templates/cdi-cr.yaml
54+
wget https://github.com/kubevirt/containerized-data-importer/releases/download/$(RELEASE)/cdi-cr.yaml -O cdi-cr.yaml.download || { rm -f cdi-cr.yaml.download; exit 1; }
55+
@# The download survives a mismatch, because reconciling one starts by
56+
@# reading it. It is removed only once it has matched.
57+
@actual=$$($(SHA256SUM) cdi-cr.yaml.download | awk '{print $$1}'); \
58+
if [ "$$actual" != "$(CR_SHA256)" ]; then \
59+
echo "ERROR: cdi-cr.yaml for $(RELEASE) hashed $$actual," >&2; \
60+
echo " expected $(CR_SHA256). The download is kept as" >&2; \
61+
echo " cdi-cr.yaml.download." >&2; \
62+
echo " If RELEASE just moved, diff it against templates/cdi-cr.yaml," >&2; \
63+
echo " carry over what belongs there and update CR_SHA256. If RELEASE" >&2; \
64+
echo " did not change, the asset moved under a tag that should be" >&2; \
65+
echo " immutable: find out why first." >&2; \
66+
exit 1; \
67+
fi; \
68+
rm -f cdi-cr.yaml.download
69+
@echo "upstream cdi-cr.yaml for $(RELEASE) matches the pin; templates/cdi-cr.yaml is maintained by hand and was left alone."

‎packages/system/kubevirt-cdi/tests/local_templates_test.yaml‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
suite: cozy-kubevirt-cdi local templates
2+
release:
3+
name: kubevirt-cdi
4+
namespace: cozy-kubevirt-cdi
5+
# helm-unittest renders the whole chart for every test, so both upload-proxy
6+
# routes are evaluated even by a test that names neither. Both open by indexing
7+
# `.Values._cluster`, which the platform injects and this does not, and `index`
8+
# on nil is a hard template error.
9+
set:
10+
_cluster:
11+
root-host: example.com
12+
expose-services: cdi-uploadproxy
13+
gateway-enabled: "false"
14+
tests:
15+
# Upstream's cdi-cr.yaml carries none of this.
16+
- it: keeps the local additions to the CDI object
17+
template: cdi-cr.yaml
18+
documentSelector:
19+
path: kind
20+
value: CDI
21+
matchMany: false
22+
asserts:
23+
# Without the override CDI falls back to a host-assisted copy per VM.
24+
- equal:
25+
path: spec.cloneStrategyOverride
26+
value: csi-clone
27+
- contains:
28+
path: spec.config.featureGates
29+
content: ExpandDisks
30+
- contains:
31+
path: spec.infra.tolerations
32+
content:
33+
key: CriticalAddonsOnly
34+
operator: Exists
35+
36+
# What lets a tenant service account clone from the golden-image namespace.
37+
# `any: true` on both: a bare containsDocument holds every document in the
38+
# template to the match, and this template renders three.
39+
- it: keeps the golden-image clone RBAC
40+
template: cdi-cr.yaml
41+
asserts:
42+
- containsDocument:
43+
apiVersion: rbac.authorization.k8s.io/v1
44+
kind: ClusterRole
45+
name: cdi-copy-dv
46+
any: true
47+
- containsDocument:
48+
apiVersion: rbac.authorization.k8s.io/v1
49+
kind: RoleBinding
50+
name: cdi-clone-dv
51+
namespace: cozy-public
52+
any: true
53+
54+
# Addressed as a `template:` because helm-unittest fails outright on one it
55+
# cannot find, which makes a deleted file loud rather than a smaller render.
56+
- it: renders the upload-proxy Ingress when the gateway is off
57+
template: cdi-uploadproxy-ingress.yaml
58+
asserts:
59+
- equal:
60+
path: spec.rules[0].host
61+
value: cdi-uploadproxy.example.com
62+
- equal:
63+
path: metadata.annotations["nginx.ingress.kubernetes.io/ssl-passthrough"]
64+
value: "true"
65+
66+
- it: renders the upload-proxy TLSRoute when the gateway is on
67+
template: cdi-uploadproxy-tlsroute.yaml
68+
set:
69+
_cluster:
70+
root-host: example.com
71+
expose-services: cdi-uploadproxy
72+
gateway-enabled: "true"
73+
asserts:
74+
- equal:
75+
path: spec.hostnames[0]
76+
value: cdi-uploadproxy.example.com
77+
- equal:
78+
path: spec.parentRefs[0].sectionName
79+
value: tls-cdi-uploadproxy

0 commit comments

Comments
 (0)