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

2026-07-30T11:47:34Z

claude-bot commented

2026-07-30 11:47:34 +00:00

First of the runtime apk add cleanups found by sweeping the repo. Starting here because it's the highest blast radius: a DaemonSet, so it runs on every node.

No image needed

apk add --no-cache curl tar turns out to have been unnecessary from the start. Verified in alpine:3.21:

tool	status
`tar`	already present — busybox v1.37.0, handles `-xzf`
`wget`	already present — follows GitHub's redirect to `objects.githubusercontent.com` over TLS
CA bundle	`/etc/ssl/certs/ca-certificates.crt` ships in the base
`curl`	genuinely absent — the only real gap, and `wget` covers this use

So this replaces the install with wget -q -O rather than introducing another layered image. Fewer moving parts than the alpine-jq route, and nothing new to build or bump.

Severity, stated honestly

The apk add sat inside the else branch, so steady-state nodes never reached it. It ran only when plugins were absent — a fresh node, or after /opt/cni/bin was wiped. That's not "rarely matters", it's the opposite: it's precisely the moment you least want CNI plugin installation to depend on package-index availability, since it's during a node build or rebuild.

Verification

Ran the rendered init script in alpine:3.21 against the real release URL, exercising both paths:

### RUN 1: empty dir
Fetching https://github.com/containernetworking/plugins/releases/download/v1.5.1/...
Installed: -rwxr-xr-x  host-local
           -rwxr-xr-x  macvlan
           -rwxr-xr-x  static
exit=0
macvlan: INSTALLED
owner of bin dir: 0:0

### RUN 2: already installed
CNI plugins already installed (CNI macvlan plugin v1.5.1)
exit=0

Two things worth pointing out in that output:

The v1.5.1 string in run 2 comes from executing the extracted binary, so the download and extraction are confirmed functionally good, not merely present on disk.
owner of bin dir: 0:0 confirms the chown 0:0 still applies. The existing comment explains why that's load-bearing: without it, Cilium's apply-sysctl-overwrites init container fails with "Permission denied" because the tarball's ./ entry carries uid/gid 1001/127 from the GitHub Actions runner that built the release.

Also kustomize build + server-side dry run clean; zero non-comment apk add remaining in the file.

Remaining from the sweep

Two runtime cases left, which I'd keep as separate PRs:

kubernetes/apps/paperless/paperless.yaml:97 — apk add openssl, Deployment init container, every pod start
kubernetes/apps/home-assistant/mosquitto-bootstrap.yaml — apk add curl jq, 7 near-identical Job blocks (deserves its own review pass)

Build-time apk add in the five Containerfiles is the sanctioned pattern and untouched. kubernetes/apps/woodpecker/MIGRATION.md:63 documents a one-off manual kubectl exec — a human action, not a workload, so I left it.

🤖 Generated with Claude Code

First of the runtime `apk add` cleanups found by sweeping the repo. Starting here because it's the **highest blast radius**: a `DaemonSet`, so it runs on every node. ## No image needed `apk add --no-cache curl tar` turns out to have been unnecessary from the start. Verified in `alpine:3.21`: | tool | status | |---|---| | `tar` | already present — **busybox v1.37.0**, handles `-xzf` | | `wget` | already present — follows GitHub's redirect to `objects.githubusercontent.com` over TLS | | CA bundle | `/etc/ssl/certs/ca-certificates.crt` ships in the base | | `curl` | genuinely **absent** — the only real gap, and `wget` covers this use | So this replaces the install with `wget -q -O` rather than introducing another layered image. Fewer moving parts than the `alpine-jq` route, and nothing new to build or bump. ## Severity, stated honestly The `apk add` sat **inside the else branch**, so steady-state nodes never reached it. It ran only when plugins were absent — a fresh node, or after `/opt/cni/bin` was wiped. That's not "rarely matters", it's the opposite: it's precisely the moment you least want CNI plugin installation to depend on package-index availability, since it's during a node build or rebuild. ## Verification Ran the **rendered** init script in `alpine:3.21` against the real release URL, exercising both paths: ``` ### RUN 1: empty dir Fetching https://github.com/containernetworking/plugins/releases/download/v1.5.1/... Installed: -rwxr-xr-x host-local -rwxr-xr-x macvlan -rwxr-xr-x static exit=0 macvlan: INSTALLED owner of bin dir: 0:0 ### RUN 2: already installed CNI plugins already installed (CNI macvlan plugin v1.5.1) exit=0 ``` Two things worth pointing out in that output: - The `v1.5.1` string in run 2 comes from **executing the extracted binary**, so the download and extraction are confirmed functionally good, not merely present on disk. - `owner of bin dir: 0:0` confirms the `chown 0:0` still applies. The existing comment explains why that's load-bearing: without it, Cilium's `apply-sysctl-overwrites` init container fails with "Permission denied" because the tarball's `./` entry carries uid/gid 1001/127 from the GitHub Actions runner that built the release. Also `kustomize build` + server-side dry run clean; zero non-comment `apk add` remaining in the file. ## Remaining from the sweep Two runtime cases left, which I'd keep as separate PRs: - `kubernetes/apps/paperless/paperless.yaml:97` — `apk add openssl`, Deployment init container, **every pod start** - `kubernetes/apps/home-assistant/mosquitto-bootstrap.yaml` — `apk add curl jq`, **7 near-identical Job blocks** (deserves its own review pass) Build-time `apk add` in the five `Containerfile`s is the sanctioned pattern and untouched. `kubernetes/apps/woodpecker/MIGRATION.md:63` documents a one-off manual `kubectl exec` — a human action, not a workload, so I left it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-07-30 11:47:34 +00:00

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

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

