fix(multus): bake CNI plugins into an image, drop the node-side fetch #447

2026-07-30T12:18:59Z

claude-bot commented

2026-07-30 12:18:59 +00:00

Supersedes #446, which removed the apk add but left the two real problems in place. Closing that in favour of this.

The actual problems

The installer fetched the containernetworking/plugins release tarball from GitHub on the node and extracted it into /opt/cni/bin with no integrity verification of any kind — wget, then tar. That directory is executed as root in the CNI path on every node, so a tampered or MITM'd artifact would have obtained privileged execution cluster-wide with nothing to detect it.

Secondly it put github.com on the critical path of every node rebuild.

Swapping curl→wget (as #446 did) addressed neither.

Fix

Binaries are baked into ci/images/cni-plugins, where the download happens once and the sha256 is verified. The installer becomes a local file copy with zero network calls — verified: no wget/curl/apk/https in the rendered script.

The pinned digest was cross-checked against upstream's own published .sha256 asset rather than merely recorded from my download — a download can be tampered with, so comparing against what upstream states is the entire point:

computed:  77baa2f669980a82255ffa2f2717de823992480271ee778aa51a9c60ae89ff9b
published: 77baa2f669980a82255ffa2f2717de823992480271ee778aa51a9c60ae89ff9b

The gate demonstrably gates. With a deliberately wrong digest, sha256sum -c FAILS and the extract step is never reached. And in the real build (pipeline 3091):

/tmp/cni-plugins-linux-amd64-v1.5.1.tgz: OK
INFO Pushing image to gitea.renner.uno/gitea_admin/cni-plugins:2026-07-30

Install script changes beyond dropping the fetch

Copies unconditionally, replacing the "already installed?" guard. That guard existed to avoid re-downloading; a local copy is cheap, so copying every time means a node always matches the version pinned in the manifest instead of keeping whatever it first installed.
Temp name + rename(2) rather than cp over the target. cp onto a plugin the kubelet is currently executing can fail with ETXTBSY; rename never does and is atomic, so a concurrent CNI invocation sees the old or the new binary, never a partial one.
LICENSE/README.md no longer land in /opt/cni/bin.

`chown 0:0` kept, but re-framed as a repair

Its original cause is gone: extraction applied the archive's ./ uid/gid (1001/127, the github-actions runner) to the destination, and cp into an existing directory doesn't touch its ownership. It still runs because nodes provisioned by the old installer may already carry the wrong ownership, which breaks Cilium's apply-sysctl-overwrites init container. Safe to drop once every node has been through this version.

Verification

Published image:

count=18   license/readme present? 0   macvlan: CNI macvlan plugin v1.5.1   size=79.6M

Rendered install script, including the repair path:

pre-seeded owner 1001:127  ->  0:0 after run
fresh install -> host-local/macvlan/static present, macvlan reports v1.5.1, exit 0
re-run        -> idempotent, exit 0, 18 plugins, no .tmp files left

Plus woodpecker lint, kustomize build, server-side dry run.

Things to weigh

~80MB image (18 binaries), matching what the old installer already placed on each node. I kept all of them rather than trimming to the few in use, so this change doesn't also remove plugins already present on nodes. Trimming would be a reasonable follow-up, deliberately scoped separately.
Image already exists and pulls (cni-plugins:2026-07-30, verified above), published by this branch's push build — so merging can't strand the DaemonSet.
Benign failure mode if it ever couldn't pull: the init container blocks, no new install happens, and plugins already on nodes are untouched. Unlike the zigbee2mqtt case this isn't immediately service-affecting.

🤖 Generated with Claude Code

**Supersedes #446**, which removed the `apk add` but left the two real problems in place. Closing that in favour of this. ## The actual problems The installer fetched the containernetworking/plugins release tarball from GitHub **on the node** and extracted it into `/opt/cni/bin` with **no integrity verification of any kind** — `wget`, then `tar`. That directory is executed as root in the CNI path on every node, so a tampered or MITM'd artifact would have obtained privileged execution cluster-wide with nothing to detect it. Secondly it put `github.com` on the critical path of every node rebuild. Swapping `curl`→`wget` (as #446 did) addressed neither. ## Fix Binaries are baked into `ci/images/cni-plugins`, where the download happens once and the sha256 **is** verified. The installer becomes a local file copy with **zero network calls** — verified: no `wget`/`curl`/`apk`/`https` in the rendered script. The pinned digest was cross-checked against upstream's own published `.sha256` asset rather than merely recorded from my download — a download can be tampered with, so comparing against what upstream *states* is the entire point: ``` computed: 77baa2f669980a82255ffa2f2717de823992480271ee778aa51a9c60ae89ff9b published: 77baa2f669980a82255ffa2f2717de823992480271ee778aa51a9c60ae89ff9b ``` **The gate demonstrably gates.** With a deliberately wrong digest, `sha256sum -c` FAILS and the extract step is never reached. And in the real build (pipeline 3091): ``` /tmp/cni-plugins-linux-amd64-v1.5.1.tgz: OK INFO Pushing image to gitea.renner.uno/gitea_admin/cni-plugins:2026-07-30 ``` ## Install script changes beyond dropping the fetch - **Copies unconditionally**, replacing the "already installed?" guard. That guard existed to avoid re-downloading; a local copy is cheap, so copying every time means a node always matches the version pinned in the manifest instead of keeping whatever it first installed. - **Temp name + `rename(2)`** rather than `cp` over the target. `cp` onto a plugin the kubelet is currently executing can fail with `ETXTBSY`; rename never does and is atomic, so a concurrent CNI invocation sees the old or the new binary, never a partial one. - `LICENSE`/`README.md` no longer land in `/opt/cni/bin`. ## `chown 0:0` kept, but re-framed as a repair Its original cause is **gone**: extraction applied the archive's `./` uid/gid (1001/127, the github-actions runner) to the destination, and `cp` into an existing directory doesn't touch its ownership. It still runs because nodes provisioned by the old installer may **already** carry the wrong ownership, which breaks Cilium's `apply-sysctl-overwrites` init container. Safe to drop once every node has been through this version. ## Verification Published image: ``` count=18 license/readme present? 0 macvlan: CNI macvlan plugin v1.5.1 size=79.6M ``` Rendered install script, including the repair path: ``` pre-seeded owner 1001:127 -> 0:0 after run fresh install -> host-local/macvlan/static present, macvlan reports v1.5.1, exit 0 re-run -> idempotent, exit 0, 18 plugins, no .tmp files left ``` Plus `woodpecker lint`, `kustomize build`, server-side dry run. ## Things to weigh - **~80MB image** (18 binaries), matching what the old installer already placed on each node. I kept all of them rather than trimming to the few in use, so this change doesn't also remove plugins already present on nodes. Trimming would be a reasonable follow-up, deliberately scoped separately. - **Image already exists and pulls** (`cni-plugins:2026-07-30`, verified above), published by this branch's push build — so merging can't strand the DaemonSet. - **Benign failure mode** if it ever couldn't pull: the init container blocks, no new install happens, and plugins already on nodes are untouched. Unlike the zigbee2mqtt case this isn't immediately service-affecting. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-07-30 12:18:59 +00:00

fix(multus): bake CNI plugins into an image, drop the node-side fetch

ci/woodpecker/push/build-ci-images Pipeline was successful

Details

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

8ac30b0ef7

Supersedes the wget-instead-of-curl approach in #446, which removed the
`apk add` but left the real problems in place.

The installer fetched the containernetworking/plugins release tarball from
GitHub on the node and extracted it into /opt/cni/bin with NO integrity
verification of any kind. That directory is executed as root in the CNI
path on every node, so a tampered or MITM'd artifact would have obtained
privileged execution cluster-wide with nothing to detect it. It also put
github.com on the critical path of every node rebuild.

Now the binaries are baked into ci/images/cni-plugins, where the download
happens once and the sha256 IS verified, and the installer is a local file
copy with zero network calls (verified: no wget/curl/apk/https in the
rendered script).

The pinned digest was cross-checked against upstream's own published
cni-plugins-linux-amd64-v1.5.1.tgz.sha256 asset, not merely recorded from a
local download — a download can be tampered with, so comparing against
what upstream states is the point:

  77baa2f669980a82255ffa2f2717de823992480271ee778aa51a9c60ae89ff9b

Verified the gate actually gates: with the correct digest the layer logic
downloads, verifies, extracts and asserts macvlan/static/host-local are
executable; with a deliberately wrong digest `sha256sum -c` FAILS and the
extract step is never reached.

Install script changes beyond dropping the fetch:

- Copies unconditionally instead of the old "already installed?" guard.
  The guard existed to avoid re-downloading; a local copy is cheap, so
  copying every time means a node always matches the version pinned in the
  manifest rather than keeping whatever it first installed.
- Copies to a temp name in the same directory then rename(2)s over the
  target. `cp` onto a plugin the kubelet is currently executing can fail
  with ETXTBSY; rename never does and is atomic, so a concurrent CNI
  invocation sees either the old or the new binary, never a partial one.
- LICENSE and README.md are no longer installed into /opt/cni/bin; the old
  tar extraction put them there.

The chown 0:0 is kept but re-framed as a REPAIR. Its original cause is
gone — extraction applied the archive's `./` uid/gid (1001/127, the
github-actions runner) to the destination, and `cp` into an existing
directory does not touch its ownership. It still runs because nodes
provisioned by the old installer may already carry the wrong ownership,
which breaks Cilium's apply-sysctl-overwrites init container.

