Merge pull request #46211 from thaJeztah/daemon_error_cleanup

daemon: WithNamespaces(): improve error-handling, and assorted cleanups

daemon/container.go

@@ -68,16 +68,6 @@ func (daemon *Daemon) GetContainer(prefixOrName string) (*container.Container, e
 	return ctr, nil
 }
 
-// checkContainer make sure the specified container validates the specified conditions
-func (daemon *Daemon) checkContainer(container *container.Container, conditions ...func(*container.Container) error) error {
-	for _, condition := range conditions {
-		if err := condition(container); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // Exists returns a true if a container of the specified ID or name exists,
 // false otherwise.
 func (daemon *Daemon) Exists(id string) bool {

daemon/container_operations_unix.go

@@ -59,60 +59,54 @@ func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]s
 	return env, nil
 }
 
-func (daemon *Daemon) getIpcContainer(id string) (*container.Container, error) {
-	errMsg := "can't join IPC of container " + id
-	// Check the container exists
+func (daemon *Daemon) getIPCContainer(id string) (*container.Container, error) {
+	// Check if the container exists, is running, and not restarting
 	ctr, err := daemon.GetContainer(id)
 	if err != nil {
-		return nil, errors.Wrap(err, errMsg)
+		return nil, errdefs.InvalidParameter(err)
 	}
-	// Check the container is running and not restarting
-	if err := daemon.checkContainer(ctr, containerIsRunning, containerIsNotRestarting); err != nil {
-		return nil, errors.Wrap(err, errMsg)
+	if !ctr.IsRunning() {
+		return nil, errNotRunning(id)
 	}
+	if ctr.IsRestarting() {
+		return nil, errContainerIsRestarting(id)
+	}
+
 	// Check the container ipc is shareable
 	if st, err := os.Stat(ctr.ShmPath); err != nil || !st.IsDir() {
 		if err == nil || os.IsNotExist(err) {
-			return nil, errors.New(errMsg + ": non-shareable IPC (hint: use IpcMode:shareable for the donor container)")
+			return nil, errdefs.InvalidParameter(errors.New("container " + id + ": non-shareable IPC (hint: use IpcMode:shareable for the donor container)"))
 		}
 		// stat() failed?
-		return nil, errors.Wrap(err, errMsg+": unexpected error from stat "+ctr.ShmPath)
+		return nil, errdefs.System(errors.Wrap(err, "container "+id))
 	}
 
 	return ctr, nil
 }
 
-func (daemon *Daemon) getPidContainer(ctr *container.Container) (*container.Container, error) {
-	containerID := ctr.HostConfig.PidMode.Container()
-	ctr, err := daemon.GetContainer(containerID)
+func (daemon *Daemon) getPIDContainer(id string) (*container.Container, error) {
+	ctr, err := daemon.GetContainer(id)
 	if err != nil {
-		return nil, errors.Wrapf(err, "cannot join PID of a non running container: %s", containerID)
+		return nil, errdefs.InvalidParameter(err)
 	}
-	return ctr, daemon.checkContainer(ctr, containerIsRunning, containerIsNotRestarting)
-}
-
-func containerIsRunning(c *container.Container) error {
-	if !c.IsRunning() {
-		return errdefs.Conflict(errors.Errorf("container %s is not running", c.ID))
+	if !ctr.IsRunning() {
+		return nil, errNotRunning(id)
 	}
-	return nil
-}
-
-func containerIsNotRestarting(c *container.Container) error {
-	if c.IsRestarting() {
-		return errContainerIsRestarting(c.ID)
+	if ctr.IsRestarting() {
+		return nil, errContainerIsRestarting(id)
 	}
-	return nil
+
+	return ctr, nil
 }
 
-func (daemon *Daemon) setupIpcDirs(c *container.Container) error {
+func (daemon *Daemon) setupIPCDirs(c *container.Container) error {
 	ipcMode := c.HostConfig.IpcMode
 
 	switch {
 	case ipcMode.IsContainer():
-		ic, err := daemon.getIpcContainer(ipcMode.Container())
+		ic, err := daemon.getIPCContainer(ipcMode.Container())
 		if err != nil {
-			return err
+			return errors.Wrapf(err, "failed to join IPC namespace")
 		}
 		c.ShmPath = ic.ShmPath
 

daemon/oci_linux.go

@@ -244,34 +244,47 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
 		userNS := false
 		// user
 		if c.HostConfig.UsernsMode.IsPrivate() {
-			uidMap := daemon.idMapping.UIDMaps
-			if uidMap != nil {
+			if uidMap := daemon.idMapping.UIDMaps; uidMap != nil {
 				userNS = true
-				ns := specs.LinuxNamespace{Type: "user"}
-				setNamespace(s, ns)
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.UserNamespace,
+				})
 				s.Linux.UIDMappings = specMapping(uidMap)
 				s.Linux.GIDMappings = specMapping(daemon.idMapping.GIDMaps)
 			}
 		}
 		// network
 		if !c.Config.NetworkDisabled {
-			ns := specs.LinuxNamespace{Type: "network"}
-			if c.HostConfig.NetworkMode.IsContainer() {
-				nc, err := daemon.getNetworkedContainer(c.ID, c.HostConfig.NetworkMode.ConnectedContainer())
+			networkMode := c.HostConfig.NetworkMode
+			switch {
+			case networkMode.IsContainer():
+				nc, err := daemon.getNetworkedContainer(c.ID, networkMode.ConnectedContainer())
 				if err != nil {
 					return err
 				}
-				ns.Path = fmt.Sprintf("/proc/%d/ns/net", nc.State.GetPID())
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.NetworkNamespace,
+					Path: fmt.Sprintf("/proc/%d/ns/net", nc.State.GetPID()),
+				})
 				if userNS {
-					// to share a net namespace, they must also share a user namespace
-					nsUser := specs.LinuxNamespace{Type: "user"}
-					nsUser.Path = fmt.Sprintf("/proc/%d/ns/user", nc.State.GetPID())
-					setNamespace(s, nsUser)
+					// to share a net namespace, the containers must also share a user namespace.
+					//
+					// FIXME(thaJeztah): this will silently overwrite an earlier user namespace when joining multiple containers: https://github.com/moby/moby/issues/46210
+					setNamespace(s, specs.LinuxNamespace{
+						Type: specs.UserNamespace,
+						Path: fmt.Sprintf("/proc/%d/ns/user", nc.State.GetPID()),
+					})
 				}
-			} else if c.HostConfig.NetworkMode.IsHost() {
-				ns.Path = c.NetworkSettings.SandboxKey
+			case networkMode.IsHost():
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.NetworkNamespace,
+					Path: c.NetworkSettings.SandboxKey,
+				})
+			default:
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.NetworkNamespace,
+				})
 			}
-			setNamespace(s, ns)
 		}
 
 		// ipc
@@ -281,64 +294,73 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
 		}
 		switch {
 		case ipcMode.IsContainer():
-			ns := specs.LinuxNamespace{Type: "ipc"}
-			ic, err := daemon.getIpcContainer(ipcMode.Container())
+			ic, err := daemon.getIPCContainer(ipcMode.Container())
 			if err != nil {
-				return errdefs.InvalidParameter(errors.Wrapf(err, "invalid IPC mode: %v", ipcMode))
+				return errors.Wrap(err, "failed to join IPC namespace")
 			}
-			ns.Path = fmt.Sprintf("/proc/%d/ns/ipc", ic.State.GetPID())
-			setNamespace(s, ns)
+			setNamespace(s, specs.LinuxNamespace{
+				Type: specs.IPCNamespace,
+				Path: fmt.Sprintf("/proc/%d/ns/ipc", ic.State.GetPID()),
+			})
 			if userNS {
-				// to share an IPC namespace, they must also share a user namespace
-				nsUser := specs.LinuxNamespace{Type: "user"}
-				nsUser.Path = fmt.Sprintf("/proc/%d/ns/user", ic.State.GetPID())
-				setNamespace(s, nsUser)
+				// to share a IPC namespace, the containers must also share a user namespace.
+				//
+				// FIXME(thaJeztah): this will silently overwrite an earlier user namespace when joining multiple containers: https://github.com/moby/moby/issues/46210
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.UserNamespace,
+					Path: fmt.Sprintf("/proc/%d/ns/user", ic.State.GetPID()),
+				})
 			}
 		case ipcMode.IsHost():
-			oci.RemoveNamespace(s, "ipc")
+			oci.RemoveNamespace(s, specs.IPCNamespace)
 		case ipcMode.IsEmpty():
 			// A container was created by an older version of the daemon.
 			// The default behavior used to be what is now called "shareable".
 			fallthrough
 		case ipcMode.IsPrivate(), ipcMode.IsShareable(), ipcMode.IsNone():
-			ns := specs.LinuxNamespace{Type: "ipc"}
-			setNamespace(s, ns)
+			setNamespace(s, specs.LinuxNamespace{
+				Type: specs.IPCNamespace,
+			})
 		}
 
 		// pid