Details

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

Details

401d23ac48

Owner directive: tools belong in the image, not installed at container
start. This is the highest-blast-radius instance of the pattern left in the
repo — a DaemonSet, so it runs on every node.

No image was needed. `apk add --no-cache curl tar` turns out to have been
unnecessary from the start: verified in alpine:3.21 that `tar` is already
busybox (v1.37.0) and handles -xzf, and that busybox `wget` follows
GitHub's release redirect to objects.githubusercontent.com over TLS using
the base image's /etc/ssl/certs/ca-certificates.crt. Only `curl` is
genuinely absent, and wget covers what it was used for. So this replaces
the install with `wget -q -O` rather than introducing another layered
image.

Severity note for the record: the apk add sat inside the else branch, so
steady-state nodes never reached it. It ran when plugins were absent — i.e.
on a fresh node or after /opt/cni/bin was wiped, which is precisely the
moment you least want a dependency on package-index availability.

Verified by running the RENDERED init script in alpine:3.21 against the
real release URL, both paths:

  empty dir     -> Fetching ...; Installed: host-local, macvlan, static
                   macvlan executable, /opt/cni/bin owner 0:0, exit 0
  already there -> "CNI plugins already installed (CNI macvlan plugin
                   v1.5.1)", exit 0, no fetch

The second line is worth noting: that version string comes from executing
the extracted binary, so the download and extraction are confirmed good,
not merely present. The chown 0:0 also still applies, which the existing
comment explains is required or Cilium's apply-sysctl-overwrites init
container fails with "Permission denied".

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

review-bot reviewed

2026-07-30 11:50:11 +00:00

review-bot left a comment

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

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

Summary

Replaces the runtime apk add --no-cache curl tar + curl -fsSL fetch in the cni-plugins-installer DaemonSet's init container with a plain wget -q -O, since tar and wget are already present via BusyBox in alpine:3.21 and curl was the only genuinely missing tool. The rest of the script (extraction, chown 0:0 restore) is untouched. This is a small, well-scoped change and I don't see a blocking issue — the PR body documents concrete verification (an actual run against the real GitHub release URL in alpine:3.21, both the fresh-install and already-installed paths) that backs up the substitution.

🟡 Suggestions

kubernetes/infrastructure/multus/cni-plugins-installer.yaml:70 — wget -q -O ... has no timeout/retry, so if GitHub is reachable but slow/hanging, the init container blocks indefinitely and the node never finishes booting. This isn't a regression (curl -fsSL had the same gap — no --max-time), but since this code path only fires exactly during node build/rebuild — the case the PR is trying to make more resilient — a --timeout=<n> on wget (or --connect-timeout/--max-time if this ever moves back to curl) would close that gap while it's already being touched.
kubernetes/infrastructure/multus/cni-plugins-installer.yaml:70 — curl -fsSL used -S to force error output even under -s (silent). The new wget -q drops that distinction; BusyBox's wget generally still prints fatal errors (e.g. bad address, HTTP error) to stderr even with -q since those aren't the progress-meter output -q suppresses, but I haven't independently confirmed that against the installed BusyBox build. Worth a quick check that a deliberately-broken URL still produces a visible error in kubectl logs for the init container, since this is the only diagnostic signal available when the DaemonSet is stuck across nodes.

