Skip to content

feat(multus)!: stage reference CNI plugins from the multus image - #3195

Merged
Aleksei Sviridkin (lexfrei) merged 3 commits into
mainfrom
feat/multus-stage-reference-cni-plugins
Aug 6, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 3 commits into
mainfrom
feat/multus-stage-reference-cni-plugins

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Jul 4, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (bridge, macvlan, ipvlan, ...), and nothing guarantees a node has them where Multus looks. On k3s they live under /var/lib/rancher/k3s/data/cni while Multus and Cilium install into /opt/cni/bin, which is left without a bridge binary. A bridge-type NAD then fails with failed to find plugin "bridge" in path [/opt/cni/bin], and today you work around it by copying the binary in by hand. #3196 documents one such pattern (attaching a VM to an external VLAN) that this unblocks.

This bundles 14 of the reference plugins into the multus-cni image and adds an install-cni-plugins init container that installs them into the host /opt/cni/bin, mirroring the existing install-multus-binary init container.

Where it applies, and how to turn it off

The new networking.stageCniPlugins value defaults to on for both bundles. Talos puts six binaries in that directory -- bridge, firewall, host-local, loopback and portmap stripped from the same upstream release, plus flannel, which is a different project -- so the other ten are missing there exactly as on generic Linux and a NAD naming macvlan, ipvlan or vlan fails on either platform. Four of those six are names this change stages; loopback is deliberately not staged and flannel was never in the reference set. The four names that overlap are rewritten on every Multus pod recreation with the same upstream version -- Talos pins containernetworking/plugins v1.9.1 in siderolabs/pkgs, and so does this image -- so it is not a downgrade of a system component. A marker in the image Dockerfile records the Talos release that pin was compared against, and a test holds it to the release the platform ships. isp-hosted ships no multus at all and is unaffected. Both defaults can be overridden either way.

If you run a generic-Linux cluster and manage /opt/cni/bin yourself, this changes behaviour on upgrade. Multus starts installing 14 plugins into that directory every time its pod is recreated, replacing files of the same name, so a hand-placed or locally patched plugin there will not survive. Set networking.stageCniPlugins: false to decline, and set it before the upgrade that first enables staging: nothing here removes or restores a plugin, so declining afterwards protects what is still yours and does not give back what was already replaced. Changing the value edits the Multus DaemonSet's pod template, so Multus rolls node by node, and while a node's daemon is down pod creation and deletion on that node fail for the length of the restart. The four plugins the package never touches are loopback, dhcp, dummy and tap.

Where staging runs it is unconditional, not only-when-absent, because nothing here ever revisits a plugin it has written. Installing only when absent would pin each node to whichever version arrived first, and a later CNI_PLUGINS_VERSION bump, including one fixing a binary that runs as root on every node, would reach zero existing nodes. The directory is not ours alone and the decision does not assume it is: kube-ovn copies loopback, portmap and macvlan there from its own image, two of which this change also writes, node provisioning may place the set, and /opt/cni/bin is also the CNI bin directory on RKE2. Overwriting is what delivering a version requires, and networking.stageCniPlugins: false is how an operator who owns those files declines it.

Notes

  • The manifest change lives in patches/customize-deployment.patch so it survives make update, and the release rewrite in that target runs after the patch rather than before it, so the line the patch adds lands on the same reference as the two it already had. The patch seeds that line with the upstream image reference while the committed template carries the pinned cozystack digest, because the package's image: target rewrites every multus-cni image line to one digest. So it does not reverse-apply against the committed template. That make update reproduces the tree is verified forward from pristine upstream instead.
  • /opt/cni/bin stays shared where staging is on. kube-ovn installs portmap and macvlan there and has been seen to do so with a plain copy onto the live path, which truncates in place. Each plugin is installed under a temporary name and swapped in with a rename, atomic within the mount, so the kubelet never execs a half-written binary. The Dockerfile records which kube-ovn release the version pin was checked against, and hack/cni-plugins-staging-contract.bats fails when that marker and the vendored release part ways. That check fires on the next kube-ovn bump and compares the marker with the vendored release; it does not compare the two CNI_PLUGINS_VERSION pins. Moving the marker is the acknowledgement that someone looked -- so read kube-ovn's dist/images/Dockerfile.base before moving it, because a silent divergence there makes portmap and macvlan flap between the two DaemonSets on every pod recreation.
  • The init container runs unprivileged, with capabilities dropped and no mount propagation. Writing into a hostPath needs root inside the container and nothing on the host, which is how cilium's install-cni-binaries copies plugin binaries into the same directory on the same nodes. Bidirectional propagation governs mounts made inside a container, this script makes none, and asking for it is what would force privileged. The tests pin the absence.
  • loopback is excluded because replacing it is what a node cannot survive: the runtime calls it for every sandbox, and Cilium and kube-ovn already install it. dhcp needs a host daemon this package does not run. tap and dummy have no consumer here. portmap is included because a NAD conflist may chain it for hostPort.
  • The plugin version is pinned via ARG CNI_PLUGINS_VERSION (v1.9.1) and the release tarball is verified against a per-arch SHA-256 before extraction. These are root-executed node binaries, so a tag alone is not enough. An unknown or empty TARGETARCH fails the build.
  • The init container skips staging when the image has no /cni-plugins, rather than crashlooping the DaemonSet node-wide. The manifest and the image digest are re-pinned on different schedules, so a tree taken between a Dockerfile change and the next release bake can carry one without the other. A plugin it cannot copy is reported on stderr and skipped, for the same reason. multusConfigFile: auto makes 00-multus.conf the primary CNI config and there is no cleanupConfigOnExit, so on a node where Multus has already run, an init container that never completes leaves the shim's socket gone with that config still primary, and the node stops scheduling anything that goes through CNI; host-network pods still start. Failing on a copy error would therefore leave the node worse off than before this change, which only ever left it without one plugin. The cost of continuing is that the pod still reports Ready, so the summary is also written to /dev/termination-log, which the kubelet surfaces in kubectl describe pod regardless of the exit code.
  • The bundled plugins add roughly 67 MiB uncompressed per architecture to an image pulled by every node, including installs where staging is off. Measured by extracting the staged set from the v1.9.1 release tarball.

