diff --git a/docs/extension.md b/docs/extension.md index 9f88d9e15..d3dbd9d85 100644 --- a/docs/extension.md +++ b/docs/extension.md @@ -266,8 +266,12 @@ sequenceDiagram Compose owns the platform knowledge behind the announced address: on a standalone engine it is the relay-link network's IPv4 gateway — an address the provider's host owns on that dedicated bridge, reachable from the relay -(same-bridge local delivery) but joined by nothing else, so never reachable from the LAN or from a project's own -service networks; under Docker Desktop it is `127.0.0.1` — the network lives inside the VM, and the host's own +(same-bridge local delivery) and joined by nothing else: no other container is a member of that network, and none +of the project's service networks includes it. This is isolation by network membership, not network-level +unreachability: the gateway is an address of the host itself, so a local container that can route to it can +still reach a listener bound there by IP. What keeps the endpoint off loopback and off every LAN-facing address +is the provider binding **only** the announced gateway — never a wildcard or any other address. Under Docker +Desktop the announced address is `127.0.0.1` — the network lives inside the VM, and the host's own loopback is, factually, where a host process is reached through the Desktop proxy, so no dedicated network is created there. The provider simply binds the announced gateway and publishes the endpoint exactly as bound: a routable address passes to the relay untouched, a loopback one is announced as `localhost` (translated to diff --git a/pkg/api/labels.go b/pkg/api/labels.go index b85017efa..02d11dc13 100644 --- a/pkg/api/labels.go +++ b/pkg/api/labels.go @@ -53,7 +53,10 @@ const ( // provider-managed service's relay link — the sole channel between the // relay container and the provider's own runtime (see get-relay-info). // Never a project's user-declared network, and never joined by any - // dependent or sibling container: only the relay connects to it. + // dependent or sibling container: only the relay connects to it. That is + // isolation by network membership, not network-level unreachability: the + // gateway is a host address, reachable by IP from any local container + // that can route to it. RelayNetworkLabel = "com.docker.compose.relay-network" // SlugLabel stores unique slug used for one-off container identity SlugLabel = "com.docker.compose.slug" diff --git a/pkg/compose/down.go b/pkg/compose/down.go index 69a704511..30a7ff08c 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -122,7 +122,7 @@ func (s *composeService) down(ctx context.Context, projectName string, options a } ops := s.ensureNetworksDown(ctx, project, limiter) - ops = append(ops, s.ensureRelayLinkNetworksDown(ctx, project)...) + ops = append(ops, s.ensureRelayLinkNetworksDown(ctx, project, limiter)...) if options.Images != "" { ops = append(ops, s.ensureImagesDown(ctx, project, options, limiter)...) diff --git a/pkg/compose/relay.go b/pkg/compose/relay.go index a08b1deac..2decdeea0 100644 --- a/pkg/compose/relay.go +++ b/pkg/compose/relay.go @@ -20,7 +20,6 @@ import ( "context" "crypto/sha256" "encoding/hex" - "errors" "fmt" "net" "os" @@ -34,6 +33,7 @@ import ( "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" "github.com/sirupsen/logrus" + "golang.org/x/sync/semaphore" "github.com/docker/compose/v5/pkg/api" ) @@ -131,15 +131,22 @@ func relayNetworks(project *types.Project, service types.ServiceConfig) []string // on it — its gateway. A provider running its service locally can bind that // address: reachable from the relay (same-bridge local delivery), and — // unlike a project's own bridge networks, which every service on them can -// also reach — joined by nothing else. A single entry, or none when it -// could not be resolved (see ensureRelayLinkNetwork): a provider must treat -// that as "bind elsewhere". +// also reach — joined by nothing else. That is isolation by network +// membership, not network-level unreachability: the gateway is an address +// of the host itself, so a local container able to route to it can still +// reach a listener bound there by IP. Binding ONLY this address is what +// keeps the endpoint off loopback and every LAN-facing address. A single +// entry, or none when it could not be resolved (see ensureRelayLinkNetwork): +// a provider must treat that as "bind elsewhere". type relayInfoAnswer struct { Networks []relayNetworkInfo `json:"networks"` } type relayNetworkInfo struct { - // Name is the concrete engine-level network name. + // Name is the engine-level name of the dedicated relay-link network on a + // standalone engine. Under Docker Desktop it is the placeholder + // "desktop": no such network exists there, so it is only meaningful + // for logging, never as an engine-level network reference. Name string `json:"name"` // Gateway is the address the provider's host owns that the relay can // reach on this network: the dedicated relay-link network's IPv4 @@ -208,13 +215,30 @@ func relayLinkNetworkName(projectName, serviceName string) string { // (a remote resource, e.g. an RDS instance) must never get one conjured // for it just because it happened to publish something. func (s *composeService) findRelayLinkNetwork(ctx context.Context, projectName, serviceName string) (name string, ok bool, err error) { - name = relayLinkNetworkName(projectName, serviceName) - filters := projectFilter(projectName).Add("label", serviceFilter(serviceName)).Add("label", api.RelayNetworkLabel) + existing, err := s.listRelayLinkNetworks(ctx, projectName, serviceName) + if err != nil { + return "", false, err + } + return relayLinkNetworkName(projectName, serviceName), len(existing) > 0, nil +} + +// listRelayLinkNetworks lists the relay-link networks of one provider +// service, or of every provider service of the project when serviceName is +// empty. Selection is by label alone — the one criterion that still holds +// for a project reconstructed from live containers. +func (s *composeService) listRelayLinkNetworks(ctx context.Context, projectName, serviceName string) ([]network.Summary, error) { + filters := projectFilter(projectName).Add("label", api.RelayNetworkLabel) + if serviceName != "" { + filters = filters.Add("label", serviceFilter(serviceName)) + } existing, err := s.apiClient().NetworkList(ctx, client.NetworkListOptions{Filters: filters}) if err != nil { - return "", false, fmt.Errorf("list relay link network for service %s: %w", serviceName, err) + if serviceName == "" { + return nil, fmt.Errorf("list relay link networks for project %s: %w", projectName, err) + } + return nil, fmt.Errorf("list relay link network for service %s: %w", serviceName, err) } - return name, len(existing.Items) > 0, nil + return existing.Items, nil } // ensureRelayLinkNetwork converges the dedicated bridge network one provider @@ -224,7 +248,7 @@ func (s *composeService) findRelayLinkNetwork(ctx context.Context, projectName, // dependents' networks the relay joins to expose the compose-native // alias), nothing else is ever attached to this one — not a dependent, not // a sibling project container — so the answer has exactly one, -// unambiguous, exclusive gateway to give. +// unambiguous gateway to give, on a network nothing else is a member of. // // Called only from relayInfo, in response to the provider's own // get-relay-info request — see findRelayLinkNetwork for why creation must @@ -310,33 +334,47 @@ func (s *composeService) relayLinkNetworkGateway(ctx context.Context, idOrName s // removeRelayLinkNetwork removes a service's relay link network, if any — // the counterpart to ensureRelayLinkNetwork, called wherever the service's // relay itself is torn down (see removeServiceRelay) so the network never -// outlives the relay it exists for. Every caller removes the relay container -// first, so the network is not expected to still have endpoints attached — -// but the daemon disconnects them asynchronously, so a ContainerRemove that -// already returned can still race NetworkRemove here; this tolerates that -// (errdefs.IsConflict) the same way ensureRelayLinkNetworksDown does for the -// down path, besides the network already being gone (errdefs.IsNotFound). +// outlives the relay it exists for. func (s *composeService) removeRelayLinkNetwork(ctx context.Context, projectName, serviceName string) error { - filters := projectFilter(projectName).Add("label", serviceFilter(serviceName)).Add("label", api.RelayNetworkLabel) - existing, err := s.apiClient().NetworkList(ctx, client.NetworkListOptions{Filters: filters}) + existing, err := s.listRelayLinkNetworks(ctx, projectName, serviceName) if err != nil { - return fmt.Errorf("list relay link network for service %s: %w", serviceName, err) + return err } - for _, n := range existing.Items { - if _, err := s.apiClient().NetworkRemove(ctx, n.ID, client.NetworkRemoveOptions{}); err != nil { - if errdefs.IsNotFound(err) { - continue - } - if errdefs.IsConflict(err) { - logrus.Warnf("relay link network %s still has active endpoints, skipping removal", n.Name) - continue - } - return fmt.Errorf("remove relay link network for service %s: %w", serviceName, err) + for _, n := range existing { + if err := s.removeRelayLinkNetworkByID(ctx, n); err != nil { + return err } } return nil } +// removeRelayLinkNetworkByID is the single place a relay-link network is +// removed, shared by the up path (removeRelayLinkNetwork) and the down path +// (ensureRelayLinkNetworksDown). Every caller removes the relay container +// first, but the daemon disconnects its endpoints asynchronously, so a +// ContainerRemove that already returned can still race NetworkRemove: a +// conflict (errdefs.IsConflict) is tolerated, left for a later up/down to +// retry, as is the network already being gone (errdefs.IsNotFound). +func (s *composeService) removeRelayLinkNetworkByID(ctx context.Context, n network.Summary) error { + eventName := "Network " + n.Name + s.events.On(removingEvent(eventName)) + if _, err := s.apiClient().NetworkRemove(ctx, n.ID, client.NetworkRemoveOptions{}); err != nil { + switch { + case errdefs.IsNotFound(err): + s.events.On(newEvent(eventName, api.Warning, "No resource found to remove")) + return nil + case errdefs.IsConflict(err): + s.events.On(newEvent(eventName, api.Warning, "Resource is still in use")) + return nil + default: + s.events.On(errorEvent(eventName, err.Error())) + return fmt.Errorf("remove relay link network %s: %w", n.Name, err) + } + } + s.events.On(removedEvent(eventName)) + return nil +} + // ensureServiceRelay converges the relay container standing in for a provider // service that published endpoints: consumers reach the provider's resource // at the compose-native address (http://:) through it. The @@ -488,40 +526,26 @@ func (s *composeService) ensureRelayNetworks(ctx context.Context, project *types // file takes) never repopulates Provider: nothing in a container's own // labels says its service declared one, so a per-service check here would // silently skip every service and leak the network on every such down — -// the common case, not an edge one. -func (s *composeService) ensureRelayLinkNetworksDown(ctx context.Context, project *types.Project) []downOp { - return []downOp{func() error { - filters := projectFilter(project.Name).Add("label", api.RelayNetworkLabel) - networks, err := s.apiClient().NetworkList(ctx, client.NetworkListOptions{Filters: filters}) - if err != nil { - return fmt.Errorf("list relay link networks for project %s: %w", project.Name, err) - } - var errs []error - for _, n := range networks.Items { - // one project can have several provider services, each with its - // own relay-link network: a transient inspect/remove failure on - // one must not leave the rest unprocessed, unlike a single - // return would. - inspected, err := s.apiClient().NetworkInspect(ctx, n.ID, client.NetworkInspectOptions{}) - if errdefs.IsNotFound(err) { - continue +// the common case, not an edge one. One op per network, each gated on the +// shared limiter like its siblings. +func (s *composeService) ensureRelayLinkNetworksDown(ctx context.Context, project *types.Project, limiter *semaphore.Weighted) []downOp { + networks, err := s.listRelayLinkNetworks(ctx, project.Name, "") + if err != nil { + // surfaced by the op rather than aborting down: the rest of the + // project's cleanup is independent of this lookup + return []downOp{func() error { return err }} + } + ops := make([]downOp, 0, len(networks)) + for _, n := range networks { + ops = append(ops, func() error { + if err := acquireSlot(ctx, limiter); err != nil { + return err } - if err != nil { - errs = append(errs, fmt.Errorf("inspect relay link network %s: %w", n.Name, err)) - continue - } - 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) - continue - } - if _, err := s.apiClient().NetworkRemove(ctx, n.ID, client.NetworkRemoveOptions{}); err != nil && !errdefs.IsNotFound(err) { - errs = append(errs, fmt.Errorf("remove relay link network %s: %w", n.Name, err)) - } - } - return errors.Join(errs...) - }} + defer releaseSlot(limiter) + return s.removeRelayLinkNetworkByID(ctx, n) + }) + } + return ops } // waitRelayRemoved polls until the service's relay container is gone, giving diff --git a/pkg/compose/relay_test.go b/pkg/compose/relay_test.go index c7d8b0910..3f3e05638 100644 --- a/pkg/compose/relay_test.go +++ b/pkg/compose/relay_test.go @@ -28,6 +28,7 @@ import ( "github.com/moby/moby/api/types/network" "github.com/moby/moby/client" "go.uber.org/mock/gomock" + "golang.org/x/sync/semaphore" "gotest.tools/v3/assert" "github.com/docker/compose/v5/pkg/api" @@ -344,6 +345,25 @@ func TestRemoveRelayLinkNetworkToleratesActiveEndpointsConflict(t *testing.T) { assert.NilError(t, svc.removeRelayLinkNetwork(t.Context(), "p", "db")) } +// The network disappearing between the list and the remove (a concurrent +// down) is the desired end state, not a failure. +func TestRemoveRelayLinkNetworkToleratesAlreadyGone(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + apiMock, cli := prepareMocks(mockCtrl) + tested, err := NewComposeService(cli) + assert.NilError(t, err) + svc := tested.(*composeService) + + apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{ + Items: []network.Summary{{Network: network.Network{ID: "net-1", Name: "p_db_relay"}}}, + }, nil) + apiMock.EXPECT().NetworkRemove(gomock.Any(), "net-1", gomock.Any()). + Return(client.NetworkRemoveResult{}, errdefs.ErrNotFound) + + assert.NilError(t, svc.removeRelayLinkNetwork(t.Context(), "p", "db")) +} + // ensureRelayLinkNetworksDown removes every provider service's relay link // network on a full `down` — the path relay container removal itself // already takes generically (matched by ServiceLabel), but that a project's @@ -377,21 +397,52 @@ func TestEnsureRelayLinkNetworksDown(t *testing.T) { }).Return(client.NetworkListResult{ Items: []network.Summary{{Network: network.Network{ID: "net-1", Name: "p_db_relay"}}}, }, nil) - apiMock.EXPECT().NetworkInspect(gomock.Any(), "net-1", gomock.Any()). - Return(client.NetworkInspectResult{Network: network.Inspect{Network: network.Network{Name: "p_db_relay"}}}, nil) apiMock.EXPECT().NetworkRemove(gomock.Any(), "net-1", gomock.Any()).Return(client.NetworkRemoveResult{}, nil) - ops := svc.ensureRelayLinkNetworksDown(t.Context(), project) + ops := svc.ensureRelayLinkNetworksDown(t.Context(), project, nil) assert.Equal(t, len(ops), 1) for _, op := range ops { assert.NilError(t, op()) } } +// With no relay-link network there is no op at all, so down's "No resource +// found to remove" warning stays reachable for a project with nothing else. +func TestEnsureRelayLinkNetworksDownNoNetworkNoOp(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + apiMock, cli := prepareMocks(mockCtrl) + tested, err := NewComposeService(cli) + assert.NilError(t, err) + svc := tested.(*composeService) + + apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{}, nil) + + assert.Equal(t, len(svc.ensureRelayLinkNetworksDown(t.Context(), &types.Project{Name: "p"}, nil)), 0) +} + +// A failing lookup must not abort down's other cleanup: it comes back as an +// op carrying the error, run alongside the rest. +func TestEnsureRelayLinkNetworksDownSurfacesListError(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + apiMock, cli := prepareMocks(mockCtrl) + tested, err := NewComposeService(cli) + assert.NilError(t, err) + svc := tested.(*composeService) + + apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()). + Return(client.NetworkListResult{}, errors.New("daemon unavailable")) + + ops := svc.ensureRelayLinkNetworksDown(t.Context(), &types.Project{Name: "p"}, nil) + assert.Equal(t, len(ops), 1) + assert.ErrorContains(t, ops[0](), "daemon unavailable") +} + // A network whose relay container removal hasn't been reflected by the // daemon's async disconnect yet must be left in place, not fail the whole // down — a later down retries and finds it gone. -func TestEnsureRelayLinkNetworksDownSkipsStillInUse(t *testing.T) { +func TestEnsureRelayLinkNetworksDownToleratesStillInUse(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() apiMock, cli := prepareMocks(mockCtrl) @@ -399,30 +450,20 @@ func TestEnsureRelayLinkNetworksDownSkipsStillInUse(t *testing.T) { assert.NilError(t, err) svc := tested.(*composeService) - project := &types.Project{Name: "p"} - apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{ Items: []network.Summary{{Network: network.Network{ID: "net-1", Name: "p_db_relay"}}}, }, nil) - apiMock.EXPECT().NetworkInspect(gomock.Any(), "net-1", gomock.Any()).Return(client.NetworkInspectResult{ - Network: network.Inspect{ - Network: network.Network{Name: "p_db_relay"}, - Containers: map[string]network.EndpointResource{ - "relay-1": {Name: "p-db-1"}, - }, - }, - }, nil) + apiMock.EXPECT().NetworkRemove(gomock.Any(), "net-1", gomock.Any()). + Return(client.NetworkRemoveResult{}, conflictError{}) - ops := svc.ensureRelayLinkNetworksDown(t.Context(), project) + ops := svc.ensureRelayLinkNetworksDown(t.Context(), &types.Project{Name: "p"}, nil) assert.Equal(t, len(ops), 1) - for _, op := range ops { - assert.NilError(t, op()) - } + assert.NilError(t, ops[0]()) } -// A transient inspect failure on one project's relay-link network must not -// stop the others from being cleaned up: each is independent. -func TestEnsureRelayLinkNetworksDownContinuesPastInspectError(t *testing.T) { +// One op per network: a failure removing one relay-link network must not +// stop the others from being cleaned up. +func TestEnsureRelayLinkNetworksDownOpsAreIndependent(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() apiMock, cli := prepareMocks(mockCtrl) @@ -430,25 +471,45 @@ func TestEnsureRelayLinkNetworksDownContinuesPastInspectError(t *testing.T) { assert.NilError(t, err) svc := tested.(*composeService) - project := &types.Project{Name: "p"} - apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{ Items: []network.Summary{ {Network: network.Network{ID: "net-1", Name: "p_db_relay"}}, {Network: network.Network{ID: "net-2", Name: "p_cache_relay"}}, }, }, nil) - apiMock.EXPECT().NetworkInspect(gomock.Any(), "net-1", gomock.Any()). - Return(client.NetworkInspectResult{}, errors.New("transient daemon error")) - apiMock.EXPECT().NetworkInspect(gomock.Any(), "net-2", gomock.Any()). - Return(client.NetworkInspectResult{Network: network.Inspect{Network: network.Network{Name: "p_cache_relay"}}}, nil) + apiMock.EXPECT().NetworkRemove(gomock.Any(), "net-1", gomock.Any()). + Return(client.NetworkRemoveResult{}, errors.New("transient daemon error")) apiMock.EXPECT().NetworkRemove(gomock.Any(), "net-2", gomock.Any()).Return(client.NetworkRemoveResult{}, nil) - ops := svc.ensureRelayLinkNetworksDown(t.Context(), project) + ops := svc.ensureRelayLinkNetworksDown(t.Context(), &types.Project{Name: "p"}, nil) + assert.Equal(t, len(ops), 2) + assert.ErrorContains(t, ops[0](), "transient daemon error") + assert.NilError(t, ops[1]()) +} + +// Like its siblings, every relay-link network removal gates itself on the +// shared down limiter: with no slot available, the op never reaches the +// engine. +func TestEnsureRelayLinkNetworksDownHonorsLimiter(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + apiMock, cli := prepareMocks(mockCtrl) + tested, err := NewComposeService(cli) + assert.NilError(t, err) + svc := tested.(*composeService) + + apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{ + Items: []network.Summary{{Network: network.Network{ID: "net-1", Name: "p_db_relay"}}}, + }, nil) + + limiter := semaphore.NewWeighted(1) + assert.NilError(t, limiter.Acquire(t.Context(), 1)) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + ops := svc.ensureRelayLinkNetworksDown(ctx, &types.Project{Name: "p"}, limiter) assert.Equal(t, len(ops), 1) - for _, op := range ops { - assert.ErrorContains(t, op(), "transient daemon error") - } + assert.ErrorIs(t, ops[0](), context.Canceled) } // ensureServiceRelay runs concurrently per provider service under the shared @@ -635,7 +696,9 @@ func TestEnsureServiceRelayRemovesExistingWhenNoDependents(t *testing.T) { }, nil) apiMock.EXPECT().ContainerRemove(gomock.Any(), "relay-1", client.ContainerRemoveOptions{Force: true}). Return(client.ContainerRemoveResult{}, nil) - apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{}, nil) + apiMock.EXPECT().NetworkList(gomock.Any(), client.NetworkListOptions{ + Filters: projectFilter("p").Add("label", serviceFilter("db")).Add("label", api.RelayNetworkLabel), + }).Return(client.NetworkListResult{}, nil) err = svc.ensureServiceRelay(t.Context(), project, db, endpoints, nil) assert.NilError(t, err) @@ -671,7 +734,9 @@ func TestRemoveServiceRelayRemovesExisting(t *testing.T) { Return(client.ContainerRemoveResult{}, nil) // removeServiceRelay always also tries the relay-link network, even // when nothing is left connected to it. - apiMock.EXPECT().NetworkList(gomock.Any(), gomock.Any()).Return(client.NetworkListResult{}, nil) + apiMock.EXPECT().NetworkList(gomock.Any(), client.NetworkListOptions{ + Filters: projectFilter("p").Add("label", serviceFilter("db")).Add("label", api.RelayNetworkLabel), + }).Return(client.NetworkListResult{}, nil) assert.NilError(t, svc.removeServiceRelay(t.Context(), "p", "db")) }