containers: Always run container egress/ingress sidecar, and make ingress go through it

[gabivlj]

We are introducing a new to ingress to containers in local dev that makes us avoid needing a EXPOSE <port> in the Dockerfiles.
We do not manage the port mapping through docker, instead we do HTTP CONNECT ingressing in an agreed upon port with proxy-everything.

We simplify the sidecar setup and make it mandatory for it to be configured and run it next to the user container. This also makes networking data races not happen anymore in local dev.

[ask-bonk]

I see a potential lifetime issue. On line 228, headers borrows *headerTable. On line 235, headerTable is moved into the httpClient attachment. After this, headers has a dangling HttpHeaderTable* inside. On line 236, headers is passed to httpClient->connect(). Since httpClient now owns headerTable, the HttpHeaderTable object is still alive, so the pointer inside headers is technically valid. But this is fragile and relies on understanding the internal ownership chain.

More importantly, connectRequest returned from httpClient->connect() at line 236 borrows from httpClient. Then at line 237, co_await connectRequest.status suspends. During this suspension, httpClient is still alive as a coroutine local. At line 255, connectRequest.connection is moved out. Then httpClient is moved into the attach() at line 264. This keeps the HTTP client alive for the pump. This all looks correct.

One concern: connectRequest at line 236 is a kj::HttpClient::ConnectRequest which contains kj::Promise<kj::HttpClient::ConnectRequest::Status> status and kj::Own<kj::AsyncIoStream> connection. After co_await connectRequest.status on line 237, we get the status. Then on line 255, we kj::mv(connectRequest.connection) to get the tunneled connection. This is the standard pattern for HTTP CONNECT and looks correct.

After this exhaustive analysis, the code is clean. The only thing I want to flag is a minor comment inconsistency and the fact that the destroy() method now destroys the sidecar after the main container but the comment in the destructor still says "Best-effort cleanup for both containers" without explaining the order. But this is very minor.

Actually, wait -- I want to verify one more thing. I noticed that in the start() flow, the sidecar is created and started before the main container:

co_await ensureEgressListenerStarted();  // start egress listener
containerSidecarStarted = false;
co_await ensureSidecarStarted();          // create + start sidecar
co_await createContainer(...);            // create main container (networkMode: container:sidecar)
co_await startContainer();                // start main container

This is correct because the main container's networkMode is container:sidecarContainerName, which requires the sidecar to exist and be running first. Good.

And in destroy():

co_await destroyContainer();       // destroy main container first
co_await destroySidecarContainer(); // then destroy sidecar

This is also correct -- you can't destroy the sidecar first because the main container's network namespace would break.

But in the destructor:

auto sidecarCleanup = dockerApiRequest(...DELETE sidecar...);
auto mainCleanup = dockerApiRequest(...DELETE main...);
cleanupCallback(kj::joinPromises(kj::arr(kj::mv(sidecarCleanup), kj::mv(mainCleanup))));

Both deletions are started concurrently (via joinPromises). Since these are force-delete with ?force=true, order shouldn't matter for cleanup. This is fine.

After this thorough review, the code looks correct. The PR cleanly restructures the container networking model so the sidecar owns the network namespace and handles both ingress and egress. The test coverage includes a new testPortNotListening test case.

LGTM

github run

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 136 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.59%. Comparing base (755795d) to head (faa96fd).

Files with missing lines	Patch %	Lines
src/workerd/server/container-client.c++	0.00%	134 Missing ⚠️
src/workerd/server/server.c++	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6292      +/-   ##
==========================================
- Coverage   70.64%   70.59%   -0.05%     
==========================================
  Files         420      420              
  Lines      111666   111736      +70     
  Branches    18383    18394      +11     
==========================================
- Hits        78886    78881       -5     
- Misses      21799    21871      +72     
- Partials    10981    10984       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