Testing

packages/core/platform/tests/bundles_multus_staging_test.yaml pins the delivery point: both variant defaults, both explicit overrides, and that multus is still shipped in each bundle at all. That last one because multus is now included by hand in two branches rather than by the shared helper.

hack/multus-install-cni-plugins.bats extracts the init container's script from the rendered DaemonSet and runs it. It pins that every plugin lands executable, that a replacement swaps the inode rather than truncating in place, that temp files never leak, that a missing or empty directory skips instead of failing, and that a plugin which cannot be installed is reported on stderr and skipped rather than failing the container. Both halves of that last one are asserted, because exit 0 alone would still pass with the report deleted.

hack/cni-plugins-staging-contract.bats covers what the rendered manifest cannot show: that the Dockerfile stages the intended plugins and only those, copies them into the final stage, and that nothing there removes them again, that the tarball is verified before extraction, and that the version marker matches the vendored kube-ovn release. helm-unittest pins both states of the value, including off, which is what most installs run, plus the quoted-string forms a values file can produce. An explicit null is covered too: it is how a values file clears the key, and the chart folds it onto its own default rather than refusing it as an unreadable spelling.

The feature is inert until the image is re-baked. All three image: lines still point at the pre-change digest, which has no /cni-plugins, so on main after merge a generic install renders the init container, logs no /cni-plugins in this image; skipping staging, and installs nothing until release-prep re-pins. That is the intended flow (first-party digests are release-managed, not hand-edited), and the guide documents the log line, but anyone testing this on main will meet it first and the pod will look healthy.

No CI job stages plugins on a node. CI boots Talos, where the value is off, and isp-full-generic appears in no e2e workflow. The mechanics are covered by the bats above, which run the real script from the real manifest and also under dash, the shell in the runtime image, but nothing demonstrates it end to end on a node.

Defaulting it on for the generic bundle is a deliberate maintainer decision taken with that gap known, not a side effect of the diff. Shipping it off would leave the bug in place on the only platform where the feature does anything, and would not add coverage either, since the code stays unexecuted in CI both ways. What makes the risk acceptable: the init container skips rather than crashlooping when the image carries no plugins, each plugin is swapped in with an atomic rename instead of being written onto the live path, the tarball is checksummed before extraction, and declining is a value rather than disabling the package. And a plugin it cannot install is reported and skipped rather than failing the pod, so the worst case leaves the node exactly as it was before this change rather than unable to schedule.

Screenshots

N/A, no UI changes.

Release note

feat(multus): install the reference CNI plugins (bridge, macvlan, ipvlan, ...) into /opt/cni/bin on generic-Linux installs, where nodes do not otherwise provide them. On by default on every platform: Talos ships five of the fourteen, and the four that overlap are rewritten with the same upstream version. On generic clusters this replaces same-named files in /opt/cni/bin every time the Multus pod is recreated, so a locally managed plugin there will not survive. Set networking.stageCniPlugins: false to decline.

BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI plugins in /opt/cni/bin on existing nodes, Talos included -- four of the names it writes are ones the Talos node image also carries. Opt out before upgrading: declining afterwards does not restore what was replaced.

Summary by CodeRabbit

  • New Features

    • Multus can now stage reference CNI plugins into /opt/cni/bin, adding missing plugins and safely replacing matching versions.
    • Added the stageCniPlugins setting, with platform-specific defaults and explicit opt-out support.
    • Invalid configuration values now produce clear rendering errors.
    • Improved plugin installation diagnostics and failure reporting.
  • Documentation

    • Expanded guidance for CNI prerequisites, plugin staging behavior, troubleshooting, rollout impact, and configuration.
    • Clarified Multus image reference handling in platform documentation.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The Multus CNI image now bundles verified reference CNI plugins. Platform bundles configure when staging runs. A DaemonSet init container installs plugins on hosts with atomic replacement and failure reporting. Tests and documentation cover the new behavior.

Changes

Reference CNI plugin staging and installation

