Restore isolated Docker network for managed containers#6208
Restore isolated Docker network for managed containers#6208
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR restores network isolation for workerd-managed Docker containers by creating a dedicated workerd-network bridge instead of using the shared default bridge.
Issues (ranked by severity)
-
[High] Atomic guard is broken across concurrent coroutines —
workerdNetworkCreatedusesexchange(true, acquire)as a "run once" guard, matching the pattern of other guards in this file. However,createWorkerdNetwork()is called from two sites: once fromgetDockerBridgeIPAMConfig()(on 404) and once fromcreateContainer(). Because the guard fires on the first call, the second call site becomes a no-op regardless of whether the first actually succeeded. If the first call comes fromgetDockerBridgeIPAMConfig()and the network already exists (409), this is fine. But if the first call fails (non-201, non-409) and theKJ_ON_SCOPE_FAILUREresets the flag, a concurrent coroutine that already readtruefrom the exchange will have silently skipped creation. The real issue is simpler though — see issue #2. -
[High]
createWorkerdNetwork()is called redundantly fromgetDockerBridgeIPAMConfig()—getDockerBridgeIPAMConfig()already handles a 404 by callingcreateWorkerdNetwork()then re-inspecting. ThencreateContainer()also callscreateWorkerdNetwork()just before building the host config. Because the atomic guard makes the second call a no-op this is merely wasteful, but it makes the control flow confusing and fragile. The call increateContainer()should be removed —getDockerBridgeIPAMConfig()(already called earlier in thestart()flow) handles on-demand creation. -
[Medium] Missing space in error message — The string literal concatenation on line 449 produces
"Failed to create workerd-network.Status: ..."(no space after the period). C++ concatenates adjacent string literals. -
[Low] IPv6 warning is noisy for the common case — Most Docker installations have IPv6 disabled by default. Logging a WARNING on every network creation will generate noise. Consider downgrading to
INFOor only warning once.
|
|
||
| if (response.statusCode != 201 && response.statusCode != 409) { | ||
| JSG_FAIL_REQUIRE(Error, | ||
| "Failed to create workerd-network." |
There was a problem hiding this comment.
Missing space: adjacent string literals are concatenated by the compiler, producing "Failed to create workerd-network.Status: ". Add a space (or newline) after the period.
| "Failed to create workerd-network." | |
| "Failed to create workerd-network. " |
| jsonEnv.set(envSize + i, defaultEnv[i]); | ||
| } | ||
|
|
||
| co_await createWorkerdNetwork(); |
There was a problem hiding this comment.
createWorkerdNetwork() is already called on-demand inside getDockerBridgeIPAMConfig() when the network doesn't exist (404 → create → re-inspect). Since getDockerBridgeIPAMConfig() is called earlier in the start() flow (via ensureSidecarStarted / ensureEgressListenerStarted), this explicit call is redundant. If the intent is to guarantee the network exists even when those paths haven't run, this should at least have a comment explaining why.
If it is truly just a safety net, consider removing it to keep the control flow clear:
| co_await createWorkerdNetwork(); |
| jsonRoot.setEnableIpv6(ipv6Enabled); | ||
| if (!ipv6Enabled) { | ||
| KJ_LOG(WARNING, | ||
| "Docker has IPv6 disabled. Connections to Workers via IPv6 won't work. If you want to enable IPv6, remove the workerd-network after re-enabling."); |
There was a problem hiding this comment.
Most Docker installations have IPv6 disabled by default, so this WARNING will fire on virtually every network creation. Consider downgrading to INFO, or gating on a debug flag, to avoid log noise in the common case.
|
Review posted successfully on PR #6208. Here's a summary of what I flagged:
|
|
Hey, |
In other words, we are not really worried about container network isolation in local development. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6208 +/- ##
==========================================
- Coverage 70.58% 70.56% -0.03%
==========================================
Files 413 413
Lines 109992 110024 +32
Branches 18120 18125 +5
==========================================
Hits 77640 77640
- Misses 21544 21575 +31
- Partials 10808 10809 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Motivation
bridge, removing the isolation boundary and allowing lateral access to other containers on the host.workerd-networkbridge instead of the shared default.Description
WORKERD_NETWORK_NAMEandcreateWorkerdNetwork()to create a dedicatedworkerd-networkbridge (with IPv6 enabled according to daemon capability) when missing.getDockerBridgeIPAMConfig()to inspect/networks/workerd-networkand create it on-demand if it does not exist, returning gateway and subnet from that network.createWorkerdNetwork()before container creation and setHostConfig.NetworkModetoWORKERD_NETWORK_NAMEso managed containers are attached to the dedicated network.workerdNetworkCreatedand corresponding header declarations to avoid races when creating the network.Testing
bazel build //src/workerd/server:container-client, but the Bazel bootstrap download was blocked in this environment (Forbidden), so the build could not be completed.Codex Task