Probe the firewall under the runtime the run will select - #102
Merged
Amitgb14 merged 7 commits intoAug 22, 2026
Merged
Conversation
The egress allowlist cannot be programmed under gVisor, so `--profile prod
--runtime runsc` refuses to start:
ip6tables: Failed to initialize nft: Protocol not supported
sandbox-cli: egress firewall setup failed; refusing to run without the requested allowlist
Debian points `iptables` at the nft variant, which is right on an ordinary host.
gVisor implements only the older setsockopt interface, so the nft binaries fail
before touching a rule while the `-legacy` ones work. Measured inside this image
on Rocky Linux 10.2:
iptables: FAILS ip6tables: FAILS
iptables-legacy: ok ip6tables-legacy: ok
That made prod's two demands mutually exclusive on a gVisor host: it requires a
kernel of its own *and* an egress allowlist, and the allowlist could not be
programmed inside the kernel it was demanding.
pick_iptables tries the nft binaries first and falls back to legacy, and every
call site now goes through what it chose. Order matters: an ordinary runc host
keeps the backend its distribution selected and nothing about those runs
changes. Chosen by trying rather than by detecting the runtime, for the reason
the host-side probe already gives — a runtime name says what somebody installed,
and what matters here is what the kernel will answer.
Still fails closed. A container that can serve neither backend refuses, because
running without the requested allowlist is the one thing this script must never
do; the message names `runsc install -- --net-raw`, which is the other half of
this on a gVisor host. Without that flag gVisor serves neither backend, and no
change here can fix it — the container simply cannot filter.
Found by #89 phase 3/4 on a host with gVisor actually installed. The Dockerfile
is content-addressed into the image tag, so the first run after this rebuilds.
Six review findings, and the first two say the fallback was added in the wrong shape: only the container script learned it, while everything else that touches iptables kept hardcoding the bare binary. **doctor and the launch now disagreed on the hosts this fixes.** FirewallProgrammable's probe ran bare `iptables`, which resolves to the nft variant — the one gVisor cannot serve. On a runsc-default host the preflight reported "a container here cannot program the firewall" and advised `--network default`, while the run itself worked. Before this branch they at least agreed; the fix made them disagree, which CLAUDE.md names as the failure to avoid. **The only end-to-end firewall assertion read the wrong store.** The integration test execs `iptables -S INPUT`; on the legacy path the rules live elsewhere, so it would report "the INPUT chain was never programmed" for a container that is fully default-deny. The ~42 rewritten call sites had no test that could observe them on the backend being added. So the choice moves into the image as `sandbox-iptables`, and all three callers ask it: the entrypoint programs with it, doctor's probe uses it, the suite reads back through it. Three copies of "which binary works" would drift, and the drift is a preflight that answers differently from the launch. **It writes rather than lists.** `-L -n` only proves the table can be read — docker_cli.go already argues this, which is why the host probe creates rules — and today's measurements prove it: legacy passes `-L` under gVisor and then fails on `-m conntrack`. Creating and deleting one empty chain is the cheapest thing that proves the kernel accepts a write. **The kernel's own error survives.** The first version swallowed stderr from both candidates and offered only a gVisor remedy, so a rootless daemon — whose real error is "Permission denied (you must be root)" — was told to install runsc. The last candidate's stderr is now ours. Also: the chosen backend is logged, because IPv4 and IPv6 are picked independently and an operator reading back with the wrong binary sees an empty chain; and a CHANGELOG entry, which says plainly that this removes one barrier to gVisor and not the last one — the allowlist still cannot be enforced there, because gVisor has no connection tracking at all.
Pick the iptables backend the kernel will serve
…-selected-runtime
doctor asked whether a container can program iptables by starting one on the
engine's *default* runtime. Whether it can is a property of the kernel the
container gets, and runtimes differ: gVisor serves only the legacy backend, and
only when installed with --net-raw.
Measured on a Rocky Linux host with gVisor registered — doctor reported
ok egress firewall a container here can program the nat, redirect, owner
and conntrack rules the firewall needs
from a runc probe, while `--runtime runsc` could not program a single rule. The
preflight passing and the launch failing is what ClassifyRuntimeGap was
centralised to prevent; this check was simply never centralised with it.
FirewallProgrammable now takes the runtime and renders --runtime on the probe
container, and doctor passes the same selectedRuntime it already resolves for
the kernel-boundary check — so `doctor --runtime runsc` asks about runsc, and a
config naming one asks about that.
Empty stays empty: with nothing selected the probe runs on the engine's default,
which is what it should answer for.
Found by #89 phase 4. It is on this branch rather than main because the runtime
is already plumbed into RunChecks here; doing it on main would have duplicated
that and conflicted.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Five review findings, and the first two are the bug this commit set out to fix, inverted. **The probe sent the user's spelling, not the engine's.** The run path translates a runtime through resolveRuntime — a containerd daemon lists `io.containerd.runc.v2` while a config says `runtime: runc` — and the probe did not. So on that host the probe died with "unknown or invalid runtime name" and doctor reported that a machine whose runs program the firewall perfectly well cannot. Scoping the probe to a runtime and then naming it differently from the run reintroduced the disagreement it exists to prevent. **A runtime the engine does not have was reported as a host defect.** That is FirewallUnknown, not FirewallBlocked: the probe never ran, so it learned nothing about whether this host can filter. As Blocked it produced StatusWeak, blamed the daemon, and offered `--network default` — telling a prod operator to switch the egress allowlist off to fix a misspelled --runtime, while checkRuntimes reported the real GapMissing one line below. **The remedy still named only daemon causes.** Once the probe runs under a selected runtime, "rootless or userns-remapped" is not the only explanation and on gVisor it is not the explanation at all — iptables is gated behind `runsc install -- --net-raw`. The remedy now says so when a runtime was selected, and is unchanged when none was, which is the case it was written for. Also: the probe gets its own half of doctor's budget, because it is the only check that starts a container and under a stronger runtime that means booting a micro-VM — sharing one deadline turned a slow probe into two prod failures. And the argv moves into a pure firewallProbeArgs so it can be asserted: it is built outside BuildArgs, so the golden --dry-run test never saw it, and deleting the --runtime append left everything green. The new test also pins the ordering, since a flag after the image reference is handed to the container instead.
The review of this PR found five things, all in the seams where the new runtime dimension meets messaging that predates it. "Unknown" now covers three causes — an unbuilt image, a host too busy to answer, and a runtime the engine does not have — and one canned remedy told every one of them to build the image. Under prod that is unactionable advice attached to a failure. The cause is now a **value** carried beside the verdict (`FirewallReport`), for the reason its neighbour gives: the alternative is the caller matching on reason prose, which is the substring-across-a-package-boundary the enum replaced. The blocked remedy keyed on the runtime a *flag* named, so a daemon whose default-runtime is runsc — nothing selected, gVisor nonetheless what runs — was told "rootless or userns-remapped daemons often cannot; use --network default". That is the one instruction the comment three lines above says a mis-identified check must never give, and it does not fix gVisor: its netstack has no conntrack, which the rules need, so the `runsc install -- --net-raw` this used to lead with sends an operator away and back to a byte-identical failure. An engine refusing the runtime *name* is not a container failing to program rules — the container never started. resolveRuntime is non-fatal in two paths by design, so an unaccepted spelling still reaches the launch, and reading the refusal as Blocked blamed the daemon while the runtime check one line below reported that same runtime as fine. The probe's half-budget is gone, and the ordering is what replaces it: the runtime facts come from one `info` call made *before* the probe, so the expensive check can take the time a micro-VM needs without starving the cheap one. That also removes a second `info` call, and two dead parameters from checkRuntimes, which no longer asks anything. Not fixed here, deliberately: the `sandbox-iptables` block splits the sandbox-egress-setup comment mid-sentence. This branch changes nothing else in the Dockerfile, and image.Ref hashes it — so rejoining a sentence would invalidate every cached base image and cost every user a multi-minute rebuild. It should ride along with the next change that alters the image for a reason.
Owner
Author
|
Review addressed — five of six fixed, one deliberately deferred with a reason.
On finding 1, the cause is carried as a value rather than sniffed from the reason string, which is what the On finding 5, ordering replaces the cap: On finding 6 — not fixed, and this is the trade. This branch changes nothing else in the Dockerfile, and Tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #88 — base is
feat/prod-demands-a-kernel, so the diff here is justthis change.
doctorasked whether a container can program iptables by starting one on theengine's default runtime. Whether it can is a property of the kernel the
container gets, and runtimes differ: gVisor serves only the legacy backend, and
only when installed with
--net-raw.Measured on the Rocky Linux host in #89, with gVisor registered:
…from a runc probe, while
--runtime runsccould not program a single rule. Thepreflight passing and the launch failing is exactly what
ClassifyRuntimeGapwas centralised to prevent; this check simply was never centralised with it.
FirewallProgrammablenow takes the runtime and renders--runtimeon theprobe container, and
doctorpasses the sameselectedRuntimeit alreadyresolves for the kernel-boundary check. So
doctor --runtime runscasks aboutrunsc, and a config naming one asks about that.
Empty stays empty: with nothing selected the probe runs on the engine's default,
which is the right thing to answer for.
Why here and not on main
The runtime is already plumbed into
RunCheckson this branch. Doing it on mainwould have meant adding that plumbing a second time and conflicting with #88 in
the code #88 rewrites.