Layer / File(s) Summary
Package verified CNI plugins
packages/system/multus/images/multus-cni/Dockerfile, packages/system/multus/Makefile
Downloads architecture-specific plugins, verifies checksums, extracts selected binaries and licenses, and includes them in the runtime image.
Configure platform staging
packages/core/platform/templates/_helpers.tpl, packages/core/platform/templates/bundles/system.yaml, packages/core/platform/values.yaml, packages/system/multus/values.yaml
Adds validated stageCniPlugins handling with variant-specific defaults and explicit opt-out support.
Install plugins from the DaemonSet
packages/system/multus/templates/multus-daemonset-thick.yml, packages/system/multus/patches/customize-deployment.patch
Adds the optional installer init container. It copies plugins through temporary paths, atomically renames them into the host CNI directory, reports failures, and applies the configured security and resource settings.
Validate and document the staging contract
packages/system/multus/tests/multus_test.yaml, packages/core/platform/tests/*, hack/*multus*, hack/cni-plugins-staging-contract.bats, docs/vm-external-vlan.md, docs/agents/image-refs.md, hack/lib/image-refs.sh
Adds rendering, image, installation, patch-alignment, dependency, image-reference, and documentation consistency tests. Documents plugin staging and image-reference handling.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PlatformValues
  participant MultusDaemonSet
  participant MultusImage
  participant InstallCniPlugins
  participant HostCniDirectory
  PlatformValues->>MultusDaemonSet: Set validated stageCniPlugins
  MultusImage->>InstallCniPlugins: Provide /cni-plugins
  MultusDaemonSet->>InstallCniPlugins: Run conditional init container
  InstallCniPlugins->>HostCniDirectory: Atomically publish staged plugins
Loading

Suggested labels: area/kubernetes, area/platform

Suggested reviewers: sircthulhu, lllamnyp, kvaps

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: staging reference CNI plugins from the Multus image.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/multus-stage-reference-cni-plugins

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/feature Categorizes issue or PR as related to a new feature size/M This PR changes 30-99 lines, ignoring generated files labels Jul 4, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) marked this pull request as ready for review July 4, 2026 11:45
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the Multus CNI deployment by ensuring essential reference plugins are available on the host system. By bundling these plugins directly into the Multus image and deploying an init container to stage them into the host's CNI binary directory, the change resolves runtime failures where NetworkAttachmentDefinitions would otherwise fail due to missing dependencies in environments like k3s.

Highlights

  • CNI Plugin Bundling: Bundled a curated set of upstream containernetworking reference plugins into the multus-cni image.
  • Init Container Addition: Added an 'install-cni-plugins' init container to the Multus DaemonSet to automatically copy these plugins into the host's /opt/cni/bin directory.
  • Compatibility Improvement: Ensures that Multus-backed NetworkAttachmentDefinitions function correctly on nodes (like k3s) that do not provide the default reference plugins.
  • Test Coverage: Added a helm-unittest to verify the init container configuration and ensure it uses the correct image.
New Features

🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Ignored Files
  • Ignored by pattern: **/*.patch (1)
    • packages/system/multus/patches/customize-deployment.patch
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment Gemini (@gemini-code-assist) Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@dosubot dosubot Bot added the area/networking Issues or PRs related to networking (ingress, gateway, vpn, metallb, kube-ovn) label Jul 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request packages and stages upstream reference CNI plugins (such as bridge, macvlan, etc.) into the Multus-CNI image and adds an init container to copy them to the host's /opt/cni/bin directory. This ensures that Multus-backed NetworkAttachmentDefinitions function correctly on distributions like k3s. The review feedback highlights that using cp -f can cause Text file busy errors if the binaries are in use, suggesting the use of install instead. Additionally, it recommends dropping privileged: true on the init container in favor of a more restricted security context, and updating the corresponding test assertions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +225 to +228
command:
- "sh"
- "-c"
- "cp -f /cni-plugins/* /host/opt/cni/bin/"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using cp -f to copy binaries to /host/opt/cni/bin/ can fail with a Text file busy (ETXTBSY) error if any of the CNI plugins (like bridge or loopback) are currently being executed or held open by the container runtime or another process on the host.

To prevent this, use the install utility instead of cp. The install command unlinks the destination file before copying, which avoids the Text file busy error and allows running processes to continue using the old inode safely.

Since the image is based on debian:stable-slim, install is guaranteed to be available.

          command:
            - "sh"
            - "-c"
            - "install -t /host/opt/cni/bin/ /cni-plugins/*"

Comment on lines +233 to +234
securityContext:
privileged: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The install-cni-plugins container does not require privileged: true to copy files to the hostPath volume. Running with full privileges is a security risk.

Instead, you can run without privileged: true and apply a more restricted security context. Since it only needs to write to the hostPath volume (which requires root permissions if /opt/cni/bin is owned by root), you can drop all capabilities and prevent privilege escalation.

          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
                - ALL
            readOnlyRootFilesystem: true
References
  1. Flag overly broad RBAC, missing securityContext, containers running as root without an explicit reason, and hostPath/hostNetwork usage without clear rationale. (link)

Comment on lines +48 to +53
path: spec.template.spec.initContainers[1].command
value:
- "sh"
- "-c"
- "cp -f /cni-plugins/* /host/opt/cni/bin/"
- equal:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

Update the test assertion to match the recommended install command to ensure the helm-unittest suite passes.

      - equal:
          path: spec.template.spec.initContainers[1].command
          value:
            - "sh"
            - "-c"
            - "install -t /host/opt/cni/bin/ /cni-plugins/*"

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with non-blocking notes

Vendored-chart discipline is correct (patches/customize-deployment.patch +
make update, make image rewrites all three image refs), unit test is green,
Phase 5b (upgrade = rolling DaemonSet, fresh install = digest-pinned image) is fine.
Two non-blocking notes:

[MINOR] Unconditional cp -f silently overwrites host CNI plugins

packages/system/multus/templates/multus-daemonset-thick.yml:75-91

