daemon: WithNamespaces(): fix incorrect error for PID, IPC namespace

`Daemon.getPidContainer()` was wrapping the error-message with a message
("cannot join PID of a non running container") that did not reflect the
actual reason for the error; `Daemon.GetContainer()` could either return
an invalid parameter (invalid / empty identifier), or a "not found" error
if the specified container-ID could not be found.

In the latter case, we don't want to return a "not found" error through
the API, as this would indicate that the container we're _starting_ was
not found (which is not the case), so we need to convert the error into
an `errdefs.ErrInvalidParameter` (the container-ID specified for the PID
namespace is invalid if the container doesn't exist).

This logic is similar to what we do for IPC namespaces. which received
a similar fix in c3d7a0c6033a2764dd85c3863809ac498ef129f2.

This patch updates the error-types, and moves them into the getIpcContainer
and getPidContainer container functions, both of which should return
an "invalid parameter" if the container was not found.

It's worth noting that, while `WithNamespaces()` may return an "invalid
parameter" error, the `start` endpoint itself may _not_ be. as outlined
in commit bf1fb97575ae0c929075f8340d7deb4ae9f41fae, starting a container
that has an invalid configuration should be considered an internal server
error, and is not an invalid _request_. However, for uses other than
container "start", `WithNamespaces()` should return the correct error
to allow code to handle it accordingly.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/container_operations_unix.go

@@ -65,7 +65,7 @@ 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(errors.Wrap(err, errMsg))
 	}
 	if !ctr.IsRunning() {
 		return nil, errNotRunning(id)
@@ -77,10 +77,10 @@ func (daemon *Daemon) getIpcContainer(id string) (*container.Container, error) {
 	// 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(errMsg + ": 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, errMsg+": unexpected error from stat "+ctr.ShmPath))
 	}
 
 	return ctr, nil
@@ -90,7 +90,7 @@ func (daemon *Daemon) getPidContainer(ctr *container.Container) (*container.Cont
 	id := ctr.HostConfig.PidMode.Container()
 	ctr, err := daemon.GetContainer(id)
 	if err != nil {
-		return nil, errors.Wrapf(err, "cannot join PID of a non running container: %s", id)
+		return nil, errdefs.InvalidParameter(err)
 	}
 	if !ctr.IsRunning() {
 		return nil, errNotRunning(id)

daemon/oci_linux.go

@@ -294,7 +294,7 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
 		case ipcMode.IsContainer():
 			ic, err := daemon.getIpcContainer(ipcMode.Container())
 			if err != nil {
-				return errdefs.InvalidParameter(errors.Wrapf(err, "invalid IPC mode: %v", ipcMode))
+				return errors.Wrapf(err, "invalid IPC mode: %v", ipcMode)
 			}
 			setNamespace(s, specs.LinuxNamespace{
 				Type: specs.IPCNamespace,
@@ -328,7 +328,7 @@ func WithNamespaces(daemon *Daemon, c *container.Container) coci.SpecOpts {
 		case pidMode.IsContainer():
 			pc, err := daemon.getPidContainer(c)
 			if err != nil {
-				return err
+				return errors.Wrap(err, "failed to join PID namespace")
 			}
 			setNamespace(s, specs.LinuxNamespace{
 				Type: specs.PIDNamespace,

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