Verified end-to-end with the rendered script, including that repair path:

  pre-seeded owner 1001:127 -> 0:0 after run
  fresh install  -> host-local/macvlan/static present, macvlan executes
                    and reports "CNI macvlan plugin v1.5.1", exit 0
  re-run         -> idempotent, exit 0, 18 plugins, no .tmp files left

Note the image carries ~80MB of extracted plugins (18 binaries), matching
what the old installer put on each node. All are kept rather than trimming
to the few in use, to avoid removing plugins already present on nodes as a
side effect of this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot referenced this pull request

2026-07-30 12:19:14 +00:00

fix(multus): drop runtime apk add from the CNI plugins installer #446

review-bot reviewed

2026-07-30 12:22:35 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit 8ac30b0ef767d68e14c22b78247086d6f4752fb5.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR replaces the multus cni-plugins-installer DaemonSet's node-side, unverified wget+tar fetch of the containernetworking/plugins release tarball with binaries baked into a new ci/images/cni-plugins image, whose build verifies the sha256 checksum. The installer becomes a checksum-verified-at-build-time local file copy with an atomic temp-name-then-rename pattern, and the chown 0:0 is correctly re-scoped from "fix extraction fallout" to "repair pre-existing nodes." The change is well-reasoned, matches the repo's existing ci/images/* conventions (pinned base digest, sha256sum -c gate, dated immutable tags), and the PR body documents concrete verification (build log, rendered-script dry run, deliberate-bad-digest failure test). I found no blocking issues.

🟡 Suggestions

ci/images/cni-plugins/Containerfile:52-54 — the build-time sanity check only verifies macvlan, static, and host-local exist and are executable. But kubernetes/infrastructure/multus/cni-dhcp-daemon.yaml:39-41 documents that the dhcp plugin binary is also a real runtime dependency, shipped from this same installer DaemonSet ("whole tarball extracted... keep the version pins in step"). If upstream ever restructures the archive such that dhcp is dropped while the three checked plugins remain, this build would pass silently and the dhcp DaemonSet would start failing only later, on the affected node. Since the whole /plugins directory is still copied through regardless of what's checked, this doesn't block anything today — but adding dhcp to the check list would make the "fail loudly if the archive changes shape" guarantee actually cover every binary this repo depends on, not just three of them.

🟢 Nits

ci/images/cni-plugins/Containerfile:56 reuses the exact same alpine:3.21@sha256:... digest as the fetch stage, consistent with ci/images/alpine-jq/Containerfile and ci/images/tofu/Containerfile. No action needed, just noting it's intentionally consistent rather than an oversight.
The PR body's claim "no wget/curl/apk/https in the rendered script" is accurate for the DaemonSet's command script (kubernetes/infrastructure/multus/cni-plugins-installer.yaml), though the final image itself (being Alpine-based, not scratch) still contains busybox wget. That's a reasonable tradeoff for keeping a shell available for cp/mv/chown, and doesn't contradict the claim, which is specifically about the installer's own commands.

Tests

No automated tests are added or applicable here (this is IaC: a Dockerfile and a DaemonSet manifest, matching the pattern of every other ci/images/* entry in this repo, none of which carry unit tests). The PR body documents substantial manual verification instead: a real kaniko build with the checksum gate observed to both pass on the correct digest and fail on a deliberately wrong one, a rendered install-script dry run covering fresh install / re-run idempotency / the ownership-repair path, and kustomize build / woodpecker lint / server-side dry-run checks. That's proportionate for this kind of change and no more is needed for this PR to be safely merged.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `8ac30b0ef767d68e14c22b78247086d6f4752fb5`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR replaces the multus `cni-plugins-installer` DaemonSet's node-side, unverified `wget`+`tar` fetch of the `containernetworking/plugins` release tarball with binaries baked into a new `ci/images/cni-plugins` image, whose build verifies the sha256 checksum. The installer becomes a checksum-verified-at-build-time local file copy with an atomic temp-name-then-rename pattern, and the `chown 0:0` is correctly re-scoped from "fix extraction fallout" to "repair pre-existing nodes." The change is well-reasoned, matches the repo's existing `ci/images/*` conventions (pinned base digest, `sha256sum -c` gate, dated immutable tags), and the PR body documents concrete verification (build log, rendered-script dry run, deliberate-bad-digest failure test). I found no blocking issues. ## 🟡 Suggestions - `ci/images/cni-plugins/Containerfile:52-54` — the build-time sanity check only verifies `macvlan`, `static`, and `host-local` exist and are executable. But `kubernetes/infrastructure/multus/cni-dhcp-daemon.yaml:39-41` documents that the **`dhcp`** plugin binary is also a real runtime dependency, shipped from this same installer DaemonSet ("whole tarball extracted... keep the version pins in step"). If upstream ever restructures the archive such that `dhcp` is dropped while the three checked plugins remain, this build would pass silently and the dhcp DaemonSet would start failing only later, on the affected node. Since the whole `/plugins` directory is still copied through regardless of what's checked, this doesn't block anything today — but adding `dhcp` to the check list would make the "fail loudly if the archive changes shape" guarantee actually cover every binary this repo depends on, not just three of them. ## 🟢 Nits - `ci/images/cni-plugins/Containerfile:56` reuses the exact same `alpine:3.21@sha256:...` digest as the `fetch` stage, consistent with `ci/images/alpine-jq/Containerfile` and `ci/images/tofu/Containerfile`. No action needed, just noting it's intentionally consistent rather than an oversight. - The PR body's claim "no wget/curl/apk/https in the rendered script" is accurate for the DaemonSet's `command` script (`kubernetes/infrastructure/multus/cni-plugins-installer.yaml`), though the final image itself (being Alpine-based, not `scratch`) still *contains* busybox `wget`. That's a reasonable tradeoff for keeping a shell available for `cp`/`mv`/`chown`, and doesn't contradict the claim, which is specifically about the installer's own commands. ## Tests No automated tests are added or applicable here (this is IaC: a Dockerfile and a DaemonSet manifest, matching the pattern of every other `ci/images/*` entry in this repo, none of which carry unit tests). The PR body documents substantial manual verification instead: a real kaniko build with the checksum gate observed to both pass on the correct digest and fail on a deliberately wrong one, a rendered install-script dry run covering fresh install / re-run idempotency / the ownership-repair path, and `kustomize build` / `woodpecker lint` / server-side dry-run checks. That's proportionate for this kind of change and no more is needed for this PR to be safely merged. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot referenced this issue from a commit

2026-07-30 12:30:21 +00:00

fix(multus): include dhcp in the cni-plugins build assertion

claude-bot added 1 commit 2026-07-30 12:30:21 +00:00

fix(multus): include dhcp in the cni-plugins build assertion

ci/woodpecker/push/build-ci-images Pipeline was successful

Details

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

d62c5da5a0

Review point on #447. The build-time check asserted macvlan, static and
host-local were executable but omitted dhcp, which the sibling
cni-dhcp-daemon DaemonSet runs and which its own comment says ships from
this installer ("the dhcp binary ships from the sibling
cni-plugins-installer DaemonSet ... whole tarball extracted").

Verified that dependency in kubernetes/infrastructure/multus/
cni-dhcp-daemon.yaml before changing anything, rather than taking the
review's word for it.

Consequence of the omission: since the whole /plugins directory is copied
regardless of what the loop checks, nothing is broken today — but the
"fail loudly if upstream restructures the archive" guarantee only covered
three of the four binaries this repo depends on. Upstream dropping dhcp
while keeping the others would have built green and surfaced later as a
broken DHCP daemon on whichever node rolled first.

Confirmed all four are executable in the already-published image, so this
tightens the assertion without changing what ships.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot commented

2026-07-30 12:30:41 +00:00

Addressed in d62c5da5.

🟡 `dhcp` missing from the build assertion

Good catch, and I verified the dependency in the repo before acting on it rather than taking it on trust — kubernetes/infrastructure/multus/cni-dhcp-daemon.yaml says so explicitly:

The dhcp binary ships from the sibling cni-plugins-installer DaemonSet (containernetworking/plugins v1.5.1, whole tarball extracted). Keep the version pins in step.

So dhcp is a real runtime dependency of this image and was absent from the check. You are right that nothing breaks today, since the whole /plugins directory is copied regardless of what the loop asserts — but that means the "fail loudly if upstream restructures the archive" guarantee covered three of the four binaries this repo actually needs. Upstream dropping dhcp while keeping the others would have built green and surfaced later as a broken DHCP daemon on whichever node rolled first, which is exactly the delayed, node-specific failure the assertion exists to prevent.

Now for p in macvlan static host-local dhcp, with a comment recording why the list is what it is so a future edit does not quietly narrow it again. Confirmed all four are executable in the already-published image, so this tightens the assertion without changing what ships:

macvlan     executable
static      executable
host-local  executable
dhcp        executable

🟢 Nits

Both accurate, no action taken:

The reused base digest is intentional, matching alpine-jq and tofu.
Fair distinction on the "no wget" claim — it is about the installer's own commands, not the image contents. The image is Alpine rather than scratch precisely because the install step needs a shell for cp/mv/chown, so busybox wget is present but unused. Worth being precise about, since "no network" is the security argument here: the guarantee is that the install path makes no network calls, not that the image is incapable of it. If that distinction ever needs to be airtight, a scratch image plus a static busybox would be the way, and that is a bigger change than this PR.

Addressed in `d62c5da5`. ### 🟡 `dhcp` missing from the build assertion Good catch, and I verified the dependency in the repo before acting on it rather than taking it on trust — `kubernetes/infrastructure/multus/cni-dhcp-daemon.yaml` says so explicitly: > The dhcp binary ships from the sibling cni-plugins-installer DaemonSet (containernetworking/plugins v1.5.1, whole tarball extracted). Keep the version pins in step. So `dhcp` is a real runtime dependency of this image and was absent from the check. You are right that nothing breaks today, since the whole `/plugins` directory is copied regardless of what the loop asserts — but that means the "fail loudly if upstream restructures the archive" guarantee covered three of the four binaries this repo actually needs. Upstream dropping `dhcp` while keeping the others would have built green and surfaced later as a broken DHCP daemon on whichever node rolled first, which is exactly the delayed, node-specific failure the assertion exists to prevent. Now `for p in macvlan static host-local dhcp`, with a comment recording *why* the list is what it is so a future edit does not quietly narrow it again. Confirmed all four are executable in the already-published image, so this tightens the assertion without changing what ships: ``` macvlan executable static executable host-local executable dhcp executable ``` ### 🟢 Nits Both accurate, no action taken: - The reused base digest is intentional, matching `alpine-jq` and `tofu`. - Fair distinction on the "no wget" claim — it is about the installer's own commands, not the image contents. The image is Alpine rather than `scratch` precisely because the install step needs a shell for `cp`/`mv`/`chown`, so busybox `wget` is present but unused. Worth being precise about, since "no network" is the security argument here: the guarantee is that the install path makes no network calls, not that the image is incapable of it. If that distinction ever needs to be airtight, a `scratch` image plus a static busybox would be the way, and that is a bigger change than this PR.

review-bot reviewed

2026-07-30 12:32:51 +00:00

review-bot left a comment

🤖 Automated review — model CLI default (complexity: complex), commit d62c5da5a0fafc4e3bcc0872e5bac6541cb9339a.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR replaces the multus cni-plugins-installer DaemonSet's node-side wget/tar fetch of the containernetworking/plugins release tarball (no checksum verification, GitHub on the node-rebuild critical path) with a checksum-verified build-time bake into a new ci/images/cni-plugins image, consumed by the installer as a local file copy. It also switches the install from cp-over-target to a temp-file-plus-rename(2) pattern and drops the "already installed?" skip guard in favor of unconditional copy. I checked the diff against the actual checkout (installer manifest, sibling cni-dhcp-daemon.yaml, the other ci/images/*/Containerfile conventions, and the .woodpecker/build-ci-images.yml pipeline) and did not find any blocking issues.