cp -f /cni-plugins/* /host/opt/cni/bin/ runs on every (re)start. If k3s or another
actor already placed a differently-versioned bridge/portmap/loopback there, it
is silently overwritten by the containernetworking v1.9.1 build (last-writer-wins).
Plugins are ABI-stable so risk is low, but the overwrite is silent and repeats.
Consider cp -n, or document that last-writer-wins is intentional.

[MINOR] CNI plugins tarball fetched without checksum

packages/system/multus/images/multus-cni/Dockerfile:20-24

The tarball is pulled via curl | tar -xz with no sha256 verification, baking an
unverified third-party binary into a privileged host-writing image. Not a regression
(the existing multus download is the same), but consider pinning the tarball digest.

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Jul 8, 2026
## What this PR does

Adds `docs/vm-external-vlan.md`, a guide for attaching a `vm-instance`
VM directly to an external, physically-routed VLAN via a Linux bridge
and a `bridge`-type NetworkAttachmentDefinition.

The documented VM networking story is overlay-only (pod network +
KubeOVN VPC subnets). Attaching a VM to a real VLAN — so it shares a
broadcast domain with external hardware and takes an address from that
VLAN's subnet — is undocumented, and the obvious `macvlan` approach
silently fails with KubeVirt's default bridge binding: macvlan
demultiplexes ingress by the child interface's MAC while bridge binding
puts the guest's own MAC on the wire, so gateway replies reach the
parent interface and are dropped before the guest. The working recipe is
a Linux bridge + `bridge` CNI NAD.

The guide covers why bridge and not macvlan, the host bridge, the NAD
(one per tenant namespace), assigning a static guest address through
cloud-init (the chart has no `networkData`), the always-present
dual-homed pod NIC, and the post-recreate ARP staleness from KubeVirt
regenerating the guest MAC.

Docs-only; no chart or manifest changes. The recipe depends on the
`bridge` CNI plugin being present in `/opt/cni/bin`; #3195 makes
Cozystack's multus package stage the reference plugins there.

### Screenshots

N/A — no UI changes.

### Release note

```release-note
docs(vm-instance): add a guide for attaching a VM to an external VLAN via a Linux bridge and a bridge-type NetworkAttachmentDefinition
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added a new guide for connecting a `vm-instance` directly to an
externally routed VLAN.
* Includes setup steps for creating a bridged VLAN interface, defining a
network attachment, and configuring the VM with a static IP.
* Notes important networking caveats such as dual-homing behavior, MAC
changes after recreation, and host-to-VM connectivity requirements.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

@myasnikovdaniil myasnikovdaniil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall — approve-level with a few non-blocking notes below. The motivation (bridge-type NADs failing on nodes that don't populate /opt/cni/bin, e.g. k3s) is real and well-documented, and the implementation follows the repo's conventions correctly: the manifest change lives in patches/customize-deployment.patch so it survives make update, and there's a focused helm-unittest pinning the container name/command/mount and the image repository.

I verified the parts that looked risky:

  • The reused @sha256:e006fe… digest (the same one install-multus-binary carries) is the pre-existing, plugin-less image — but that's a build-time placeholder, not what ships. make image re-stamps every multus-cni ref via sed "s|image: .*multus-cni.*|…@$DIGEST|g", so the PR build (fragment → finalize → installer OCI artifact consumed by e2e) and the release build (make build → Prepare release commit) both carry the freshly-built, plugin-carrying digest. The committed digest never reaches a cluster as-is. Only a hypothetical promote-without-rebuild that reused a pre-this-change digest could regress, which isn't the current flow.
  • e2e gives real coverage: cozystack always installs multus, so the install-cni-plugins init container actually runs cp -f /cni-plugins/* against the freshly-built image during e2e — a crashloop (empty/missing /cni-plugins) would fail the install, not just the unittest.
  • cni-plugins v1.9.1 exists and the asset names match the URL template exactly for both amd64 and arm64; all 15 extracted members are present in the tarball (excluded dhcp/dummy/tap are a deliberate curation). A typo'd member name would fail tar loudly at build.

Nothing blocking. Say the word if you'd like me to convert this to a formal Approve.

ARG TARGETARCH
ARG CNI_PLUGINS_VERSION=v1.9.1
RUN mkdir -p /cni-plugins && \
curl --fail --silent --show-error --location "https://github.com/containernetworking/plugins/releases/download/${CNI_PLUGINS_VERSION}/cni-plugins-linux-${TARGETARCH}-${CNI_PLUGINS_VERSION}.tgz" | \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking): the tarball is pinned by version tag but not checksum/digest-verified. This is consistent with the multus source fetch above (curl -sSL … | tar at line 9), so it's not a regression — just noting it for supply-chain hardening if you ever want to add a sha256sum -c. --fail correctly turns a 404 / error page into a loud build failure instead of feeding garbage into tar, which is the important part here. 👍

RUN mkdir -p /cni-plugins && \
curl --fail --silent --show-error --location "https://github.com/containernetworking/plugins/releases/download/${CNI_PLUGINS_VERSION}/cni-plugins-linux-${TARGETARCH}-${CNI_PLUGINS_VERSION}.tgz" | \
tar -xz -C /cni-plugins \
./bridge ./host-local ./loopback ./static ./macvlan ./ipvlan ./vlan \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads-up on loopback: it's bundled here and later cp -f'd unconditionally, and Cilium also installs its own loopback. Identical today (both are the canonical reference build), but the init container re-runs on every multus pod (re)start, so it will silently pin the node's loopback to reference v1.9.1 even if Cilium later ships a different one. Low risk given how stable loopback is — flagging the version coupling, no change required.

command:
- "sh"
- "-c"
- "cp -f /cni-plugins/* /host/opt/cni/bin/"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glob makes this all-or-nothing: if /cni-plugins is ever absent/empty (a stale image, or a future make update regenerating this container onto the upstream image), the glob stays literal, cp exits non-zero, and the init container crashloops — blocking the whole multus pod, not just skipping plugin install. That's arguably the right (loud) behavior given the build always re-stamps to a plugin-carrying digest and e2e exercises it, and the unittest guards the image repo. Just recording the coupling: correctness here depends on the build re-stamping the digest — worth keeping in mind if the release path ever moves toward promote-without-rebuild. No change requested.

IvanHunters
IvanHunters previously approved these changes Jul 15, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Reference CNI plugins are baked into the digest-pinned image and staged via an init container mirroring the existing install-multus-binary pattern; the manifest change lives in patches/customize-deployment.patch so it survives make update, and the helm-unittest guard catches both patch drift and image reset. Verified upgrade (rolling DaemonSet restart only, primary CNI untouched) and fresh install (no new bundle/RBAC/PackageSource).

Non-blocking follow-ups: (1) Dockerfile fetches the cni-plugins tarball without sha256 verification (build-time/CI only; runtime is digest-pinned); (2) the init container cp -fs all plugins into /opt/cni/bin on every restart, so on managed-k8s this could overwrite a provider's newer portmap/bandwidth (tradeoff acknowledged in-code); (3) the test asserts on positional initContainers[1].

@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/multus-stage-reference-cni-plugins branch from 7bbdf8c to 44801b4 Compare July 16, 2026 19:07
@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files and removed size/M This PR changes 30-99 lines, ignoring generated files labels Jul 16, 2026
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/multus-stage-reference-cni-plugins branch from 44801b4 to 0de4de0 Compare July 16, 2026 20:08
@lexfrei Aleksei Sviridkin (lexfrei) removed the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Jul 16, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed hermetically. LGTM with non-blocking notes.

Verified:

  • The cni-plugins v1.9.1 SHA-256 values in the Dockerfile match the upstream .sha256 sidecars.
  • Atomic rename staging; helm unittest 6/6, dropping to 5/6 under a mutation of the atomic-rename step (non-vacuous); the bats suite (7 tests) passes; shellcheck clean.

Non-blocking:

  • PR body omits the mandatory Downstream Repositories checklist; the trigger map shows no downstream repo affected, but it should be stated.
  • The install-cni-plugins init container is a second non-toggleable sequential startup dependency; a persistent failure on a node blocks kube-multus from starting, the same class as the existing install-multus-binary.
  • The init-container image is digest-pinned but not via cozy-lib.image; not new debt, the line matches sibling containers in the same vendored file.
  • CI E2E is red for an unrelated reason: a stale fixture references an already-removed API group; the build and unit/controller jobs are green.

@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/multus-stage-reference-cni-plugins branch from 0de4de0 to 6be23f9 Compare July 27, 2026 20:52
@github-actions github-actions Bot added the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Jul 27, 2026
@lexfrei Aleksei Sviridkin (lexfrei) changed the title feat(multus): stage reference CNI plugins into /opt/cni/bin feat(multus): stage reference CNI plugins where nodes lack them Jul 28, 2026
@lexfrei Aleksei Sviridkin (lexfrei) removed the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Jul 28, 2026
@lexfrei Aleksei Sviridkin (lexfrei) changed the title feat(multus): stage reference CNI plugins where nodes lack them feat(multus): stage reference CNI plugins from the multus image Jul 28, 2026
@lexfrei Aleksei Sviridkin (lexfrei) changed the title feat(multus): stage reference CNI plugins from the multus image feat(multus)!: stage reference CNI plugins from the multus image Jul 28, 2026
@lexfrei Aleksei Sviridkin (lexfrei) added the kind/breaking-change Indicates the change introduces a breaking API or behaviour change label Jul 28, 2026
Multus-backed NetworkAttachmentDefinitions delegate to the upstream
containernetworking reference plugins (bridge, macvlan, ipvlan, ...),
and nothing guarantees a node has them in the directory Multus
searches.  On k3s they live under /var/lib/rancher/k3s/data/cni while
Multus and Cilium install into /opt/cni/bin, so a bridge-type NAD fails
there with `failed to find plugin "bridge" in path [/opt/cni/bin]`.

Bundle 14 of the release's plugins into the multus image and add an
install-cni-plugins init container that installs them into the host
/opt/cni/bin, mirroring the existing install-multus-binary container.
loopback, dhcp, tap and dummy are left out; the Dockerfile records why.

Gate it on a new chart value, stageCniPlugins, resolved per bundle and
on for both.  Talos bundles only bridge, firewall, flannel, host-local,
loopback and portmap, so the other ten are missing there exactly as on
generic Linux; the four that overlap are rewritten with the same
upstream version, v1.9.1 on both sides, rather than an older one.  A
marker in the image Dockerfile records the Talos release that pin was
compared against, and a test holds it to the release the platform
ships.  multus therefore moves out of system.common-packages into the
two variant branches, the way linstor already does for talos.enabled.
networking.stageCniPlugins overrides the variant default in either
direction.

Where staging is on the install is unconditional: a plugin is replaced
on every pod recreation, not only when the destination is missing.  A
destination that is already a directory is reported and left alone,
since replacing it is not something this can do safely.  Turning the
value off afterwards does not undo it; nothing here removes or
restores a plugin.

The container is not privileged and takes no mount propagation.  It
runs in the spc_t SELinux domain, without which an enforcing node
refuses the write.  Each plugin is installed under a temporary name and
swapped in with a rename: /opt/cni/bin is shared with cilium and
kube-ovn.  The plugin tarball is checksummed before extraction.

A plugin that cannot be installed is reported on stderr and to
/dev/termination-log, and the container still exits 0: Multus stays the
node's primary CNI and nothing removes its config, so an init container
that never completes would leave the shim with no daemon behind it.

The manifest change lives in customize-deployment.patch so it survives
make update.

BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI
plugins in /opt/cni/bin on existing nodes, Talos included -- four of the
names it writes are ones the Talos node image also carries.  Opt out
before upgrading: declining afterwards does not restore what was
replaced.

Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
The check that multus is pointed at the cilium conflist matched its
JSON fragment anywhere in the rendered daemon config.  The same text
nested inside any other key satisfies it, so a config whose effective
multusMasterCNI names a different conflist passes.  Multus uses that
value as written when it is non-empty -- it delegates to whatever is
named, so a wrong-but-existing name silently routes through the other
CNI instead of cilium, which is the state this assertion exists to
prevent.  Auto-discovery happens only when the value is empty, and a
name that does not exist fails manager startup rather than falling
back.

Anchor it to the top level, where the template writes its keys.  That
pins the position rather than parsing the document; deciding which key
is effective needs a JSON parser, which helm-unittest does not have.

Signed-off-by: Aleksei Sviridkin <[email protected]>
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the feat/multus-stage-reference-cni-plugins branch from 6be23f9 to cd57ee6 Compare August 5, 2026 11:59
@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review size/XXL This PR changes 1000+ lines, ignoring generated files and removed size/L This PR changes 100-499 lines, ignoring generated files labels Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

No confirmed CRITICAL/MAJOR defects and no regression on any supported path: every schema-valid corner of the toggle matrix renders correctly, the tests are non-vacuous and reach the dangerous branch, the patch still reproduces the committed template, and the feature is inert until the image is rebuilt. The breaking change is declared explicitly (! + BREAKING CHANGE + release note) with an opt-out and a migration path.

Two items to confirm before merge (not blocking review):

  1. Please confirm that siderolabs/pkgs on Talos v1.13.6 pins containernetworking/plugins v1.9.1. This is the safety premise for defaulting ON on Talos (the change rewrites bridge/firewall/host-local/portmap on every Talos node). The contract test only compares the talos-cni-plugins-checked-against marker, not the actual plugin versions, so a future drift would silently change 4 system CNI binaries per node.
  2. Minor claim mismatch in the PR body: for isp-full (Talos) the default is ON, not off — nothing is staged only because the pinned image carries no /cni-plugins yet, not because the value is off. The body's conclusion (no CI job stages plugins) is correct; the stated reason is not.

Non-blocking: of the flagged comment blocks, only the # block in multus-daemonset-thick.yml actually reaches the rendered manifest (the others are {{/* */}} / .tpl and strip out); it explains a non-obvious securityContext (SELinux spc_t), so it is borderline justified.

