feat(provider): publish-endpoint deploys a network relay for provider services - #14193
feat(provider): publish-endpoint deploys a network relay for provider services#14193ndeloof wants to merge 7 commits into
Conversation
ee1f177 to
243b41f
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR introduces a well-structured relay mechanism for provider services — the core logic in relay.go, plugins.go, and relay/main.go is sound. One low-severity issue was found in the example/demo code only.
c0c356c to
fee8789
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
fea2480 to
893cb95
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two findings in the new relay code introduced by this PR.
893cb95 to
51b5795
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium-severity correctness/reliability issues and two low-severity issues were found in the new relay code.
Findings summary:
relay/main.go:wg.Wait()does not drain activeforward()connections on shutdown (medium, 🟢 strong 100/100)pkg/compose/relay.go: paused relay with matching identity is force-removed instead of unpaused (medium, 🟢 strong 97/100)pkg/compose/relay.go: missing existence check beforeproject.Networks[key].Name(low, 🟡 moderate 57/100)docs/examples/provider.go:bufio.ReadStringhas no timeout when reading serve-demo address (low, 🟢 strong 100/100)
51b5795 to
9a1e73c
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
1 critical finding (high-severity CONFIRMED bug) and 1 minor finding (low-severity CONFIRMED).
Summary: The PR introduces a solid relay mechanism for provider services. The main correctness issue is an orphan container left behind when network attachment partially fails; the Dockerfile has a dead ARG that should be wired up.
9a1e73c to
4bbd5eb
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium-confidence findings in the new relay code introduced by this PR.
4bbd5eb to
f7054f2
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding: the relay guard introduced by this PR is incomplete in pkg/compose/cp.go — the default branch (common cp usage without --index) skips the checkRelayTarget check that was correctly added to the options.Index > 0 branch.
f7054f2 to
83afdc3
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
🟢 No issues found — LGTM! View logs.
83afdc3 to
f0431d0
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One confirmed issue found in the new relay deployment code.
f0431d0 to
af2aaf6
Compare
ae1739c to
55984e3
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR adds a well-structured network relay feature for Docker Compose provider services. The WaitGroup lifecycle in relay/main.go is correct — serve() completes all wg.Add calls before defer wg.Done() fires, so wg.Wait() in main only returns when all connection goroutines are finished. The relay container lifecycle (idempotency, state handling, label computation, network selection) and ordering in down look correct.
One minor issue in the example provider's subprocess management is noted below.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Incremental review — covers only commits since
55984e38f42b3e5edc1d218415366b595be37152.
The three changed files are a clean, focused refactoring:
-
pkg/compose/plugins.go— replacesdefer mux.Unlock()with an explicitmux.Unlock()so the mutex is released before the Docker API work inensureServiceRelay. The lock acquisition and release are on a single, straight-line path (no early returns between them), so the refactoring is safe.networkKeysis correctly computed under the lock viarelayNetworks, which readsproject.Services. -
pkg/compose/relay.go—relayNetworkscall removed fromensureServiceRelay; the result is now received as a parameter instead. All remaining reads ofprojectinside this function touch onlyproject.Name,project.Networks,project.ComposeFiles, andproject.WorkingDir— none of which concurrent providers mutate — so the post-lock reads are race-free. Thelen(networkKeys) == 0guard at the top of the create path correctly prevents an index-out-of-bounds onnetworkKeys[0]. -
docs/examples/provider.go— comment-only addition explaining the subprocess lifetime design.
No bugs found in the incremental changes.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity and one low-severity finding in the new relay code.
… services
A provider's resource lives outside the compose network: consumers could
only reach it through injected variables carrying a host-published
address — nothing like the compose-native experience of addressing a
service by name at its well-known port.
A provider may now publish where each endpoint of its resource actually
listens:
{"type": "publish-endpoint", "message": "80=localhost:49152"}
The endpoint is announced as seen from the provider's host: the relay —
the component that knows it runs inside a container — rewrites loopback
or unspecified upstream hosts to host.docker.internal (resolved through
its injected host-gateway extra_host); routable addresses pass through.
When at least one endpoint is published, compose deploys a relay
container in place of the service: a minimal TCP forwarder (new relay/
directory, published as docker/compose-relay, overridable with
COMPOSE_RELAY_IMAGE for internal registries) joining the networks of the
services that depend on the provider service, aliased with the service
name. Consumers then use http://<service>:<port> as if the service were
a regular container.
The relay is a first-class project container — canonical name, standard
compose labels including config-hash (label-driven commands run without
the compose file keep seeing the service: ps, logs, stop, down) — plus
the com.docker.compose.relay label declaring its role:
- the reconciler already leaves provider services' containers alone, and
the relay's identity hash (image + routes) makes up idempotent: kept
when routes are unchanged, recreated otherwise;
- process-level commands (exec, cp) refuse a relay — there is no service
process in it to act on;
- the up monitor excludes relays from the containers whose termination
ends an attached up: they are long-lived infrastructure and would
otherwise keep 'up' waiting forever.
The example provider demonstrates the flow behind PROVIDER_DEMO_ENDPOINT
(a detached helper serving a fixed HTTP response), backed by an e2e
scenario asserting the compose-native address works and exec is refused.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…internal A provider legitimately expresses its endpoints from the host's perspective — "localhost:5734" is where its resource listens, on the machine compose runs on. But the relay dials from its own network namespace, where loopback names the relay container itself: routes were passed verbatim, so every connection died on the relay's own empty loopback while the host.docker.internal ExtraHosts mapping provisioned for exactly this purpose sat unused. relayRoutesSpec now rewrites host-relative upstreams (localhost, any loopback IP, unspecified or empty host) to host.docker.internal before rendering; LAN IPs and DNS names still pass verbatim. The rewrite happens before the identity hash, so existing relays carrying the old routes are recreated on the next up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…onse Providers push messages on their own initiative — setenv, info, publish-endpoint… — they do not respond to anything. "invalid response from plugin" sent users looking for a request that never existed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
down handled a provider service by running the plugin alone — but the service may own a project container, the relay deployed when it published endpoints. Left running, it kept the project network in use and `down -v` failed with "Resource is still in use". The relay is now part of the service's deprovisioning: its containers are removed first, then the plugin removes the provider's resource — mirroring up, which provisions the resource before deploying the relay. The example provider's down used to answer with a hardcoded error (a leftover no test relied on): it now succeeds, with the failure simulation kept behind PROVIDER_DOWN_FAILURE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
runPlugin held the global mutex — there to guard concurrent writes to project.Services — across ensureServiceRelay, whose Docker API work (list, create, image pull, start, a 30s removal wait) forced every concurrent provider to wait on the slowest one. The mutex now covers only the shared-state work: env-var injection plus the relay's network selection, which reads project.Services and must not race with another provider's writes. The relay is deployed after the lock is released, taking the pre-computed network keys — simply unlocking around the call, as first suggested, would have traded the serialization for a data race on the services map. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…time The serve-demo subprocess is the demo's provisioned resource: it must outlive the provider invocation — consumers reach it through the relay after up returns — and it reaps itself after three minutes. Spell that out where a reader (or reviewer) would otherwise expect a Kill/Wait. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
126009f to
3fb9b3a
Compare
A peer that never closes after receiving the relayed FIN used to pin the forward's goroutine pair and both TCP connections until SIGKILL — and with them the drain in main. Force-closing both ends as soon as one direction finishes would have thrown out TCP half-close (a client that FINs its request and then reads a long response), so the surviving direction now runs under an idle grace instead: a read deadline re-armed before every Read once the other direction is done. Active streams are never cut — only pairs sitting idle past the grace are reaped. Locked by two tests: the silent-peer pair is reaped at the grace, and a response still streaming after the client's half-close survives well past it. The demo provider's comment also spells out why serve-demo is neither Wait()ed nor a zombie: the provider exits within seconds, so init has long adopted — and reaps — the subprocess when its three minutes are up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR adds transparent relay infrastructure for provider services that publish endpoints — a clean design that correctly handles the relay lifecycle (up idempotency, down ordering, exec/cp guards, monitor exclusion). The implementation is solid overall.
Two minor correctness issues found in the new relay binary:
| // persist until a descriptor is freed, and a tight loop would | ||
| // spin the CPU and flood the log. | ||
| log.Printf("accept on %s: %v", listener.Addr(), err) | ||
| time.Sleep(backoff) |
There was a problem hiding this comment.
[low] Backoff cap overshoots intended 1s maximum — actual max sleep is 1280ms
The condition if backoff < time.Second { backoff *= 2 } fires the cap check after computing the new value, not before. Walking the doubling sequence from 5ms: 5→10→20→40→80→160→320→640→1280ms (because 640 < 1000, so it doubles to 1280; then 1280 >= 1000, so it stops). The maximum sleep is 1280ms, 28% above the apparent 1s intent.
| time.Sleep(backoff) | |
| if backoff < 500*time.Millisecond { | |
| backoff *= 2 | |
| } |
This caps at 1s exactly (640ms doubles to 1280ms → blocked; 320ms doubles to 640ms → allowed). Alternatively, use backoff = min(backoff*2, time.Second) (Go 1.21+, available since this module declares go 1.23).
| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
| // hostIsContainerLocal reports whether a host announced by the provider | ||
| // designates the provider's own host machine (loopback or unspecified) — | ||
| // unreachable under that name from inside a container. | ||
| func hostIsContainerLocal(host string) bool { |
There was a problem hiding this comment.
[low] hostIsContainerLocal uses case-sensitive "localhost" comparison while the compose-side relayUpstream uses strings.EqualFold
relay/main.go normalizes loopback addresses in parseRoutes using hostIsContainerLocal, which checks host == "localhost" (case-sensitive). The compose side in pkg/compose/relay.go:relayUpstream uses strings.EqualFold(host, "localhost"), so it correctly handles "LOCALHOST" or "Localhost".
In the normal flow this gap is unexposed — compose always pre-rewrites upstreams before setting RELAY_ROUTES, so the relay binary never receives an unnormalized "LOCALHOST". However, a user (or a future provider) who sets RELAY_ROUTES manually, or tests the relay image directly, may encounter unexpected behaviour: RELAY_ROUTES=80=LOCALHOST:5432 would silently pass through without rewriting to host.docker.internal, causing dial failures inside the container.
| func hostIsContainerLocal(host string) bool { | |
| func hostIsContainerLocal(host string) bool { | |
| if strings.EqualFold(host, "localhost") { | |
| return true | |
| } | |
| ip := net.ParseIP(host) | |
| return ip != nil && (ip.IsLoopback() || ip.IsUnspecified()) | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 57/100 |
2 participants
A provider's resource lives outside the compose network: consumers could only reach it through injected variables carrying a host-published address. This adds the transparent path:
publish-endpointprovider message:{"type": "publish-endpoint", "message": "80=localhost:49152"}— container port consumers know on the left, real location on the right. Providers express that location naturally from the host's perspective; compose owns the translation to the relay's vantage point, rewriting host-relative addresses (localhost, any loopback IP, unspecified or empty host) tohost.docker.internal— provisioned through ExtraHosts (host-gateway) so it also works on a plain Linux engine. LAN IPs and DNS names pass verbatim.docker/compose-relay(newrelay/directory: static Go TCP forwarder,FROM scratch, bake targetrelay-image,COMPOSE_RELAY_IMAGEoverride for internal registries) in place of the service — canonical<project>-<service>-1name, service alias on the networks of the depending services. Consumers usehttp://<service>:<port>, no injected variables involved.ps,logs,stop,down) keep seeing the service — pluscom.docker.compose.relay(value: identity hash of image+routes) declaring its role:upidempotent: relay kept when routes unchanged, recreated otherwise (reconciler already leaves provider services' containers alone);downtreats the relay as part of the provider service's deprovisioning: its containers are removed before the plugin removes the provider's resource — mirroringup, which provisions the resource before deploying the relay — sodown -vreleases the project network instead of failing on "resource is still in use";exec/cprefuse a relay (no service process to act on);upmonitor excludes relays from the containers whose termination ends the command — they are long-lived infrastructure.This benefits every provider: the example provider demonstrates the flow (detached HTTP helper + publish-endpoint), covered by an e2e scenario asserting
http://dbworks from a consumer,exec dbis refused, anddown -vleaves no relay behind, plus unit tests (message parsing, upstream rewriting, route identity, network selection, relay guard, image override). A provider message compose cannot decode is now reported as an "invalid message" rather than an "invalid response" — providers push messages, they don't answer requests.Standalone — no dependency on #14175 (the two protocol additions are orthogonal).
Publishing note for maintainers:
docker/compose-relayis a new image to publish on Docker Hub under the docker/ organization (bake target included; release wiring to be added to CI).🤖 Generated with Claude Code