Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 4b8105ea10ec.
Assessment: 🟡 NEEDS ATTENTION
Two medium-confidence findings in the new race-condition mitigations added to ensureRelayLinkNetworksDown. The relayInfo name-guard and the ensureRelayLinkNetwork NotFound switch look correct.
| if len(inspected.Network.Containers) > 0 { | ||
| // the daemon's async disconnect of the just-removed relay | ||
| // container hasn't caught up yet; a later down retries | ||
| logrus.Warnf("relay link network %s is still in use, skipping removal", n.Name) |
There was a problem hiding this comment.
[medium] ensureRelayLinkNetworksDown skips network removal with no in-process retry when containers are still attached
When inspected.Network.Containers is non-empty, the new code logs a warning and continues, deferring cleanup to a future down invocation. This is safe for the expected transient case — the daemon's async disconnect of the just-removed relay container — but the loop has no bounded retry or polling timeout to confirm the disconnect actually completes. If any container (not necessarily the relay) remains attached to the internal:true relay-link network — even due to a daemon bug or a mistaken docker network connect — every subsequent down call will silently skip removal with the same warning and no escalation path. The user sees repeated warnings but the orphaned network is never cleaned up automatically.
The comment says "a later down retries," which is accurate for normal operation, but there is no mechanism inside the function to distinguish a transient disconnect lag (milliseconds) from a stuck container (permanent), so the skip is open-ended rather than bounded.
Consider replacing the skip with a short poll (e.g. 3 attempts × 500 ms) for the container list to drain before giving up, or at minimum changing the log from Warn to a more prominent message that includes actionable guidance (docker network disconnect <network> <container>).
| Confidence | Score |
|---|---|
| 🟡 moderate | 62/100 |
There was a problem hiding this comment.
Not adding a bounded retry/poll here: this mirrors removeNetwork's existing skip-and-warn behavior for regular project networks exactly (also open-ended, also deferred to the next down), so it's consistent with the codebase's existing convention rather than a new gap this PR introduces. A container that never disconnects (daemon bug, stray manual network connect) is an operational anomaly outside what either code path currently handles — happy to revisit if that's a real risk worth carrying in this PR, deferring to @ndeloof on whether it's worth the added complexity here.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This review covers only the commits pushed since
0e2129a6a289d85124a5ff05866656262910452b.
The incremental diff is focused and correct. ensureRelayLinkNetworksDown now accumulates errors with errors.Join instead of returning early, so a transient NetworkInspect failure on one relay-link network no longer blocks cleanup of the remaining networks. Key correctness points:
errors.Join(nil)/errors.Join()returnsnil, so the no-error case is handled correctly — no spurious errors.- The
continueafter appending an inspect error correctly skips theNetworkRemovecall for the failed network, preserving the safe-by-default posture. - The new test
TestEnsureRelayLinkNetworksDownContinuesPastInspectErrorexercises the exact failure path: net-1 inspect fails (accumulated), net-2 inspect succeeds and is removed, the single returned op() surfaces the inspect error. The assertionlen(ops) == 1is correct becauseensureRelayLinkNetworksDownalways returns a one-element[]downOpslice.
No bugs found in the added code.
822c706 to
989e55d
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
- [low] pkg/compose/relay.go:197 —
relayLinkNetworkNameformat{project}_{service}_relaycan collide with a user-declared network key{service}_relay(confidence: weak 30/100)
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 989e55d6a3df.
Assessment: 🟢 APPROVE
The two changes in this incremental diff are both correct:
-
ensureRelayLinkNetworkconflict path — AfterNetworkCreatereturnserrdefs.IsConflict, the code now re-validates ownership viafindRelayLinkNetwork(label-filtered) before proceeding. If no labeled relay network is found, it fails with a descriptive error rather than silently adopting an unrelated same-named network. The if/else chain is logically sound: the shadowederrinside the:=is idiomatic Go, and the error message correctly uses the in-scopenamevariable. -
removeRelayLinkNetworkIsConflict tolerance — Tolerateerrdefs.IsConflictonNetworkRemovewith warn+skip, matchingensureRelayLinkNetworksDown's existing behavior. The Docker daemon disconnects containers asynchronously, so a brief conflict window afterContainerRemoveis expected and transient. Any non-transient orphan would be caught by a subsequentdowncall viaensureRelayLinkNetworksDown, which lists by the same labels. This is consistent with the codebase-level convention used byremoveNetworkfor regular project networks.
e601e7a to
659a85b
Compare
get-relay-info answers pointed a locally-bound provider at one of the dependents' own networks: shared infrastructure every other service on it could also reach, and no more isolated than any project bridge. Provision a dedicated, internal:true bridge network scoped to that one provider service instead, created only when the provider itself sends get-relay-info -- the provider's own declaration that it binds locally, never for a provider backing a remote resource. The relay container joins it alongside the dependents' networks it already joins; it is torn down with the relay on endpoint changes and on down, including the no-compose-file down path. The network's lifecycle is hardened against the races a shared, project-scoped resource runs into: - a NetworkCreate conflict on the deterministic name re-checks by label (not just by name) before adopting the existing network, so a genuine name clash with an unrelated, user-declared network fails loudly instead of being silently adopted into the relay's isolation boundary; - a NotFound gateway lookup after losing that conflict falls back to an unresolved answer instead of propagating the error; - removeRelayLinkNetwork and ensureRelayLinkNetworksDown both tolerate a NetworkRemove conflict from the daemon's asynchronous endpoint disconnect, which can race the relay container's own removal; - ensureRelayLinkNetworksDown accumulates failures with errors.Join across every relay-link network of a project instead of aborting on the first one, and skips (warn-only) a network that still has active endpoints, mirroring removeNetwork's existing tolerance for regular project networks. docs/extension.md documents that the "desktop" network name in the get-relay-info answer (under Docker Desktop) is an opaque placeholder, never a real Docker network a provider can inspect or connect to. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
659a85b to
4dbd41a
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Compose now gives each locally-bound provider service its own dedicated bridge network for the relay link, instead of relying on the provider guessing a bind address that happens to work.
Context
When a provider service publishes an endpoint that runs on the provider's own host (not a remote resource), Compose deploys a relay container in front of it so dependents keep connecting to the compose-native
<service>:<port>address. That relay has to reach the endpoint the provider bound — but on a standalone Linux engine, no address is both relay-reachable and off the LAN by default:host.docker.internalresolves to a project bridge's gateway, which every other container on that bridge can also reach, and the wildcard exposes the port on every host interface.get-relay-infoalready let the provider ask Compose which gateway to bind, but the answer pointed at one of the dependents' own networks — shared infrastructure the provider's endpoint had no business sitting on.What the PR brings
get-relay-inforequest now provisions (or reuses) a dedicated,internal:truebridge network scoped to that one provider service — joined by the relay container and nothing else — and answers with its name and gateway.get-relay-infois itself the provider's declaration that it binds locally: a provider backing a remote resource (an Amazon RDS instance, say) never sends it, so no network is ever created on its behalf. The network is created lazily, from the request, never speculatively from a published endpoint alone.down, including the "no compose file"down --project-namepath where per-service provider metadata can't be reconstructed from container labels alone.sequenceDiagram participant Compose participant Provider participant net as relay-link network<br/>(internal, per-service) participant relay as relay container rect rgb(235, 245, 255) note over Compose,net: Provider running its service locally Compose->>Provider: compose up --project-name=xx "database" Provider->>Compose: json { "type": "get-relay-info" } Compose->>net: create (or reuse) the dedicated<br/>relay-link network for "database" Compose--)Provider: json {"networks":[{"name":"myproject_database_relay",<br/>"gateway":"172.20.0.1"}]} Provider->>Provider: bind local endpoint to 172.20.0.1 Provider--)Compose: json { "type": "publish-endpoint",<br/>"message": "80=172.20.0.1:49152" } Compose->>relay: deploy, join dependents' networks<br/>AND the relay-link network endsequenceDiagram participant Compose participant Provider participant resource as remote resource<br/>(e.g. Amazon RDS) participant relay as relay container rect rgb(255, 245, 235) note over Compose,resource: Provider backing a remote resource Compose->>Provider: compose up --project-name=xx "database" Provider->>resource: provision note over Provider: no local endpoint to bind:<br/>get-relay-info is never sent Provider--)Compose: json { "type": "publish-endpoint",<br/>"message": "80=resource.example.com:5432" } Compose->>relay: deploy, join dependents' networks only<br/>(no relay-link network created) endThe two flows share the same relay deployment and
publish-endpointcontract; only whetherget-relay-infois ever sent decides whether a relay-link network exists at all.🤖 Generated with Claude Code