Recommended follow-up: run an end-to-end staging test on a generic-Linux node after the release rebuilds the multus image with /cni-plugins — the on-node staging path (SELinux write, atomic rename over a live path, copy-error delivery to kubectl describe) is the one surface covered by neither static analysis nor CI.

@lexfrei
Aleksei Sviridkin (lexfrei) merged commit b758c3b into main Aug 6, 2026
17 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the feat/multus-stage-reference-cni-plugins branch August 6, 2026 12:24
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 7, 2026
The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists
inside hack/cozyreport.bats: one naming the files known to be clean, and
one freezing an exact trap count for each file that was not. Both had to
be edited from whatever change moved them, and that is where the guard
failed, and it never worked even once. The inventory landed in #3567 at
15:25:20; #3195 had landed multus-install-cni-plugins.bats carrying
twelve traps forty-three seconds earlier, so replaying the old guard
against the tree at the commit that introduced it already gives
found != frozen. Main stayed red for about twenty-two and three quarter
hours. The first repair, #3584, landed already red the next morning
because #3548 had brought in run-kubernetes-talos-diagnostics_test.bats
with eight traps two minutes ahead of it, so that fix bought no green
time at all; green came only with the second repair. Neither pair of
PRs shared a line, and each was green against its own base.

A change adding a trap to its own file had to edit a string in a suite
it otherwise never touches, so two
changes sharing no line still invalidated each other: each stayed green
against its own base, git merged both cleanly, and the guard went red
only once the second landed.
A file arriving with traps hit the same wall from the other side -- the
inventory did notice it, since the string it compared was built by
scanning the directory, but absorbing it meant an edit in a file nothing
in the author's diff pointed at.