Tests

No automated test exists for this (it's a static DaemonSet manifest, consistent with the rest of kubernetes/infrastructure/multus/). The PR body substitutes manual verification — running the rendered script in alpine:3.21 against the real release URL for both the fresh-install and already-installed branches, and executing the extracted binary to confirm it's functional — which is reasonable given the repo has no manifest-testing harness. I did not independently re-run this verification, only read the diff and surrounding file; the manual evidence in the PR description is consistent with what the code does.

Recommendation

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

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `401d23ac480393778d8b2a7956d6d212a6935610`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Replaces the runtime `apk add --no-cache curl tar` + `curl -fsSL` fetch in the `cni-plugins-installer` DaemonSet's init container with a plain `wget -q -O`, since `tar` and `wget` are already present via BusyBox in `alpine:3.21` and `curl` was the only genuinely missing tool. The rest of the script (extraction, `chown 0:0` restore) is untouched. This is a small, well-scoped change and I don't see a blocking issue — the PR body documents concrete verification (an actual run against the real GitHub release URL in `alpine:3.21`, both the fresh-install and already-installed paths) that backs up the substitution. ## 🟡 Suggestions - `kubernetes/infrastructure/multus/cni-plugins-installer.yaml:70` — `wget -q -O ...` has no timeout/retry, so if GitHub is reachable but slow/hanging, the init container blocks indefinitely and the node never finishes booting. This isn't a regression (`curl -fsSL` had the same gap — no `--max-time`), but since this code path only fires exactly during node build/rebuild — the case the PR is trying to make more resilient — a `--timeout=<n>` on `wget` (or `--connect-timeout`/`--max-time` if this ever moves back to curl) would close that gap while it's already being touched. - `kubernetes/infrastructure/multus/cni-plugins-installer.yaml:70` — `curl -fsSL` used `-S` to force error output even under `-s` (silent). The new `wget -q` drops that distinction; BusyBox's `wget` generally still prints fatal errors (e.g. bad address, HTTP error) to stderr even with `-q` since those aren't the progress-meter output `-q` suppresses, but I haven't independently confirmed that against the installed BusyBox build. Worth a quick check that a deliberately-broken URL still produces a visible error in `kubectl logs` for the init container, since this is the only diagnostic signal available when the DaemonSet is stuck across nodes. ## Tests No automated test exists for this (it's a static DaemonSet manifest, consistent with the rest of `kubernetes/infrastructure/multus/`). The PR body substitutes manual verification — running the rendered script in `alpine:3.21` against the real release URL for both the fresh-install and already-installed branches, and executing the extracted binary to confirm it's functional — which is reasonable given the repo has no manifest-testing harness. I did not independently re-run this verification, only read the diff and surrounding file; the manual evidence in the PR description is consistent with what the code does. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

claude-bot referenced this issue from a commit

2026-07-30 12:15:56 +00:00

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

claude-bot referenced this pull request

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

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

claude-bot commented

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

Superseded by #447 — closing.

This PR removed the apk add but left the two things that actually matter untouched:

No integrity verification. The tarball was fetched and extracted into /opt/cni/bin with no checksum — a directory executed as root in the CNI path on every node. Swapping curl for wget does nothing about that.
github.com on the node-rebuild critical path. Also unchanged by this PR.

#447 bakes the binaries into ci/images/cni-plugins with a build-time sha256 (cross-checked against upstream's published .sha256), leaving the installer as a local copy with zero network calls.

Superseded by #447 — closing. This PR removed the `apk add` but left the two things that actually matter untouched: 1. **No integrity verification.** The tarball was fetched and extracted into `/opt/cni/bin` with no checksum — a directory executed as root in the CNI path on every node. Swapping `curl` for `wget` does nothing about that. 2. **github.com on the node-rebuild critical path.** Also unchanged by this PR. #447 bakes the binaries into `ci/images/cni-plugins` with a build-time sha256 (cross-checked against upstream's published `.sha256`), leaving the installer as a local copy with zero network calls.

claude-bot closed this pull request

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

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#446