From 3564d03b0f0cb4a274d54f9aa872bce34b779a69 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 26 Sep 2022 20:41:46 +0200 Subject: [PATCH 1/5] daemon: remove getPortMapInfo alias The getPortMapInfo var was introduced in f198dfd856ca6125ef50b11d9d698550d66c9d4e, and (from looking at that patch) looks to have been as a quick and dirty workaround for the `container` argument colliding with the `container` import. Signed-off-by: Sebastiaan van Stijn --- daemon/container_operations.go | 9 +++------ daemon/network.go | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 4c3d37468a..2e912cff6e 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -26,12 +26,9 @@ import ( "github.com/sirupsen/logrus" ) -var ( - // ErrRootFSReadOnly is returned when a container - // rootfs is marked readonly. - ErrRootFSReadOnly = errors.New("container rootfs is marked read-only") - getPortMapInfo = getSandboxPortMapInfo -) +// ErrRootFSReadOnly is returned when a container +// rootfs is marked readonly. +var ErrRootFSReadOnly = errors.New("container rootfs is marked read-only") func (daemon *Daemon) getDNSSearchSettings(container *container.Container) []string { if len(container.HostConfig.DNSSearch) > 0 { diff --git a/daemon/network.go b/daemon/network.go index 438d13d71a..3b4786bc3d 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -886,7 +886,7 @@ func buildCreateEndpointOptions(c *container.Container, n libnetwork.Network, ep } // Port-mapping rules belong to the container & applicable only to non-internal networks - portmaps := getSandboxPortMapInfo(sb) + portmaps := getPortMapInfo(sb) if n.Info().Internal() || len(portmaps) > 0 { return createOptions, nil } @@ -960,8 +960,8 @@ func buildCreateEndpointOptions(c *container.Container, n libnetwork.Network, ep return createOptions, nil } -// getSandboxPortMapInfo retrieves the current port-mapping programmed for the given sandbox -func getSandboxPortMapInfo(sb libnetwork.Sandbox) nat.PortMap { +// getPortMapInfo retrieves the current port-mapping programmed for the given sandbox +func getPortMapInfo(sb libnetwork.Sandbox) nat.PortMap { pm := nat.PortMap{} if sb == nil { return pm From e31e9180cda6fe21cc2f23baf41a1cb7287e5e8e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 26 Sep 2022 21:02:35 +0200 Subject: [PATCH 2/5] integration-cli: remove isCpCannotCopyReadOnly utility This utility was just string-matching error output, and no longer had a direct connection with ErrContainerRootfsReadonly / ErrVolumeReadonly. Moving it inline better shows what it's actually checking for. Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_cli_cp_to_container_test.go | 7 ++----- integration-cli/docker_cli_cp_utils_test.go | 4 ---- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/integration-cli/docker_cli_cp_to_container_test.go b/integration-cli/docker_cli_cp_to_container_test.go index fb22b68a74..ac54e4ebe7 100644 --- a/integration-cli/docker_cli_cp_to_container_test.go +++ b/integration-cli/docker_cli_cp_to_container_test.go @@ -411,9 +411,7 @@ func (s *DockerCLICpSuite) TestCpToErrReadOnlyRootfs(c *testing.T) { dstPath := containerCpPath(containerID, "/root/shouldNotExist") err := runDockerCp(c, srcPath, dstPath) - assert.ErrorContains(c, err, "") - - assert.Assert(c, isCpCannotCopyReadOnly(err), "expected ErrContainerRootfsReadonly error, but got %T: %s", err, err) + assert.ErrorContains(c, err, "marked read-only") assert.NilError(c, containerStartOutputEquals(c, containerID, ""), "dstPath should not have existed") } @@ -436,8 +434,7 @@ func (s *DockerCLICpSuite) TestCpToErrReadOnlyVolume(c *testing.T) { dstPath := containerCpPath(containerID, "/vol_ro/shouldNotExist") err := runDockerCp(c, srcPath, dstPath) - assert.ErrorContains(c, err, "") + assert.ErrorContains(c, err, "marked read-only") - assert.Assert(c, isCpCannotCopyReadOnly(err), "expected ErrVolumeReadonly error, but got %T: %s", err, err) assert.NilError(c, containerStartOutputEquals(c, containerID, ""), "dstPath should not have existed") } diff --git a/integration-cli/docker_cli_cp_utils_test.go b/integration-cli/docker_cli_cp_utils_test.go index 4ac95f5059..562409fdfa 100644 --- a/integration-cli/docker_cli_cp_utils_test.go +++ b/integration-cli/docker_cli_cp_utils_test.go @@ -232,10 +232,6 @@ func isCpCannotCopyDir(err error) bool { return strings.Contains(err.Error(), archive.ErrCannotCopyDir.Error()) } -func isCpCannotCopyReadOnly(err error) bool { - return strings.Contains(err.Error(), "marked read-only") -} - func fileContentEquals(c *testing.T, filename, contents string) error { c.Helper() From 0f1eeed5c2a5c08117fd12f8d8da5beac5df8d97 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 26 Sep 2022 20:44:36 +0200 Subject: [PATCH 3/5] daemon: replace ErrRootFSReadOnly with errdefs It was only used in a single location, and the ErrRootFSReadOnly was not checked for, or used as a sentinel error. This error was introduced in c32dde5baadc8c472666ef9d5cead13ab6de28ea, originally named `ErrContainerRootfsReadonly`. It was never used as a sentinel error, but from that commit, it looks like it was added as a package variable to mirror the coding style of already existing errors defined at the package level. This patch removes the exported variable, and replaces the error with an errdefs.InvalidParameter(), so that the API also returns the correct (400) status code. Signed-off-by: Sebastiaan van Stijn --- daemon/archive.go | 2 +- daemon/container_operations.go | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 6d6d01c180..d5f46bf323 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -326,7 +326,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path } if !toVolume && container.HostConfig.ReadonlyRootfs { - return ErrRootFSReadOnly + return errdefs.InvalidParameter(errors.New("container rootfs is marked read-only")) } options := daemon.defaultTarCopyOptions(noOverwriteDirNonDir) diff --git a/daemon/container_operations.go b/daemon/container_operations.go index 2e912cff6e..a8ae9802f7 100644 --- a/daemon/container_operations.go +++ b/daemon/container_operations.go @@ -26,10 +26,6 @@ import ( "github.com/sirupsen/logrus" ) -// ErrRootFSReadOnly is returned when a container -// rootfs is marked readonly. -var ErrRootFSReadOnly = errors.New("container rootfs is marked read-only") - func (daemon *Daemon) getDNSSearchSettings(container *container.Container) []string { if len(container.HostConfig.DNSSearch) > 0 { return container.HostConfig.DNSSearch From c78af57e21f74319228d0398cc09c847726f193e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 27 Sep 2022 22:17:27 +0200 Subject: [PATCH 4/5] daemon: replace ErrVolumeReadonly with errdefs It was only used in a single location, and the ErrVolumeReadonly was not checked for, or used as a sentinel error. This error was introduced in c32dde5baadc8c472666ef9d5cead13ab6de28ea. It was never used as a sentinel error, but from that commit, it looks like it was added as a package variable to mirror already existing errors defined at the package level. This patch removes the exported variable, and replaces the error with an errdefs.InvalidParameter(), so that the API also returns the correct (400) status code. Signed-off-by: Sebastiaan van Stijn --- daemon/archive_unix.go | 4 +++- daemon/volumes.go | 6 ------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/daemon/archive_unix.go b/daemon/archive_unix.go index 863788d72d..7a5e29905a 100644 --- a/daemon/archive_unix.go +++ b/daemon/archive_unix.go @@ -5,7 +5,9 @@ package daemon // import "github.com/docker/docker/daemon" import ( "github.com/docker/docker/container" + "github.com/docker/docker/errdefs" volumemounts "github.com/docker/docker/volume/mounts" + "github.com/pkg/errors" ) // checkIfPathIsInAVolume checks if the path is in a volume. If it is, it @@ -19,7 +21,7 @@ func checkIfPathIsInAVolume(container *container.Container, absPath string) (boo if mnt.RW { break } - return false, ErrVolumeReadonly + return false, errdefs.InvalidParameter(errors.New("mounted volume is marked read-only")) } } return toVolume, nil diff --git a/daemon/volumes.go b/daemon/volumes.go index 6148018aff..6e17e221c6 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -21,12 +21,6 @@ import ( "github.com/sirupsen/logrus" ) -var ( - // ErrVolumeReadonly is used to signal an error when trying to copy data into - // a volume mount that is not writable. - ErrVolumeReadonly = errors.New("mounted volume is marked read-only") -) - type mounts []container.Mount // Len returns the number of mounts. Used in sorting. From 8cd244a318c8c1f16eeeb6a8fa48c0cb79cc16ab Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 27 Sep 2022 22:24:51 +0200 Subject: [PATCH 5/5] daemon: replace ErrExtractPointNotDirectory with errdefs It was only used in a single location, and the ErrExtractPointNotDirectory was not checked for, or used as a sentinel error. This error was introduced in c32dde5baadc8c472666ef9d5cead13ab6de28ea. It was never used as a sentinel error, but from that commit, it looks like it was added as a package variable to mirror already existing errors defined at the package level. This patch removes the exported variable, and replaces the error with an errdefs.InvalidParameter(), so that the API also returns the correct (400) status code. Signed-off-by: Sebastiaan van Stijn --- daemon/archive.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index d5f46bf323..5a1ac57952 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -16,11 +16,6 @@ import ( "github.com/pkg/errors" ) -// ErrExtractPointNotDirectory is used to convey that the operation to extract -// a tar archive to a directory in a container has failed because the specified -// path does not refer to a directory. -var ErrExtractPointNotDirectory = errors.New("extraction point is not a directory") - // ContainerCopy performs a deprecated operation of archiving the resource at // the specified path in the container identified by the given name. func (daemon *Daemon) ContainerCopy(name string, res string) (io.ReadCloser, error) { @@ -97,7 +92,7 @@ func (daemon *Daemon) ContainerArchivePath(name string, path string) (content io // ContainerExtractToDir extracts the given archive to the specified location // in the filesystem of the container identified by the given name. The given // path must be of a directory in the container. If it is not, the error will -// be ErrExtractPointNotDirectory. If noOverwriteDirNonDir is true then it will +// be an errdefs.InvalidParameter. If noOverwriteDirNonDir is true then it will // be an error if unpacking the given content would cause an existing directory // to be replaced with a non-directory and vice versa. func (daemon *Daemon) ContainerExtractToDir(name, path string, copyUIDGID, noOverwriteDirNonDir bool, content io.Reader) error { @@ -239,7 +234,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path // containerExtractToDir extracts the given tar archive to the specified location in the // filesystem of this container. The given path must be of a directory in the -// container. If it is not, the error will be ErrExtractPointNotDirectory. If +// container. If it is not, the error will be an errdefs.InvalidParameter. If // noOverwriteDirNonDir is true then it will be an error if unpacking the // given content would cause an existing directory to be replaced with a non- // directory and vice versa. @@ -288,7 +283,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path } if !stat.IsDir() { - return ErrExtractPointNotDirectory + return errdefs.InvalidParameter(errors.New("extraction point is not a directory")) } // Need to check if the path is in a volume. If it is, it cannot be in a