-		if !c.HostConfig.PidMode.Valid() {
-			return errdefs.InvalidParameter(errors.Errorf("invalid PID mode: %v", c.HostConfig.PidMode))
+		pidMode := c.HostConfig.PidMode
+		if !pidMode.Valid() {
+			return errdefs.InvalidParameter(errors.Errorf("invalid PID mode: %v", pidMode))
 		}
-		if c.HostConfig.PidMode.IsContainer() {
-			pc, err := daemon.getPidContainer(c)
+		switch {
+		case pidMode.IsContainer():
+			pc, err := daemon.getPIDContainer(pidMode.Container())
 			if err != nil {
-				return err
+				return errors.Wrap(err, "failed to join PID namespace")
 			}
-			ns := specs.LinuxNamespace{
-				Type: "pid",
+			setNamespace(s, specs.LinuxNamespace{
+				Type: specs.PIDNamespace,
 				Path: fmt.Sprintf("/proc/%d/ns/pid", pc.State.GetPID()),
-			}
-			setNamespace(s, ns)
+			})
 			if userNS {
-				// to share a PID namespace, they must also share a user namespace
-				nsUser := specs.LinuxNamespace{
-					Type: "user",
+				// to share a PID namespace, the containers must also share a user namespace.
+				//
+				// FIXME(thaJeztah): this will silently overwrite an earlier user namespace when joining multiple containers: https://github.com/moby/moby/issues/46210
+				setNamespace(s, specs.LinuxNamespace{
+					Type: specs.UserNamespace,
 					Path: fmt.Sprintf("/proc/%d/ns/user", pc.State.GetPID()),
-				}
-				setNamespace(s, nsUser)
+				})
 			}
-		} else if c.HostConfig.PidMode.IsHost() {
-			oci.RemoveNamespace(s, "pid")
-		} else {
-			ns := specs.LinuxNamespace{Type: "pid"}
-			setNamespace(s, ns)
+		case pidMode.IsHost():
+			oci.RemoveNamespace(s, specs.PIDNamespace)
+		default:
+			setNamespace(s, specs.LinuxNamespace{
+				Type: specs.PIDNamespace,
+			})
 		}
+
 		// uts
 		if !c.HostConfig.UTSMode.Valid() {
 			return errdefs.InvalidParameter(errors.Errorf("invalid UTS mode: %v", c.HostConfig.UTSMode))
 		}
 		if c.HostConfig.UTSMode.IsHost() {
-			oci.RemoveNamespace(s, "uts")
+			oci.RemoveNamespace(s, specs.UTSNamespace)
 			s.Hostname = ""
 		}
 
@@ -346,11 +368,10 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
 		if !c.HostConfig.CgroupnsMode.Valid() {
 			return errdefs.InvalidParameter(errors.Errorf("invalid cgroup namespace mode: %v", c.HostConfig.CgroupnsMode))
 		}
-		if !c.HostConfig.CgroupnsMode.IsEmpty() {
-			if c.HostConfig.CgroupnsMode.IsPrivate() {
-				nsCgroup := specs.LinuxNamespace{Type: "cgroup"}
-				setNamespace(s, nsCgroup)
-			}
+		if c.HostConfig.CgroupnsMode.IsPrivate() {
+			setNamespace(s, specs.LinuxNamespace{
+				Type: specs.CgroupNamespace,
+			})
 		}
 
 		return nil
@@ -511,7 +532,7 @@ func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container)
 			return err
 		}
 
-		if err := daemon.setupIpcDirs(c); err != nil {
+		if err := daemon.setupIPCDirs(c); err != nil {
 			return err
 		}
 

integration/container/pidmode_linux_test.go

@@ -6,8 +6,11 @@ import (
 	"testing"
 	"time"
 
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/integration/internal/container"
 	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/poll"
 	"gotest.tools/v3/skip"
 )
@@ -33,3 +36,41 @@ func TestPIDModeHost(t *testing.T) {
 	cPid = container.GetContainerNS(ctx, t, apiClient, cID, "pid")
 	assert.Assert(t, hostPid != cPid)
 }
+
+func TestPIDModeContainer(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType != "linux")
+
+	defer setupTest(t)()
+	apiClient := testEnv.APIClient()
+	ctx := context.Background()
+
+	t.Run("non-existing container", func(t *testing.T) {
+		_, err := container.CreateFromConfig(ctx, apiClient, container.NewTestConfig(container.WithPIDMode("container:nosuchcontainer")))
+		assert.Check(t, is.ErrorType(err, errdefs.IsInvalidParameter))
+		assert.Check(t, is.ErrorContains(err, "No such container: nosuchcontainer"))
+	})
+
+	t.Run("non-running container", func(t *testing.T) {
+		const pidCtrName = "stopped-pid-namespace-container"
+		cPIDContainerID := container.Create(ctx, t, apiClient, container.WithName(pidCtrName))
+
+		ctr, err := container.CreateFromConfig(ctx, apiClient, container.NewTestConfig(container.WithPIDMode("container:"+pidCtrName)))
+		assert.NilError(t, err, "should not produce an error when creating, only when starting")
+
+		err = apiClient.ContainerStart(ctx, ctr.ID, types.ContainerStartOptions{})
+		assert.Check(t, is.ErrorType(err, errdefs.IsSystem), "should produce a System error when starting an existing container from an invalid state")
+		assert.Check(t, is.ErrorContains(err, "failed to join PID namespace"))
+		assert.Check(t, is.ErrorContains(err, cPIDContainerID+" is not running"))
+	})
+
+	t.Run("running container", func(t *testing.T) {
+		const pidCtrName = "running-pid-namespace-container"
+		container.Run(ctx, t, apiClient, container.WithName(pidCtrName))
+
+		ctr, err := container.CreateFromConfig(ctx, apiClient, container.NewTestConfig(container.WithPIDMode("container:"+pidCtrName)))
+		assert.NilError(t, err)
+
+		err = apiClient.ContainerStart(ctx, ctr.ID, types.ContainerStartOptions{})
+		assert.Check(t, err)
+	})
+}