libnetwork: ref-count service aliases for VIP DNS records

During rolling updates, VIP DNS records for service aliases were never
cleaned up when aliases were removed from the service spec. VIP alias
records were only added when the first backend joined a network
(addService=true) and only removed when the last backend left
(rmService=true). During rolling updates there is always at least one
backend, so neither condition triggered and stale aliases persisted.

Each container has its own service config, so there is no single
canonical alias list per service while a rolling update is in flight.
Instead, ref-count aliases per (network, alias) on each loadBalancer:
add the VIP DNS record on the 0->1 transition (first task on that
network claiming the alias) and remove it on the 1->0 transition (last
task referencing it has left). Ref counts are per-network because a
Swarm service can attach to multiple networks with different alias
sets, and a VIP DNS record only makes sense on the network that
actually has the alias configured.

Aliases now survive a rolling update for as long as any task -- old or
new -- still references them on that network, and new aliases register
as soon as the first task carrying them starts.

Signed-off-by: Mads Jon Nielsen <madsjon@gmail.com>

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

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
 	}

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"))
+	})
+}