Configure network endpoints after creating a container

For Linux, delay construction and configuration of network endpoints
until the container has been created (but not started).

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

daemon/container_operations.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net"
 	"os"
+	"runtime"
 	"strings"
 	"time"
 
@@ -421,40 +422,9 @@ func (daemon *Daemon) updateContainerNetworkSettings(ctr *container.Container, e
 }
 
 func (daemon *Daemon) allocateNetwork(ctx context.Context, cfg *config.Config, ctr *container.Container) (retErr error) {
-	if daemon.netController == nil {
-		return nil
-	}
-
 	start := time.Now()
 
-	// Cleanup any stale sandbox left over due to ungraceful daemon shutdown
-	if err := daemon.netController.SandboxDestroy(ctx, ctr.ID); err != nil {
-		log.G(ctx).WithError(err).Errorf("failed to cleanup up stale network sandbox for container %s", ctr.ID)
-	}
-
-	if ctr.Config.NetworkDisabled || ctr.HostConfig.NetworkMode.IsContainer() {
-		return nil
-	}
-
-	daemon.updateContainerNetworkSettings(ctr, nil)
-
-	sbOptions, err := 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(ctx)
-		}
-	}()
-
+	// An intermediate map is necessary because "connectToNetwork" modifies "container.NetworkSettings.Networks"
 	networks := make(map[string]*network.EndpointSettings)
 	for n, epConf := range ctr.NetworkSettings.Networks {
 		networks[n] = epConf
@@ -473,6 +443,86 @@ func (daemon *Daemon) allocateNetwork(ctx context.Context, cfg *config.Config, c
 	return nil
 }
 
+// initializeNetworking prepares network configuration for a new container.
+// If it creates a new libnetwork.Sandbox it's returned as newSandbox, for
+// the caller to Delete() if the container setup fails later in the process.
+func (daemon *Daemon) initializeNetworking(ctx context.Context, cfg *config.Config, ctr *container.Container) (newSandbox *libnetwork.Sandbox, retErr error) {
+	if daemon.netController == nil || ctr.Config.NetworkDisabled {
+		return nil, nil
+	}
+
+	// Cleanup any stale sandbox left over due to ungraceful daemon shutdown
+	if err := daemon.netController.SandboxDestroy(ctx, ctr.ID); err != nil {
+		log.G(ctx).WithError(err).Errorf("failed to cleanup up stale network sandbox for container %s", ctr.ID)
+	}
+
+	if ctr.HostConfig.NetworkMode.IsContainer() {
+		// we need to get the hosts files from the container to join
+		nc, err := daemon.getNetworkedContainer(ctr.ID, ctr.HostConfig.NetworkMode.ConnectedContainer())
+		if err != nil {
+			return nil, err
+		}
+
+		err = daemon.initializeNetworkingPaths(ctr, nc)
+		if err != nil {
+			return nil, err
+		}
+
+		ctr.Config.Hostname = nc.Config.Hostname
+		ctr.Config.Domainname = nc.Config.Domainname
+		return nil, nil
+	}
+
+	if ctr.HostConfig.NetworkMode.IsHost() && ctr.Config.Hostname == "" {
+		hn, err := os.Hostname()
+		if err != nil {
+			return nil, err
+		}
+		ctr.Config.Hostname = hn
+	}
+
+	daemon.updateContainerNetworkSettings(ctr, nil)
+
+	sbOptions, err := buildSandboxOptions(cfg, ctr)
+	if err != nil {
+		return nil, err
+	}
+	sb, err := daemon.netController.NewSandbox(ctx, ctr.ID, sbOptions...)
+	if err != nil {
+		return nil, err
+	}
+
+	setNetworkSandbox(ctr, sb)
+
+	defer func() {
+		if retErr != nil {
+			if err := sb.Delete(ctx); err != nil {
+				log.G(ctx).WithFields(log.Fields{
+					"error":     err,
+					"container": ctr.ID,
+				}).Warn("Failed to remove new network sandbox")
+			}
+		}
+	}()
+
+	if err := ctr.BuildHostnameFile(); err != nil {
+		return nil, err
+	}
+
+	// TODO(robmry) - on Windows, running this after the task has been created does something
+	//  strange to the resolver ... addresses are assigned properly (including addresses
+	//  specified in the 'run' command), nslookup works, but 'ping' doesn't find the address
+	//  of a container. There's no query to our internal resolver from 'ping' (there is from
+	//  nslookup), so Windows must have squirreled away the address somewhere else.
+	if runtime.GOOS == "windows" {
+		if err := daemon.allocateNetwork(ctx, cfg, ctr); err != nil {
+			return nil, err
+		}
+	}
+
+	return newSandbox, nil
+}
+
 // validateEndpointSettings checks whether the given epConfig is valid. The nw parameter can be nil, in which case it
 // won't try to check if the endpoint IP addresses are within network's subnets.
 func validateEndpointSettings(nw *libnetwork.Network, nwName string, epConfig *networktypes.EndpointSettings) error {
@@ -848,39 +898,6 @@ func (daemon *Daemon) normalizeNetMode(ctr *container.Container) error {
 	return nil
 }
 
-func (daemon *Daemon) initializeNetworking(ctx context.Context, cfg *config.Config, ctr *container.Container) error {
-	if ctr.HostConfig.NetworkMode.IsContainer() {
-		// we need to get the hosts files from the container to join
-		nc, err := daemon.getNetworkedContainer(ctr.ID, ctr.HostConfig.NetworkMode.ConnectedContainer())
-		if err != nil {
-			return err
-		}
-
-		err = daemon.initializeNetworkingPaths(ctr, nc)
-		if err != nil {
-			return err
-		}
-
-		ctr.Config.Hostname = nc.Config.Hostname
-		ctr.Config.Domainname = nc.Config.Domainname
-		return nil
-	}
-
-	if ctr.HostConfig.NetworkMode.IsHost() && ctr.Config.Hostname == "" {
-		hn, err := os.Hostname()
-		if err != nil {
-			return err
-		}
-		ctr.Config.Hostname = hn
-	}
-
-	if err := daemon.allocateNetwork(ctx, cfg, ctr); err != nil {
-		return err
-	}
-
-	return ctr.BuildHostnameFile()
-}
-
 func (daemon *Daemon) getNetworkedContainer(containerID, connectedContainerID string) (*container.Container, error) {
 	nc, err := daemon.GetContainer(connectedContainerID)
 	if err != nil {

daemon/start.go

@@ -124,9 +124,20 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore
 		return err
 	}
 
-	if err := daemon.initializeNetworking(ctx, &daemonCfg.Config, container); err != nil {
+	newSandbox, err := daemon.initializeNetworking(ctx, &daemonCfg.Config, container)
+	if err != nil {
 		return err
 	}
+	defer func() {
+		if retErr != nil && newSandbox != nil {
+			if err := newSandbox.Delete(ctx); err != nil {
+				log.G(ctx).WithFields(log.Fields{
+					"error":     err,
+					"container": container.ID,
+				}).Warn("After failure in networking initialisation, failed to remove sandbox")
+			}
+		}
+	}()
 
 	mnts, err := daemon.setupContainerDirs(container)
 	if err != nil {
@@ -221,7 +232,7 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore
 		}
 	}()
 
-	if err := daemon.initializeCreatedTask(ctx, tsk, container, spec); err != nil {
+	if err := daemon.initializeCreatedTask(ctx, &daemonCfg.Config, tsk, container, spec); err != nil {
 		return err
 	}
 

daemon/start_linux.go

@@ -5,30 +5,34 @@ import (
 	"fmt"
 
 	"github.com/docker/docker/container"
+	"github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/libcontainerd/types"
 	"github.com/docker/docker/oci"
 	"github.com/opencontainers/runtime-spec/specs-go"
-	"go.opentelemetry.io/otel"
 )
 
 // initializeCreatedTask performs any initialization that needs to be done to
 // prepare a freshly-created task to be started.
-func (daemon *Daemon) initializeCreatedTask(ctx context.Context, tsk types.Task, container *container.Container, spec *specs.Spec) error {
-	ctx, span := otel.Tracer("").Start(ctx, "daemon.initializeCreatedTask")
-	defer span.End()
-
-	if !container.Config.NetworkDisabled {
-		nspath, ok := oci.NamespacePath(spec, specs.NetworkNamespace)
-		if ok && nspath == "" { // the runtime has been instructed to create a new network namespace for tsk.
-			sb, err := daemon.netController.GetSandbox(container.ID)
-			if err != nil {
-				return errdefs.System(err)
-			}
-			if err := sb.SetKey(ctx, fmt.Sprintf("/proc/%d/ns/net", tsk.Pid())); err != nil {
-				return errdefs.System(err)
-			}
+func (daemon *Daemon) initializeCreatedTask(
+	ctx context.Context,
+	cfg *config.Config,
+	tsk types.Task,
+	ctr *container.Container,
+	spec *specs.Spec,
+) error {
+	if ctr.Config.NetworkDisabled {
+		return nil
+	}
+	nspath, ok := oci.NamespacePath(spec, specs.NetworkNamespace)
+	if ok && nspath == "" { // the runtime has been instructed to create a new network namespace for tsk.
+		sb, err := daemon.netController.GetSandbox(ctr.ID)
+		if err != nil {
+			return errdefs.System(err)
+		}
+		if err := sb.SetKey(ctx, fmt.Sprintf("/proc/%d/ns/net", tsk.Pid())); err != nil {
+			return errdefs.System(err)
 		}
 	}
-	return nil
+	return daemon.allocateNetwork(ctx, cfg, ctr)
 }

daemon/start_notlinux.go

@@ -6,12 +6,13 @@ import (
 	"context"
 
 	"github.com/docker/docker/container"
+	"github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/libcontainerd/types"
 	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 // initializeCreatedTask performs any initialization that needs to be done to
 // prepare a freshly-created task to be started.
-func (daemon *Daemon) initializeCreatedTask(ctx context.Context, tsk types.Task, container *container.Container, spec *specs.Spec) error {
+func (daemon *Daemon) initializeCreatedTask(ctx context.Context, cfg *config.Config, tsk types.Task, container *container.Container, spec *specs.Spec) error {
 	return nil
 }