Separate Sandbox/Endpoint construction

If config for legacy links needs to be added to a libnetwork.Sandbox,
add it when constructing the Endpoint that needs it - removing the
constraint on ordering of Endpoint construction, and the dependency
between Endpoint and Sandbox construction.

So, now a Sandbox can be constructed in one place, before the first
Endpoint.

Signed-off-by: Rob Murray <rob.murray@docker.com>

daemon/container_operations.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"net"
 	"os"
-	"path"
 	"strings"
 	"time"
 
@@ -25,7 +24,6 @@ import (
 	"github.com/docker/docker/internal/sliceutil"
 	"github.com/docker/docker/libnetwork"
 	"github.com/docker/docker/libnetwork/netlabel"
-	"github.com/docker/docker/libnetwork/options"
 	"github.com/docker/docker/libnetwork/scope"
 	"github.com/docker/docker/libnetwork/types"
 	"github.com/docker/docker/opts"
@@ -157,63 +155,6 @@ func (daemon *Daemon) buildSandboxOptions(cfg *config.Config, ctr *container.Con
 
 	sboxOptions = append(sboxOptions, libnetwork.OptionPortMapping(publishedPorts), libnetwork.OptionExposedPorts(exposedPorts))
 
-	// Legacy Link feature is supported only for the default bridge network.
-	// return if this call to build join options is not for default bridge network
-	// Legacy Link is only supported by docker run --link
-	defaultNetName := network.DefaultNetwork
-	bridgeSettings, ok := ctr.NetworkSettings.Networks[defaultNetName]
-	if !ok || bridgeSettings.EndpointSettings == nil || bridgeSettings.EndpointID == "" {
-		return sboxOptions, nil
-	}
-
-	var (
-		childEndpoints []string
-		cEndpointID    string
-	)
-	for linkAlias, child := range daemon.children(ctr) {
-		if !isLinkable(child) {
-			return nil, fmt.Errorf("Cannot link to %s, as it does not belong to the default network", child.Name)
-		}
-		_, alias := path.Split(linkAlias)
-		// allow access to the linked container via the alias, real name, and container hostname
-		aliasList := alias + " " + child.Config.Hostname
-		// only add the name if alias isn't equal to the name
-		if alias != child.Name[1:] {
-			aliasList = aliasList + " " + child.Name[1:]
-		}
-		defaultNW := child.NetworkSettings.Networks[defaultNetName]
-		if defaultNW.IPAddress != "" {
-			sboxOptions = append(sboxOptions, libnetwork.OptionExtraHost(aliasList, defaultNW.IPAddress))
-		}
-		if defaultNW.GlobalIPv6Address != "" {
-			sboxOptions = append(sboxOptions, libnetwork.OptionExtraHost(aliasList, defaultNW.GlobalIPv6Address))
-		}
-		cEndpointID = defaultNW.EndpointID
-		if cEndpointID != "" {
-			childEndpoints = append(childEndpoints, cEndpointID)
-		}
-	}
-
-	var parentEndpoints []string
-	for alias, parent := range daemon.parents(ctr) {
-		if cfg.DisableBridge || !ctr.HostConfig.NetworkMode.IsPrivate() {
-			continue
-		}
-
-		_, alias = path.Split(alias)
-		log.G(context.TODO()).Debugf("Update /etc/hosts of %s for alias %s with ip %s", parent.ID, alias, bridgeSettings.IPAddress)
-		sboxOptions = append(sboxOptions, libnetwork.OptionParentUpdate(parent.ID, alias, bridgeSettings.IPAddress))
-		if cEndpointID != "" {
-			parentEndpoints = append(parentEndpoints, cEndpointID)
-		}
-	}
-
-	sboxOptions = append(sboxOptions, libnetwork.OptionGeneric(options.Generic{
-		netlabel.GenericData: options.Generic{
-			"ParentEndpoints": parentEndpoints,
-			"ChildEndpoints":  childEndpoints,
-		},
-	}))
 	return sboxOptions, nil
 }
 
@@ -497,28 +438,27 @@ func (daemon *Daemon) allocateNetwork(ctx context.Context, cfg *config.Config, c
 
 	daemon.updateContainerNetworkSettings(ctr, nil)
 
-	// always connect default network first since only default
-	// network mode support link and we need do some setting
-	// on sandbox initialize for link, but the sandbox only be initialized
-	// on first network connecting.
-	defaultNetName := network.DefaultNetwork
-	if nConf, ok := ctr.NetworkSettings.Networks[defaultNetName]; ok {
-		cleanOperationalData(nConf)
-		if err := daemon.connectToNetwork(ctx, cfg, ctr, defaultNetName, nConf); err != nil {
-			return err
-		}
+	sbOptions, err := daemon.buildSandboxOptions(cfg, ctr)
+	if err != nil {
+		return err
+	}
+	sb, err := daemon.netController.NewSandbox(ctx, ctr.ID, sbOptions...)
+	if err != nil {
+		return err
 	}
 
-	// the intermediate map is necessary because "connectToNetwork" modifies "container.NetworkSettings.Networks"
+	setNetworkSandbox(ctr, sb)
+
+	defer func() {
+		if retErr != nil {
+			sb.Delete(ctx)
+		}
+	}()
+
 	networks := make(map[string]*network.EndpointSettings)
 	for n, epConf := range ctr.NetworkSettings.Networks {
-		if n == defaultNetName {
-			continue
-		}
-
 		networks[n] = epConf
 	}
-
 	for netName, epConf := range networks {
 		cleanOperationalData(epConf)
 		if err := daemon.connectToNetwork(ctx, cfg, ctr, netName, epConf); err != nil {
@@ -526,31 +466,6 @@ func (daemon *Daemon) allocateNetwork(ctx context.Context, cfg *config.Config, c
 		}
 	}
 
-	// If the container is not to be connected to any network,
-	// create its network sandbox now if not present
-	if len(networks) == 0 {
-		if _, err := daemon.netController.GetSandbox(ctr.ID); err != nil {
-			if !errdefs.IsNotFound(err) {
-				return err
-			}
-
-			sbOptions, err := daemon.buildSandboxOptions(cfg, ctr)
-			if err != nil {
-				return err
-			}
-			sb, err := daemon.netController.NewSandbox(ctx, ctr.ID, sbOptions...)
-			if err != nil {
-				return err
-			}
-			setNetworkSandbox(ctr, sb)
-			defer func() {
-				if retErr != nil {
-					sb.Delete(context.WithoutCancel(ctx))
-				}
-			}()
-		}
-	}
-
 	if _, err := ctr.WriteHostConfig(); err != nil {
 		return err
 	}
@@ -744,8 +659,11 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config,
 		return err
 	}
 
-	// TODO(thaJeztah): should this fail early if no sandbox was found?
-	sb, _ := daemon.netController.GetSandbox(ctr.ID)
+	sb, err := daemon.netController.GetSandbox(ctr.ID)
+	if err != nil {
+		return err
+	}
+
 	createOptions, err := buildCreateEndpointOptions(ctr, n, endpointConfig, sb, ipAddresses(cfg.DNS))
 	if err != nil {
 		return err
@@ -770,17 +688,10 @@ func (daemon *Daemon) connectToNetwork(ctx context.Context, cfg *config.Config,
 		return err
 	}
 
-	if sb == nil {
-		sbOptions, err := daemon.buildSandboxOptions(cfg, ctr)
-		if err != nil {
+	if nwName == network.DefaultNetwork {
+		if err := daemon.addLegacyLinks(ctx, cfg, ctr, endpointConfig, sb); err != nil {
 			return err
 		}
-		sb, err = daemon.netController.NewSandbox(ctx, ctr.ID, sbOptions...)
-		if err != nil {
-			return err
-		}
-
-		setNetworkSandbox(ctr, sb)
 	}
 
 	joinOptions, err := buildJoinOptions(ctr.NetworkSettings, n)

daemon/container_operations_unix.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"path"
 	"path/filepath"
 	"strconv"
 	"syscall"
@@ -17,12 +18,14 @@ import (
 	"github.com/docker/docker/daemon/network"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/libnetwork"
+	"github.com/docker/docker/libnetwork/drivers/bridge"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/process"
 	"github.com/docker/docker/pkg/stringid"
 	"github.com/moby/sys/mount"
 	"github.com/opencontainers/selinux/go-selinux/label"
 	"github.com/pkg/errors"
+	"go.opentelemetry.io/otel"
 	"golang.org/x/sys/unix"
 )
 
@@ -59,6 +62,90 @@ func (daemon *Daemon) setupLinkedContainers(ctr *container.Container) ([]string,
 	return env, nil
 }
 
+func (daemon *Daemon) addLegacyLinks(
+	ctx context.Context,
+	cfg *config.Config,
+	ctr *container.Container,
+	epConfig *network.EndpointSettings,
+	sb *libnetwork.Sandbox,
+) error {
+	ctx, span := otel.Tracer("").Start(ctx, "daemon.addLegacyLinks")
+	defer span.End()
+
+	if epConfig.EndpointID == "" {
+		return nil
+	}
+
+	children := daemon.children(ctr)
+	var parents map[string]*container.Container
+	if !cfg.DisableBridge && ctr.HostConfig.NetworkMode.IsPrivate() {
+		parents = daemon.parents(ctr)
+	}
+	if len(children) == 0 && len(parents) == 0 {
+		return nil
+	}
+	for _, child := range children {
+		if !isLinkable(child) {
+			return fmt.Errorf("Cannot link to %s, as it does not belong to the default network", child.Name)
+		}
+	}
+
+	var (
+		childEndpoints []string
+		cEndpointID    string
+	)
+	for linkAlias, child := range children {
+		_, alias := path.Split(linkAlias)
+		// allow access to the linked container via the alias, real name, and container hostname
+		aliasList := alias + " " + child.Config.Hostname
+		// only add the name if alias isn't equal to the name
+		if alias != child.Name[1:] {
+			aliasList = aliasList + " " + child.Name[1:]
+		}
+		defaultNW := child.NetworkSettings.Networks[network.DefaultNetwork]
+		if defaultNW.IPAddress != "" {
+			if err := sb.AddHostsEntry(ctx, aliasList, defaultNW.IPAddress); err != nil {
+				return errors.Wrapf(err, "failed to add address to /etc/hosts for link to %s", child.Name)
+			}
+		}
+		if defaultNW.GlobalIPv6Address != "" {
+			if err := sb.AddHostsEntry(ctx, aliasList, defaultNW.GlobalIPv6Address); err != nil {
+				return errors.Wrapf(err, "failed to add IPv6 address to /etc/hosts for link to %s", child.Name)
+			}
+		}
+		cEndpointID = defaultNW.EndpointID
+		if cEndpointID != "" {
+			childEndpoints = append(childEndpoints, cEndpointID)
+		}
+	}
+
+	var parentEndpoints []string
+	for alias, parent := range parents {
+		_, alias = path.Split(alias)
+		// Update ctr's IP address in /etc/hosts files in containers with legacy-links to ctr.
+		log.G(context.TODO()).Debugf("Update /etc/hosts of %s for alias %s with ip %s", parent.ID, alias, epConfig.IPAddress)
+		if psb, _ := daemon.netController.GetSandbox(parent.ID); psb != nil {
+			if err := psb.UpdateHostsEntry(alias, epConfig.IPAddress); err != nil {
+				return errors.Wrapf(err, "failed to update /etc/hosts of %s for alias %s with IP %s",
+					parent.ID, alias, epConfig.IPAddress)
+			}
+			if epConfig.GlobalIPv6Address != "" {
+				if err := psb.UpdateHostsEntry(alias, epConfig.GlobalIPv6Address); err != nil {
+					return errors.Wrapf(err, "failed to update /etc/hosts of %s for alias %s with IP %s",
+						parent.ID, alias, epConfig.GlobalIPv6Address)
+				}
+			}
+		}
+		if cEndpointID != "" {
+			parentEndpoints = append(parentEndpoints, cEndpointID)
+		}
+	}
+
+	sb.UpdateLabels(bridge.LegacyContainerLinkOptions(parentEndpoints, childEndpoints))
+
+	return nil
+}
+
 func (daemon *Daemon) getIPCContainer(id string) (*container.Container, error) {
 	// Check if the container exists, is running, and not restarting
 	ctr, err := daemon.GetContainer(id)

daemon/container_operations_windows.go

@@ -8,6 +8,7 @@ import (
 	"github.com/containerd/log"
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/daemon/config"
+	"github.com/docker/docker/daemon/network"
 	"github.com/docker/docker/libnetwork"
 	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
@@ -17,6 +18,16 @@ func (daemon *Daemon) setupLinkedContainers(ctr *container.Container) ([]string,
 	return nil, nil
 }
 
+func (daemon *Daemon) addLegacyLinks(
+	ctx context.Context,
+	cfg *config.Config,
+	ctr *container.Container,
+	epConfig *network.EndpointSettings,
+	sb *libnetwork.Sandbox,
+) error {
+	return nil
+}
+
 func (daemon *Daemon) setupConfigDir(ctr *container.Container) (setupErr error) {
 	if len(ctr.ConfigReferences) == 0 {
 		return nil

libnetwork/drivers/bridge/bridge_linux.go

@@ -1522,6 +1522,15 @@ func (d *driver) RevokeExternalConnectivity(nid, eid string) error {
 	return nil
 }
 
+func LegacyContainerLinkOptions(parentEndpoints, childEndpoints []string) map[string]interface{} {
+	return options.Generic{
+		netlabel.GenericData: options.Generic{
+			"ParentEndpoints": parentEndpoints,
+			"ChildEndpoints":  childEndpoints,
+		},
+	}
+}
+
 func (d *driver) link(network *bridgeNetwork, endpoint *bridgeEndpoint, enable bool) (retErr error) {
 	cc := endpoint.containerConfig
 	ec := endpoint.extConnConfig

libnetwork/sandbox.go

@@ -67,13 +67,6 @@ type hostsPathConfig struct {
 	hostsPath       string
 	originHostsPath string
 	extraHosts      []extraHost
-	parentUpdates   []parentUpdate
-}
-
-type parentUpdate struct {
-	cid  string
-	name string
-	ip   string
 }
 
 type extraHost struct {
@@ -273,6 +266,15 @@ func (sb *Sandbox) Refresh(ctx context.Context, options ...SandboxOption) error
 	return nil
 }
 
+func (sb *Sandbox) UpdateLabels(labels map[string]interface{}) {
+	if sb.config.generic == nil {
+		sb.config.generic = make(map[string]interface{}, len(labels))
+	}
+	for k, v := range labels {
+		sb.config.generic[k] = v
+	}
+}
+
 func (sb *Sandbox) MarshalJSON() ([]byte, error) {
 	sb.mu.Lock()
 	defer sb.mu.Unlock()

libnetwork/sandbox_dns_unix.go

@@ -28,10 +28,24 @@ const (
 	resolverIPSandbox = "127.0.0.11"
 )
 
-// finishInitDNS is to be called after the container namespace has been created,
-// before it the user process is started. The container's support for IPv6 can be
-// determined at this point.
-func (sb *Sandbox) finishInitDNS(ctx context.Context) error {
+// AddHostsEntry adds an entry to /etc/hosts.
+func (sb *Sandbox) AddHostsEntry(ctx context.Context, name, ip string) error {
+	sb.config.extraHosts = append(sb.config.extraHosts, extraHost{name: name, IP: ip})
+	return sb.rebuildHostsFile(ctx)
+}
+
+// UpdateHostsEntry updates the IP address in a /etc/hosts entry where the
+// name matches the regular expression regexp.
+func (sb *Sandbox) UpdateHostsEntry(regexp, ip string) error {
+	return etchosts.Update(sb.config.hostsPath, ip, regexp)
+}
+
+// rebuildHostsFile builds the container's /etc/hosts file, based on the current
+// state of the Sandbox (including extra hosts). If called after the container
+// namespace has been created, before the user process is started, the container's
+// support for IPv6 can be determined and IPv6 hosts will be included/excluded
+// accordingly.
+func (sb *Sandbox) rebuildHostsFile(ctx context.Context) error {
 	if err := sb.buildHostsFile(); err != nil {
 		return errdefs.System(err)
 	}
@@ -134,7 +148,7 @@ func (sb *Sandbox) buildHostsFile() error {
 		return err
 	}
 
-	return sb.updateParentHosts()
+	return nil
 }
 
 func (sb *Sandbox) updateHostsFile(ctx context.Context, ifaceIPs []string) error {
@@ -192,35 +206,6 @@ func (sb *Sandbox) deleteHostsEntries(recs []etchosts.Record) {
 	}
 }
 
-func (sb *Sandbox) updateParentHosts() error {
-	var pSb *Sandbox
-
-	for _, update := range sb.config.parentUpdates {
-		// TODO(thaJeztah): was it intentional for this loop to re-use prior results of pSB? If not, we should make pSb local and always replace here.
-		if s, _ := sb.controller.GetSandbox(update.cid); s != nil {
-			pSb = s
-		}
-		if pSb == nil {
-			continue
-		}
-		// TODO(robmry) - filter out IPv6 addresses here if !sb.ipv6Enabled() but...
-		// - this is part of the implementation of '--link', which will be removed along
-		//   with the rest of legacy networking.
-		// - IPv6 addresses shouldn't be allocated if IPv6 is not available in a container,
-		//   and that change will come along later.
-		// - I think this may be dead code, it's not possible to start a parent container with
-		//   '--link child' unless the child has already started ("Error response from daemon:
-		//   Cannot link to a non running container"). So, when the child starts and this method
-		//   is called with updates for parents, the parents aren't running and GetSandbox()
-		//   returns nil.)
-		if err := etchosts.Update(pSb.config.hostsPath, update.ip, update.name); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
 func (sb *Sandbox) restoreResolvConfPath() {
 	if sb.config.resolvConfPath == "" {
 		sb.config.resolvConfPath = defaultPrefix + "/" + sb.id + "/resolv.conf"

libnetwork/sandbox_linux.go

@@ -168,9 +168,8 @@ func (sb *Sandbox) SetKey(ctx context.Context, basePath string) error {
 		}
 	}
 
-	// Set up hosts and resolv.conf files.
 	osSbox.RefreshIPv6LoEnabled()
-	if err := sb.finishInitDNS(ctx); err != nil {
+	if err := sb.rebuildHostsFile(ctx); err != nil {
 		return err
 	}
 

libnetwork/sandbox_options.go

@@ -46,14 +46,6 @@ func OptionExtraHost(name string, IP string) SandboxOption {
 	}
 }
 
-// OptionParentUpdate function returns an option setter for parent container
-// which needs to update the IP address for the linked container.
-func OptionParentUpdate(cid string, name, ip string) SandboxOption {
-	return func(sb *Sandbox) {
-		sb.config.parentUpdates = append(sb.config.parentUpdates, parentUpdate{cid: cid, name: name, ip: ip})
-	}
-}
-
 // OptionResolvConfPath function returns an option setter for resolvconfpath option to
 // be passed to net container methods.
 func OptionResolvConfPath(path string) SandboxOption {
@@ -110,20 +102,6 @@ func OptionUseExternalKey() SandboxOption {
 	}
 }
 
-// OptionGeneric function returns an option setter for Generic configuration
-// that is not managed by libNetwork but can be used by the Drivers during the call to
-// net container creation method. Container Labels are a good example.
-func OptionGeneric(generic map[string]interface{}) SandboxOption {
-	return func(sb *Sandbox) {
-		if sb.config.generic == nil {
-			sb.config.generic = make(map[string]interface{}, len(generic))
-		}
-		for k, v := range generic {
-			sb.config.generic[k] = v
-		}
-	}
-}
-
 // OptionExposedPorts function returns an option setter for the container exposed
 // ports option to be passed to container Create method.
 func OptionExposedPorts(exposedPorts []types.TransportPort) SandboxOption {