Replace both lists with a declaration each file makes about itself: one
"# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file
carrying none must install no EXIT trap, which covers the converted
files, the files that never had one, and every file added later.
Growing or shedding a trap now fails in the file the change already
edits, so two changes that disagree about a count collide textually
instead of silently.

That collision is a steady-state property, and this change's own arrival
is the exception. A branch forked before the declaration existed has no
line to disagree with: it converts traps in its own file, merges clean,
and the count goes wrong only once both sides are on main -- verified
against a sibling branch that takes select-e2e_test.bats from fifteen
traps to zero. What the move buys even then is that the red names a file
that branch already edited, the repair is one line inside it, and
rebasing before merge catches it on the branch's own CI. None of those
three held against the inventory.

The declaration is read from the leading comment block, not from
anywhere in the file. A .bats file is shell that writes shell, so the
same line turns up inside a heredoc, a fixture writer or an
expected-output string, where it is data belonging to one test. Reading
it there as a statement about the whole file would let an unrelated
fixture excuse a real trap, and would do it silently, since nothing in
that test's own diff looks like a declaration. A comment block is the
region with no interior: stopping instead at the first @test would still
read a line out of a helper's heredoc.

Not every counted handler is debt. A trap inside an explicit subshell
does not replace the bats binary's own, so a test failing inside
`( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a
backgrounded kubectl proxy that way, and moving the kill to the end of
the body would leak a process holding a fixed port. Its declaration
records that rather than scheduling a conversion, and because the
ratchet is exact, removing the trap fails too -- the count protects the
construct. What the count cannot do is tell the two apart: substituting
a test-level trap for the subshell one keeps the total at 1 and stays
green, which the header states rather than leaves to be found.

Counting bounds the keyword and the signal the same way, at any
character that cannot be part of an identifier, and matches the signal
in either case. Whitespace on the right missed `trap ... EXIT; cd
"$tmp"`; whitespace on the left missed `tmp=$(mktemp -d);trap ... EXIT`
and `(trap ... EXIT; true)`; upper case missed `trap ... exit`, which
bash and dash both install. All are real handlers that scored zero, and
the left boundary matters most, since the inventory being replaced had
none and caught the semicolon form. A bare word boundary is not enough
either way: `bootstrap ` ends in `trap `, and it must keep scoring
nothing. A quoted signal counts for the same reason as the rest.

Two handlers sharing one line are reported rather than counted, since
the count is a count of lines
and the second would otherwise arrive without moving the total;
splitting the line properly needs a shell parser, a semicolon inside a
handler's own action not being a separator, and guessing wrong
undercounts -- the one direction a ratchet cannot afford.

The scan recurses, so hack/e2e-apps/*.bats is covered rather than
sitting one directory below the guard that claims the tree. It reads
.bats and nothing else, so a handler arriving through a sourced .sh
stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs
two, each benign for its own reason rather than by design: one sits in
a function declared with `(` and so runs in a subshell, the other in a
brace function no @test calls. That boundary is stated in the header
rather than papered over.

The guard moves to hack/bats-no-exit-trap.bats: its subject is every
unit suite under hack/, not the report collector it grew up in. Its
fixture helpers assemble the trap keyword and the signal from separate
arguments, because the guard scans its own source and a fixture written
as a literal would be counted as a real trap in it.

Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 8, 2026
…#3622)

<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

The ban on EXIT-trap cleanup in `hack/*.bats` was enforced by two lists
inside `hack/cozyreport.bats`: one naming the files known to be clean,
one freezing an exact trap count for each file that was not. Both had to
be edited from whatever change moved them, and that is where the guard
kept failing.

It is worth being exact about how badly, because the history is sharper
than "it went stale a few times". **The inventory was never correct on
main for a single commit.** It landed in #3567 at 15:25:20. #3195 had
landed `multus-install-cni-plugins.bats` carrying twelve traps at
15:24:37, forty-three seconds earlier. Replaying the old guard's own
logic against the tree at the very commit that introduced it already
gives `found != frozen`. Main then stayed red for roughly twenty-two and
three quarter hours. The first repair, #3584, landed *already red* the
next morning at 10:42:43, because #3548 had brought in
`run-kubernetes-talos-diagnostics_test.bats` with eight traps at
10:40:03, under three minutes ahead of it. That fix was correct and
bought zero green time. Green arrived only with the second repair,
#3602.

Neither pair of PRs shared a line, and each was green against its own
base. That is the whole mechanism, and it is why a third one-line repair
is not the answer.

The mechanism is structural rather than careless. A change that adds a
trap to its own file had to edit a string in a suite it otherwise never
touches, so two changes sharing no line still invalidated each other:
each stayed green against its own base, git merged both cleanly, and the
guard went red only once the second one landed. A file *arriving* with
traps was worse still, because nothing in its author's diff pointed at
that string at all. That is exactly how
`run-kubernetes-talos-diagnostics_test.bats` got in, twice.

That contention is not in the past tense. Two open PRs are editing that
one line right now, and they disagree about what it should say: #3575
adds `run-kubernetes-talos-diagnostics_test.bats=8` to it, repeating a
repair that has already landed, and #3441 removes
`select-e2e_test.bats=15` from it, because it converts that file.
Neither PR is about EXIT traps. Both have to touch that string anyway,
and whichever lands second is wrong until someone edits it again.

So this PR is not fixing a red main. It removes the thing that keeps
making main red, which is why it is worth more than the one-line fix
that is now the established habit.

One honest caveat about its own landing. The textual-conflict property
is steady-state: a branch forked *before* the declaration exists has no
line to disagree with, so it converts traps in its own file, merges
clean, and the count only goes wrong once both sides are on main. I
checked this against #3441, which takes `select-e2e_test.bats` from
fifteen traps to zero. Merged after this, that file would declare
fifteen and hold none. What the move buys even in that case is that the
red names a file the branch already edited, the repair is one line
inside it, and rebasing before merge catches it on the branch's own CI.
None of those three held against the central inventory. Whoever merges
this should expect one such adjustment on the conversion branches still
in flight.

So this replaces both lists with a declaration each file makes about
itself: one `# EXIT-TRAP DEBT: N` comment in its leading comment block.
A file carrying no declaration must install no EXIT trap. Growing or
shedding a trap now fails in the file the change already edits, so two
changes that disagree about a count get a real textual conflict instead
of silently invalidating each other, and a change that leaves the traps
alone edits nothing.

**The include list is redundant, not lost.** It named the files proven
clean, so that a trap reappearing in one of them would fail. Under the
new rule those files carry no declaration, and a file with no
declaration must hold zero traps, so a trap reappearing in any of them
fails on its own, with no list to be on. Coverage widens rather than
narrows: the two lists named twenty files between them, and the rule
covers all fifty bats files under `hack/`, subdirectories included, plus
the ones added tomorrow.

Rebasing this branch onto current main is the property working. Main has
since gained `hack/kubernetes-pre-delete-hook.bats` and
`hack/tenant-pre-delete-hook.bats`, and `hack/cozyreport.bats` grew by
some eight hundred lines. Neither new file installs an EXIT trap, so
neither needed a declaration and neither needed an edit here; the rebase
took no conflict at all. Under the inventory, each arriving file was a
coin toss on whether somebody had remembered the string.

To be precise about what the inventory could and could not do, since it
is easy to overstate: it did *notice* a new file carrying traps. The
string it compared was built by scanning the directory, so an arriving
file appended a token and failed the comparison, which is exactly how
main went red. What it could not do is let that file arrive without an
edit in a foreign suite. Being seen and being absorbable are different
properties, and only the second one decides whether two changes can land
independently.

The declaration is pinned in both directions. Declaring N while holding
N+1 fails, obviously; declaring N while holding N−1 fails too. Without
that second half the number becomes a ceiling and rots upward: somebody
converts half a file, the declaration stays, and the guard quietly
licenses traps that were removed long ago.

It is read only from the leading comment block, under the shebang and
above the first line of code. A `.bats` file is shell that writes shell,
so the same line turns up inside a heredoc, a fixture writer or an
expected-output string, where it is data belonging to one test;
honouring it there would let an unrelated fixture excuse a real trap,
silently, with nothing in that test's own diff looking like a
declaration. A comment block is the region with no interior; stopping
instead at the first `@test` would still read a line out of a helper's
heredoc.

Not every counted handler is debt. A trap inside an explicit subshell
does not replace the one the `bats` binary installs, so a test failing
inside `( … )` still prints its `not ok`, checked against a test-level
trap in the same file, where the TAP line vanishes.
`hack/e2e-test-openapi.bats` kills a backgrounded `kubectl proxy`
exactly that way, and "convert it like the others" would leak a process
holding a fixed port and wedge the next run. Its declaration now records
the carve-out instead of scheduling a conversion, and because the
ratchet is exact in both directions, *removing* that trap fails too, so
the count protects the construct rather than marking it for deletion.
`docs/agents/e2e-testing.md` previously scoped this exception to
Chainsaw `script` steps only; it now names the BATS subshell case as
well.

The counting bounds the keyword and the signal the same way, at any
character that cannot be part of an identifier, and matches the signal
in either case. Whitespace on the right missed `trap … EXIT; cd "$tmp"`;
whitespace on the left missed `tmp=$(mktemp -d);trap … EXIT` and `(trap
… EXIT; true)`; upper case missed `trap … exit`, which bash and dash
both install. All of those are real handlers that scored zero. The left
boundary is the one worth dwelling on, because the inventory being
replaced had none at all and *did* catch the semicolon form. Getting it
wrong here would have narrowed coverage while the commit claimed to
widen it. A plain word boundary is not enough either: `bootstrap ` ends
in `trap `, and it has to keep scoring nothing, or the documented answer
to a red guard (add a debt line) would buy a file a permanent licence
for one real trap to silence a line that has none.

Two handlers sharing one line are reported rather than counted, because
the count is a count of lines and the second would otherwise arrive
free. Splitting such a line properly needs a shell parser, since a
semicolon inside a handler's own quoted action is not a separator, and
guessing wrong undercounts, the one direction a ratchet cannot afford.

**What this does not fix.** The declaration is still a loophole: a new
file can write `# EXIT-TRAP DEBT: 8` instead of cleaning up, and nothing
here makes that impossible. What changes is that the admission is local
and visible. It sits at the top of the file it excuses, in front of
whoever reviews that file, instead of being a number in a neighbouring
suite nobody in that review is reading. Today's loophole is the same
size and invisible.

Three more limits, all stated in the guard's own header rather than left
to be discovered. The scan is lexical, so a signal computed at runtime
and a quoted action spanning physical lines without a backslash are both
invisible. An exact count catches addition and removal but never
substitution: swap the openapi file's subshell trap for a test-level one
and the total stays 1. And the scan reads `.bats` only, so a handler
arriving through a sourced `.sh` is outside it.
`hack/e2e-chainsaw/_lib/run-kubernetes.sh` installs two right now, and
each is benign for its own reason rather than by design: the one in
`cozy_capture_tenant_talos` because that function is declared with `(`
and so runs in a subshell, the one in `run_kubernetes_test` because no
`@test` calls it despite being declared with `{`. Three `hack/*.bats`
source that library, and two tests in the converted file call
`cozy_capture_tenant_talos`, so flipping a single `(` to `{` reinstates
a test-level handler in both of them with the guard green. Widening the
scan to `.sh` would mean counting handlers that are correct in a script
and wrong only in a test body, so the honest answer is that this is
where the instrument stops.

Three further boundaries, recorded here so they land as known edges
rather than as surprises. The old include list also failed when a file
named on it disappeared from the tree; the new rule can only judge a
file that is present, so a deleted converted file goes unnoticed. That
is a genuinely smaller check, though its absence shows up in the diff
that deletes the file. The guard's own failure messages are code lines,
so they are scanned by the pattern they belong to: they pass today only
because no bare `EXIT` or `0` happens to follow the keyword in any of
them, and a rewording that introduced one would make the file demand a
debt of itself. It fails loudly rather than quietly, and the fixture
writers and test titles already split the keyword from the signal for
this reason, but the messages do not. Finally,
`docs/agents/e2e-testing.md` bans test-level `EXIT` *and* `RETURN`
traps, while every mechanical guard this repo has had, the one being
deleted included, matches only `EXIT` and `0`. `hack/` holds no RETURN
trap today, so nothing regresses here, but half of that documented rule
has never had an executor.

The guard moves out of `hack/cozyreport.bats` into
`hack/bats-no-exit-trap.bats`, because its subject is every unit suite
under `hack/` and not the report collector it grew up in. Living inside
`cozyreport.bats` is precisely why unrelated PRs kept converging on one
line. `hack/md-no-hardwrap.bats` is the neighbouring precedent.

Separately, the eight EXIT traps in
`hack/run-kubernetes-talos-diagnostics_test.bats` are converted to
end-of-body cleanup rather than given a declaration. That file was
written six weeks after the ban was documented, so "it predates the
rule" is not available to it, and handing it a debt is literally the
move that failed the last two times. The remaining files keep their
declarations, since a mass conversion would collide with the branches
that own them.

One note on where the prose sits. The guard's own header carries only
what a reader has to keep true: why the ban exists, the two carve-outs,
where the declaration lives and why there, that the count is exact in
both directions, why the marker is anchored at column zero, and where
the lexical scan stops. Everything above about *how* the inventory
failed (the timestamps, the PR numbers, the arithmetic) is deliberately
not in the file. It is true today and will rot on its own; a comment
nobody is obliged to maintain is how a file grows a paragraph that is
quietly false.

### Screenshots

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Testing**
- Added automated auditing for `EXIT` traps across Bats end-to-end
tests, including validation of tracking declarations and edge cases.
- Improved diagnostics tests by replacing trap-based temporary-directory
cleanup with explicit cleanup steps.
  - Added tracking annotations for remaining trap-related cleanup work.

- **Documentation**
- Clarified when traps are permitted inside self-contained subshells and
how remaining cleanup debt is reported.
  - Updated review guidance for consistent end-to-end test maintenance.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/networking Issues or PRs related to networking (ingress, gateway, vpn, metallb, kube-ovn) area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/breaking-change Indicates the change introduces a breaking API or behaviour change kind/feature Categorizes issue or PR as related to a new feature size/XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants