diff --git a/daemon/libnetwork/service.go b/daemon/libnetwork/service.go index 0cb8baea24..8194ac6486 100644 --- a/daemon/libnetwork/service.go +++ b/daemon/libnetwork/service.go @@ -47,9 +47,6 @@ type service struct { // List of ingress ports exposed by the service ingressPorts portConfigs - // Service aliases - aliases []string - // This maps tracks for each IP address the list of endpoints ID // associated with it. At stable state the endpoint ID expected is 1 // but during transition and service change it is possible to have @@ -82,6 +79,11 @@ func (s *service) printIPToEndpoint(ip string) (string, bool) { type lbBackend struct { ip net.IP disabled bool + // aliases is the per-task service alias list this backend was registered + // with. Stored so rmServiceBinding can decrement the matching counts in + // service.aliasRefs even when the caller-supplied list has drifted (e.g. + // during cleanupServiceBindings). + aliases []string } type loadBalancer struct { @@ -92,6 +94,16 @@ type loadBalancer struct { // network. It is keyed with endpoint ID. backEnds map[string]*lbBackend + // aliasRefs counts how many backends reference each service alias on + // this network. A VIP DNS record for an alias is created on the 0→1 + // transition and removed on the 1→0 transition, so aliases survive + // rolling updates as long as any old or new task still claims them. + // + // The map is keyed by alias. It lives on loadBalancer (one per network + // the service is attached to) because a service can configure different + // alias sets on each of its networks. + aliasRefs map[string]int + // Back pointer to service to which the loadbalancer belongs. service *service sync.Mutex diff --git a/daemon/libnetwork/service_common.go b/daemon/libnetwork/service_common.go index 65d0a7ca8c..92f6f4d8b2 100644 --- a/daemon/libnetwork/service_common.go +++ b/daemon/libnetwork/service_common.go @@ -5,6 +5,7 @@ package libnetwork import ( "context" "net" + "slices" "github.com/containerd/log" ) @@ -46,9 +47,9 @@ func (c *Controller) addEndpointNameResolution(svcName, svcID, nID, eID, contain if addService && len(vip) != 0 { n.addSvcRecords(eID, svcName, serviceID, vip, nil, false, method) - for _, alias := range serviceAliases { - n.addSvcRecords(eID, alias, serviceID, vip, nil, false, method) - } + // VIP records for service aliases are managed by addServiceBinding + // via service.aliasRefs, not here, so they survive rolling updates + // where aliases can change between tasks. } return nil @@ -110,9 +111,8 @@ func (c *Controller) deleteEndpointNameResolution(svcName, svcID, nID, eID, cont // Remove the DNS record for VIP only if we are removing the service if rmService && len(vip) != 0 && !multipleEntries { n.deleteSvcRecords(eID, svcName, serviceID, vip, nil, false, method) - for _, alias := range serviceAliases { - n.deleteSvcRecords(eID, alias, serviceID, vip, nil, false, method) - } + // VIP records for service aliases are managed by rmServiceBinding + // via service.aliasRefs, not here. } return nil @@ -136,13 +136,12 @@ func (c *Controller) delContainerNameResolution(nID, eID, containerName string, return nil } -func newService(name string, id string, ingressPorts []*PortConfig, serviceAliases []string) *service { +func newService(name string, id string, ingressPorts []*PortConfig) *service { return &service{ name: name, id: id, ingressPorts: ingressPorts, loadBalancers: make(map[string]*loadBalancer), - aliases: serviceAliases, } } @@ -207,7 +206,7 @@ func (c *Controller) cleanupServiceBindings(cleanupNID string) { continue } for eid, be := range lb.backEnds { - cleanupFuncs = append(cleanupFuncs, makeServiceCleanupFunc(c, s, nid, eid, lb.vip, be.ip)) + cleanupFuncs = append(cleanupFuncs, makeServiceCleanupFunc(c, s, nid, eid, lb.vip, be.ip, be.aliases)) } } s.Unlock() @@ -218,12 +217,12 @@ func (c *Controller) cleanupServiceBindings(cleanupNID string) { } } -func makeServiceCleanupFunc(c *Controller, s *service, nID, eID string, vip net.IP, ip net.IP) func() { +func makeServiceCleanupFunc(c *Controller, s *service, nID, eID string, vip net.IP, ip net.IP, aliases []string) func() { // ContainerName and taskAliases are not available here, this is still fine because the Service discovery // cleanup already happened before. The only thing that rmServiceBinding is still doing here a part from the Load // Balancer bookkeeping, is to keep consistent the mapping of endpoint to IP. return func() { - if err := c.rmServiceBinding(s.name, s.id, nID, eID, "", vip, s.ingressPorts, s.aliases, []string{}, ip, "cleanupServiceBindings", false, true); err != nil { + if err := c.rmServiceBinding(s.name, s.id, nID, eID, "", vip, s.ingressPorts, aliases, []string{}, ip, "cleanupServiceBindings", false, true); err != nil { log.G(context.TODO()).Errorf("Failed to remove service bindings for service %s network %s endpoint %s while cleanup: %v", s.id, nID, eID, err) } } @@ -257,7 +256,7 @@ func (c *Controller) addServiceBinding(svcName, svcID, nID, eID, containerName s if !ok { // Create a new service if we are seeing this service // for the first time. - s = newService(svcName, svcID, ingressPorts, serviceAliases) + s = newService(svcName, svcID, ingressPorts) c.serviceBindings[skey] = s } c.mu.Unlock() @@ -279,10 +278,11 @@ func (c *Controller) addServiceBinding(svcName, svcID, nID, eID, containerName s fwMarkCtrMu.Lock() lb = &loadBalancer{ - vip: vip, - fwMark: fwMarkCtr, - backEnds: make(map[string]*lbBackend), - service: s, + vip: vip, + fwMark: fwMarkCtr, + backEnds: make(map[string]*lbBackend), + aliasRefs: make(map[string]int), + service: s, } fwMarkCtr++ @@ -292,7 +292,38 @@ func (c *Controller) addServiceBinding(svcName, svcID, nID, eID, containerName s addService = true } - lb.backEnds[eID] = &lbBackend{ip, false} + // Diff the task's aliases against the ones previously recorded for + // this endpoint. addServiceBinding can be re-invoked for the same eID + // (re-enabling a disabled backend, or a remote endpoint event whose + // alias set has changed); only the delta should adjust ref counts. + // On a 0→1 transition, a VIP DNS record is added on this network's LB; + // on a 1→0 transition it is removed. This keeps aliases registered for + // as long as any task — old or new — still claims them on this network. + var prevAliases []string + if existing, ok := lb.backEnds[eID]; ok { + prevAliases = existing.aliases + } + if len(lb.vip) != 0 { + for _, alias := range serviceAliases { + if !slices.Contains(prevAliases, alias) { + lb.aliasRefs[alias]++ + if lb.aliasRefs[alias] == 1 { + n.addSvcRecords(eID, alias, svcID, lb.vip, nil, false, "addServiceBinding") + } + } + } + for _, alias := range prevAliases { + if !slices.Contains(serviceAliases, alias) { + lb.aliasRefs[alias]-- + if lb.aliasRefs[alias] == 0 { + delete(lb.aliasRefs, alias) + n.deleteSvcRecords(eID, alias, svcID, lb.vip, nil, false, "addServiceBinding") + } + } + } + } + + lb.backEnds[eID] = &lbBackend{ip: ip, aliases: slices.Clone(serviceAliases)} ok, entries := s.assignIPToEndpoint(ip.String(), eID) if !ok || entries > 1 { @@ -350,6 +381,24 @@ func (c *Controller) rmServiceBinding(svcName, svcID, nID, eID, containerName st if fullRemove { // delete regardless delete(lb.backEnds, eID) + // Drop this backend's contribution from the per-alias ref counts. + // On a 1→0 transition, remove the VIP DNS record on this network's + // LB. The network handle may be gone (e.g. when the network has + // already been deleted from the controller); in that case the + // resolver state is torn down with the network and there is + // nothing to do here. + if len(lb.vip) != 0 && len(be.aliases) > 0 { + n, err := c.NetworkByID(nID) + for _, alias := range be.aliases { + lb.aliasRefs[alias]-- + if lb.aliasRefs[alias] == 0 { + delete(lb.aliasRefs, alias) + if err == nil { + n.deleteSvcRecords(eID, alias, svcID, lb.vip, nil, false, "rmServiceBinding") + } + } + } + } } else { be.disabled = true } diff --git a/daemon/libnetwork/service_common_unix_test.go b/daemon/libnetwork/service_common_unix_test.go index 8c1c4585d1..6a91ec169a 100644 --- a/daemon/libnetwork/service_common_unix_test.go +++ b/daemon/libnetwork/service_common_unix_test.go @@ -9,6 +9,7 @@ import ( "github.com/moby/moby/v2/daemon/libnetwork/config" "github.com/moby/moby/v2/daemon/libnetwork/ipamutils" + "github.com/moby/moby/v2/daemon/libnetwork/types" "github.com/moby/moby/v2/internal/testutil/netnsutils" "gotest.tools/v3/assert" ) @@ -56,3 +57,107 @@ func TestCleanupServiceDiscovery(t *testing.T) { t.Fatalf("Service record not cleaned correctly:%v", c.svcRecords) } } + +// TestServiceAliasRefCounting exercises the per-network alias ref-counting +// in addServiceBinding/rmServiceBinding. It verifies: +// 1. A VIP DNS record for an alias survives a rolling update as long as +// any task on that network still claims it, and disappears once the +// last task is removed. +// 2. Aliases are scoped per-network: configuring an alias on network A +// does not publish a VIP DNS record for it on network B, or vice +// versa. +func TestServiceAliasRefCounting(t *testing.T) { + defer netnsutils.SetupTestOSContext(t)() + ctx := context.Background() + + c, err := New(ctx, config.OptionDataDir(t.TempDir()), + config.OptionDefaultAddressPoolConfig(ipamutils.GetLocalScopeDefaultNetworks())) + assert.NilError(t, err) + defer c.Stop() + + n1, err := c.NewNetwork(ctx, "bridge", "net1", "", NetworkOptionEnableIPv4(true)) + assert.NilError(t, err) + defer func() { _ = n1.Delete() }() + + n2, err := c.NewNetwork(ctx, "bridge", "net2", "", NetworkOptionEnableIPv4(true)) + assert.NilError(t, err) + defer func() { _ = n2.Delete() }() + + const ( + svcName = "svc" + svcID = "svcid" + ) + vip1 := net.ParseIP("10.0.0.1") + vip2 := net.ParseIP("10.0.0.2") + + resolves := func(t *testing.T, n *Network, name string) bool { + t.Helper() + ips, _ := n.ResolveName(ctx, name, types.IPv4) + return len(ips) > 0 + } + + t.Run("rolling update preserves alias until last task leaves", func(t *testing.T) { + // Old task with alias "old". + assert.NilError(t, c.addServiceBinding(svcName, svcID, n1.ID(), "ep-old", "ctr-old", + vip1, nil, []string{"old"}, nil, net.ParseIP("172.20.0.10"), "test")) + assert.Check(t, resolves(t, n1, "old"), "alias 'old' should resolve after first task") + + // New task with alias "new" arrives mid rolling-update. + assert.NilError(t, c.addServiceBinding(svcName, svcID, n1.ID(), "ep-new", "ctr-new", + vip1, nil, []string{"new"}, nil, net.ParseIP("172.20.0.11"), "test")) + assert.Check(t, resolves(t, n1, "old"), "'old' must persist while old task still references it") + assert.Check(t, resolves(t, n1, "new"), "'new' must resolve once new task is bound") + + // Old task gone — only "new" should remain. + assert.NilError(t, c.rmServiceBinding(svcName, svcID, n1.ID(), "ep-old", "ctr-old", + vip1, nil, []string{"old"}, nil, net.ParseIP("172.20.0.10"), "test", true, true)) + assert.Check(t, !resolves(t, n1, "old"), "'old' must be gone once last referencing task is removed") + assert.Check(t, resolves(t, n1, "new"), "'new' must still resolve") + + // New task gone — service should be empty. + assert.NilError(t, c.rmServiceBinding(svcName, svcID, n1.ID(), "ep-new", "ctr-new", + vip1, nil, []string{"new"}, nil, net.ParseIP("172.20.0.11"), "test", true, true)) + assert.Check(t, !resolves(t, n1, "new")) + }) + + t.Run("aliases are scoped per network", func(t *testing.T) { + // Same service, different aliases per network. + assert.NilError(t, c.addServiceBinding(svcName, svcID, n1.ID(), "ep-net1", "ctr-1", + vip1, nil, []string{"only-on-net1"}, nil, net.ParseIP("172.20.0.20"), "test")) + assert.NilError(t, c.addServiceBinding(svcName, svcID, n2.ID(), "ep-net2", "ctr-2", + vip2, nil, []string{"only-on-net2"}, nil, net.ParseIP("172.21.0.20"), "test")) + + assert.Check(t, resolves(t, n1, "only-on-net1"), "alias should be on net1") + assert.Check(t, !resolves(t, n2, "only-on-net1"), "alias must NOT leak to net2") + assert.Check(t, resolves(t, n2, "only-on-net2"), "alias should be on net2") + assert.Check(t, !resolves(t, n1, "only-on-net2"), "alias must NOT leak to net1") + + // Tear down — each network's alias should be released independently. + assert.NilError(t, c.rmServiceBinding(svcName, svcID, n1.ID(), "ep-net1", "ctr-1", + vip1, nil, []string{"only-on-net1"}, nil, net.ParseIP("172.20.0.20"), "test", true, true)) + assert.Check(t, !resolves(t, n1, "only-on-net1")) + assert.Check(t, resolves(t, n2, "only-on-net2"), "removing net1 binding must not affect net2") + + assert.NilError(t, c.rmServiceBinding(svcName, svcID, n2.ID(), "ep-net2", "ctr-2", + vip2, nil, []string{"only-on-net2"}, nil, net.ParseIP("172.21.0.20"), "test", true, true)) + assert.Check(t, !resolves(t, n2, "only-on-net2")) + }) + + t.Run("rebinding same endpoint with changed alias set", func(t *testing.T) { + // First bind: alias=[a]. + assert.NilError(t, c.addServiceBinding(svcName, svcID, n1.ID(), "ep-rebind", "ctr-rb", + vip1, nil, []string{"a"}, nil, net.ParseIP("172.20.0.30"), "test")) + assert.Check(t, resolves(t, n1, "a")) + assert.Check(t, !resolves(t, n1, "b")) + + // Re-bind same eID with alias=[b]. Diff should drop "a" and add "b". + assert.NilError(t, c.addServiceBinding(svcName, svcID, n1.ID(), "ep-rebind", "ctr-rb", + vip1, nil, []string{"b"}, nil, net.ParseIP("172.20.0.30"), "test")) + assert.Check(t, !resolves(t, n1, "a"), "'a' should be removed by diff") + assert.Check(t, resolves(t, n1, "b")) + + assert.NilError(t, c.rmServiceBinding(svcName, svcID, n1.ID(), "ep-rebind", "ctr-rb", + vip1, nil, []string{"b"}, nil, net.ParseIP("172.20.0.30"), "test", true, true)) + assert.Check(t, !resolves(t, n1, "b")) + }) +}