Skip to content

Commit 8dab644

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. Every one of those arrived by hand, so `wget -O templates/cdi-cr.yaml` was not refreshing a vendored file, it was overwriting 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 is v1.66.1, which is what the operator manifest carries on main. ../kubevirt-cdi-operator pins nothing of its own: its recipe vendors whatever releases/latest resolved to the last time someone ran it, so the release the platform deploys can move without this file noticing. That is why this line is pointed at it by hand rather than derived, and why neither file reports the two having drifted. The difference from upstream is stated against v1.66.1 rather than as a standing fact: v1.64.0 enabled WebhookPvcRendering, which this object never carried and which v1.66.1 dropped again, so it ran both ways at one release and one way at the next. The comment says to re-derive it at each bump for that reason. 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 8dab644

3 files changed

Lines changed: 141 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: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,71 @@
11
export NAME=kubevirt-cdi
22
export NAMESPACE=cozy-$(NAME)
33

4+
# The CDI release templates/cdi-cr.yaml is reconciled against. Moving to a
5+
# newer CDI is an edit to this line. What the platform actually deploys is set
6+
# by ../kubevirt-cdi-operator, which pins nothing: its recipe vendors whatever
7+
# GitHub's releases/latest resolved to when someone last ran it. So this line
8+
# does not track that one, it has to be pointed at it by hand, and nothing in
9+
# either file reports the two having drifted apart.
10+
RELEASE = v1.66.1
11+
12+
# sha256 of the cdi-cr.yaml release asset as published. Upstream publishes no
13+
# checksum for it, so the pin is taken from a download rather than fetched
14+
# alongside one: it detects an asset that moved under its tag, and it is not
15+
# authenticity verification.
16+
CR_SHA256 = a497f90de608c1df26f9ee4095289373f17132d74a2042ca03e00c17c964f8a7
17+
18+
# sha256sum is not everywhere: current macOS carries a native one in /sbin,
19+
# older releases carry only shasum(1). Both print "<digest> <path>", so field
20+
# 1 is the digest whichever gets picked.
21+
SHA256SUM := $(shell command -v sha256sum >/dev/null 2>&1 && echo sha256sum || echo 'shasum -a 256')
22+
423
include ../../../hack/package.mk
524

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

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.
36+
# templates/cdi-cr.yaml is ours, not upstream's. Against upstream's bare CDI
37+
# object it carries Helm templating for cloneStrategyOverride, uploadProxyURL
38+
# and the worker-pod podResourceRequirements, the ExpandDisks feature gate, a
39+
# CriticalAddonsOnly toleration, and the two RBAC documents that let a tenant
40+
# clone from the golden-image namespace. That list is against upstream v1.66.1
41+
# and is not a standing property of CDI: v1.64.0 also enabled
42+
# WebhookPvcRendering, which this object never carried and which v1.66.1
43+
# dropped again, so the difference ran both ways at one release and one way at
44+
# the next. Re-derive it at each bump instead of inheriting it.
45+
#
46+
# So nothing here writes under templates/: fetching the release the object is
47+
# derived from confirms the bytes still hash to the pin, and stops there. What
48+
# that catches is an edit to RELEASE without a matching digest, and an asset
49+
# that moved under a tag that should be immutable. Moving RELEASE is therefore
50+
# the step that forces someone to reconcile this object by hand, which is the
51+
# point. It does not catch ../kubevirt-cdi-operator vendoring a new release,
52+
# because nothing here reads that package. RELEASE is checked by `update`
53+
# rather than by `test`, because helm-unittest reads rendered output and this
54+
# object renders no version string.
1655
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
56+
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; }
57+
@# The download survives a mismatch, because reconciling one starts by
58+
@# reading it. It is removed only once it has matched.
59+
@actual=$$($(SHA256SUM) cdi-cr.yaml.download | awk '{print $$1}'); \
60+
if [ "$$actual" != "$(CR_SHA256)" ]; then \
61+
echo "ERROR: cdi-cr.yaml for $(RELEASE) hashed $$actual," >&2; \
62+
echo " expected $(CR_SHA256). The download is kept as" >&2; \
63+
echo " cdi-cr.yaml.download." >&2; \
64+
echo " If RELEASE just moved, diff it against templates/cdi-cr.yaml," >&2; \
65+
echo " carry over what belongs there and update CR_SHA256. If RELEASE" >&2; \
66+
echo " did not change, the asset moved under a tag that should be" >&2; \
67+
echo " immutable: find out why first." >&2; \
68+
exit 1; \
69+
fi; \
70+
rm -f cdi-cr.yaml.download
71+
@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)