feat(provider): publish-endpoint deploys a network relay for provider services - #14193
Conversation
ee1f177 to
243b41f
Compare
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
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
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
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
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
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 |
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
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 |
The relay image gets the same pipeline as docker/compose-bin, through the shared github-builder bake workflow: - merge.yml publishes docker/compose-relay on every push — `edge` from main, and the `v1` tag consumers actually pull only on compose release tags: v1 is the image CONTRACT major, not a compose version, and moving it only at releases keeps what users pull predictable; - ci.yml validates the cross build on every PR (push: false), like bin-image-test does; - the bake file splits relay-image (local/dev build, default tag) from relay-image-cross (CI target: meta-helper-driven tags, every linux platform the compose binary ships for — the relay runs as a container on the engine, so darwin/windows make no sense for it). The docker/compose-relay Docker Hub repository itself must exist and accept pushes from the org registry identity — a one-time maintainer step, called out in the PR description. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 1a53da7 (the previous review checkpoint). Earlier commits were already reviewed.
Assessment: 🟢 APPROVE
The incremental diff adds a single docker-bake.hcl change: the existing relay-image target (which previously combined tags and platforms into one definition) is split into two:
relay-image— local/dev build, tagdocker/compose-relay:v1, uses host platform by default (no explicitplatformslist). This is the correct pattern for local development builds.relay-image-cross— CI publication target, inherits frommeta-helper(for workflow-supplied tags/labels), covers all seven Linux platforms that the compose binary itself ships for (amd64,arm/v6,arm/v7,arm64,ppc64le,riscv64,s390x). The omission of darwin/windows is correct and documented in the comment (the relay runs as a container on the engine).
The output = ["type=image"] line in relay-image-cross is correct: the reusable docker/github-builder bake workflow receives push: true in merge.yml and push: false in ci.yml (PR builds), and manages push externally — Docker Buildx's --push flag overrides/supplements the bake target's output field. This is consistent with how image-cross and other cross-compilation targets are handled in this repository.
No issues introduced by this change.
…point The 'Connection to a service' section only covered environment-variable injection, while the relay mechanism was buried in the message-type reference list. Move the relay narrative into its own subsection there, illustrated with a sequence diagram showing the full path — the provider publishing the endpoint, Compose deploying the relay on the dependents' networks, and the application connecting at the compose-native address. The publish-endpoint entry in the message list keeps the wire-format contract and points to the new section. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits since 32f8767abdf6.
Assessment: 🟢 APPROVE
This incremental diff is documentation-only: it reorganises the publish-endpoint description in docs/extension.md into a dedicated section ("Compose-native addressing with publish-endpoint") with a Mermaid sequence diagram, and updates the inline listing to link to that section. The new prose is accurate, the anchor link matches the section heading, the sequence diagram correctly depicts the provider→Compose message flow and the relay deployment, and all relay behaviour details (loopback→host.docker.internal translation, host-gateway, container labels, exec/cp refusal, recreation on endpoint change) are faithfully described in the new section.
glours
left a comment
There was a problem hiding this comment.
Worth checking:
compose stop on a provider service short-circuits straight into runPlugin(ctx, project, serv, "stop") (pkg/compose/stop.go:55-59) without ever calling stopContainers, this file isn't touched by this PR.
Since this PR gives a provider service a real, persistent relay container (RestartPolicyUnlessStopped), compose stop now leaves that relay running and still Up in compose ps, contradicting docs/extension.md's claim that stop treats the relay like the service.
down.go's new downService got the relay-aware fix (stop/remove containers before invoking the plugin); stop.go needs the same treatment but wasn't updated in this PR.
`compose stop` on a provider service short-circuited straight into runPlugin(..., "stop"), never touching stopContainers -- so the relay container this PR gives a provider service (RestartPolicyUnlessStopped, a real persistent container) was left running and still Up in `compose ps`, contradicting docs/extension.md's claim that stop treats the relay like the service. downService already stops/removes a provider's containers before invoking the plugin; stop.go gets the same treatment: the service's containers are stopped unconditionally, then the plugin's own stop hook (if any) runs for provider services. start/restart needed no change -- neither special-cases providers, so they already operate on whatever containers match the service filter, relay included. Verified locally: reverting just this change makes the new e2e steps below fail on "stop halts the relay container". Extends TestProviderPublishEndpoint with stop/start/restart coverage of the relay container. (glours review on docker#14193) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Good catch — fixed in af10501: |
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 5f875dd.
Assessment: 🟢 APPROVE
The single-file change in this incremental diff correctly reorders stopContainers before runPlugin for provider services in pkg/compose/stop.go, mirroring the established pattern in down.go. Error propagation is correct: stopContainers errors return immediately, and runPlugin errors are returned via the explicit return s.runPlugin(...) call. No logic errors, resource leaks, nil-dereferences, or other issues found.
glours
left a comment
There was a problem hiding this comment.
Fixes for potential follow up PRs
-
Relay never reconnects to new networks when routes are unchanged (
pkg/compose/relay.go:93-96relayIdentity,:140-197ensureServiceRelay) — identity hashes only image+routes, not network topology. When an already-running relay's identity still matches,ensureServiceRelayreturns early without checking whethernetworkKeysincludes a network the relay isn't attached to yet. A dependent service added on a new network after the relay is already up can never reach the provider through it, silently. -
Stale relay is never cleaned up if the provider stops publishing endpoints (
pkg/compose/plugins.go:101,126-134,pkg/compose/reconcile.go:638-654),deployRelayis only evaluated when endpoints are non-empty; if a laterupreports zero endpoints, nothing removes or revalidates the existing relay. The generic reconciler doesn't catch it either: it returns immediately for anyservice.Provider != nilbefore reaching the observed-state/recreate logic that would otherwise clean it up. -
start/restartdon't guard against acting on the relay container (pkg/compose/service_containers.go:619-641,pkg/compose/restart.go:109-131) — unlikeexec.go/cp.go, which callcheckRelayTargetto refuse a relay,startServiceContainer/restartContainerunconditionally inject secrets/configs and runPostStart/PreStophooks against any container matchingisService(name), relay included. Compose-spec schema doesn't forbidsecrets/configs/hooks on aprovider:service, so this is reachable. -
No goroutine-leak test coverage for the new relay code (
relay/main_test.go) — the module spawns several goroutines (accept loop, per-connection forward pairs, idle-reap) but has nogoleakcheck, despitegoleakbeing an established convention elsewhere in the repo (e.g.pkg/compose/pre_start_test.go). -
The
relay/Go module isn't wired into CI lint or test — it's a separate module (relay/go.mod), sogolangci-lint run ./...andgo test ./...from the repo root never touch it; CI only doesgo buildinsiderelay/Dockerfile. Practically,relay/main_test.go's two regression tests locking the idle-reap/half-close fix never run in CI, and runninggolangci-lintmanually againstrelay/surfaces 8 real issues (5 uncheckedClose()errors, one%v→%w, oneexitAfterDefer, onestaticcheckhit).
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: the CI wiring is included —
merge.ymlpublishesdocker/compose-relaythrough the same github-builder pipeline asdocker/compose-bin(edgefrom main; thev1tag consumers pull moves only on compose release tags, since v1 is the image contract major, not a compose version), and PRs validate the cross build (relay-image-test). The one remaining step is org-side: create thedocker/compose-relayDocker Hub repository and allow the existing registry identity to push to it.🤖 Generated with Claude Code