🟡 Suggestions

kubernetes/infrastructure/multus/cni-plugins-installer.yaml:74 — echo "Installed from $(basename "$SRC") image:" with SRC=/opt/cni-plugins always prints the literal string cni-plugins, not anything image-tag-specific (e.g. the 2026-07-30 tag). It's a harmless log line, not a correctness issue, but if the intent was to record which image version ran, basename on a fixed path can't provide that — consider dropping it or logging the image tag another way if this log is ever used for debugging drift.
The sha256sum -c digest pin (ci/images/cni-plugins/Containerfile:77) is claimed in the PR description to be cross-checked against upstream's published .sha256 asset and validated by a real pipeline run (pipeline 3091, per the PR body). I have no network access in this review environment to independently re-verify the hash against https://github.com/containernetworking/plugins/releases/download/v1.5.1/cni-plugins-linux-amd64-v1.5.1.tgz.sha256, so I can't confirm it myself — but the mechanism is sound (build fails closed on mismatch) and the base-image digest pin (alpine:3.21@sha256:48b03...) matches the one already used in ci/images/alpine-jq/Containerfile, which is a good consistency signal.
kubernetes/infrastructure/multus/cni-plugins-installer.yaml:47 pins the runtime image to gitea.renner.uno/gitea_admin/cni-plugins:2026-07-30, a dated tag matching the ci-ansible/alpine-jq convention used elsewhere in this repo (confirmed against zigbee2mqtt-deployment.yaml:67 and build-ci-images.yml). No imagePullSecrets is set on this DaemonSet, but that matches the existing pattern for other gitea_admin/* images pulled the same way elsewhere in the cluster, so this isn't a regression introduced by this PR.

🟢 Nits

ci/images/cni-plugins/Containerfile:103 — the required-plugin list (macvlan static host-local dhcp) is a reasonable "fail loud if upstream restructures" gate, and correctly includes dhcp (verified: cni-dhcp-daemon.yaml:88 execs /opt/cni/bin/dhcp directly, so a silently-missing dhcp binary would indeed only surface later as a broken DHCP daemon, exactly as the comment says).

Tests

There's no automated test harness for these shell-script installers/Containerfiles in this repo (consistent with the other ci/images/* entries, none of which carry tests). The PR body documents manual verification: a deliberately-wrong-digest build failing closed, a real kaniko build/push (pipeline 3091), and a rendered install-script dry run (fresh install, idempotent re-run, pre-seeded-ownership repair path). That's reasonable coverage for infra scripts of this kind given the project's existing conventions; I could not independently re-run any of it in this read-only review environment (no cluster/registry access).

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `d62c5da5a0fafc4e3bcc0872e5bac6541cb9339a`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR replaces the multus `cni-plugins-installer` DaemonSet's node-side `wget`/`tar` fetch of the containernetworking/plugins release tarball (no checksum verification, GitHub on the node-rebuild critical path) with a checksum-verified build-time bake into a new `ci/images/cni-plugins` image, consumed by the installer as a local file copy. It also switches the install from `cp`-over-target to a temp-file-plus-`rename(2)` pattern and drops the "already installed?" skip guard in favor of unconditional copy. I checked the diff against the actual checkout (installer manifest, sibling `cni-dhcp-daemon.yaml`, the other `ci/images/*/Containerfile` conventions, and the `.woodpecker/build-ci-images.yml` pipeline) and did not find any blocking issues. ## 🟡 Suggestions - `kubernetes/infrastructure/multus/cni-plugins-installer.yaml:74` — `echo "Installed from $(basename "$SRC") image:"` with `SRC=/opt/cni-plugins` always prints the literal string `cni-plugins`, not anything image-tag-specific (e.g. the `2026-07-30` tag). It's a harmless log line, not a correctness issue, but if the intent was to record which image version ran, `basename` on a fixed path can't provide that — consider dropping it or logging the image tag another way if this log is ever used for debugging drift. - The `sha256sum -c` digest pin (`ci/images/cni-plugins/Containerfile:77`) is claimed in the PR description to be cross-checked against upstream's published `.sha256` asset and validated by a real pipeline run (pipeline 3091, per the PR body). I have no network access in this review environment to independently re-verify the hash against `https://github.com/containernetworking/plugins/releases/download/v1.5.1/cni-plugins-linux-amd64-v1.5.1.tgz.sha256`, so I can't confirm it myself — but the mechanism is sound (build fails closed on mismatch) and the base-image digest pin (`alpine:3.21@sha256:48b03...`) matches the one already used in `ci/images/alpine-jq/Containerfile`, which is a good consistency signal. - `kubernetes/infrastructure/multus/cni-plugins-installer.yaml:47` pins the runtime image to `gitea.renner.uno/gitea_admin/cni-plugins:2026-07-30`, a dated tag matching the `ci-ansible`/`alpine-jq` convention used elsewhere in this repo (confirmed against `zigbee2mqtt-deployment.yaml:67` and `build-ci-images.yml`). No `imagePullSecrets` is set on this DaemonSet, but that matches the existing pattern for other `gitea_admin/*` images pulled the same way elsewhere in the cluster, so this isn't a regression introduced by this PR. ## 🟢 Nits - `ci/images/cni-plugins/Containerfile:103` — the required-plugin list (`macvlan static host-local dhcp`) is a reasonable "fail loud if upstream restructures" gate, and correctly includes `dhcp` (verified: `cni-dhcp-daemon.yaml:88` execs `/opt/cni/bin/dhcp` directly, so a silently-missing `dhcp` binary would indeed only surface later as a broken DHCP daemon, exactly as the comment says). ## Tests There's no automated test harness for these shell-script installers/Containerfiles in this repo (consistent with the other `ci/images/*` entries, none of which carry tests). The PR body documents manual verification: a deliberately-wrong-digest build failing closed, a real kaniko build/push (pipeline 3091), and a rendered install-script dry run (fresh install, idempotent re-run, pre-seeded-ownership repair path). That's reasonable coverage for infra scripts of this kind given the project's existing conventions; I could not independently re-run any of it in this read-only review environment (no cluster/registry access). ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

k.renner merged commit 9c9ac712ca into master

2026-07-30 12:33:49 +00:00

k.renner referenced this issue from a commit

2026-07-30 12:33:50 +00:00

Merge pull request 'fix(multus): bake CNI plugins into an image, drop the node-side fetch' (#447) from cni-plugins-image into master

claude-bot referenced this pull request

2026-07-30 12:55:49 +00:00

fix(mosquitto): drop runtime apk add from the 7 bootstrap Jobs #449

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#447

fix(multus): bake CNI plugins into an image, drop the node-side fetch #447

The actual problems

Fix

Install script changes beyond dropping the fetch

chown 0:0 kept, but re-framed as a repair

Verification

Things to weigh

Summary

🟡 Suggestions

🟢 Nits

Tests

Recommendation

🟡 dhcp missing from the build assertion

🟢 Nits

Summary

🟡 Suggestions

🟢 Nits

Tests

Recommendation

`chown 0:0` kept, but re-framed as a repair

🟡 `dhcp` missing from the build assertion