daemon: ContainerExtractToDir: make AllowOverwriteDirWithFile opt-in

This change changes the default for noOverwriteDirNonDir to be true
internally, with the intent to change the default at the API to follow
accordingly.

The `AllowOverwriteDirWithFile` option in the Client was added when
reimplementing the CLI using the API Client lib in [moby@1b2b91b].
Before that refactor, the `noOverwriteDirNonDir` query argument
[would be set unconditionally][1] by the CLI, with no options to
control the behavior.

The `noOverwriteDirNonDir` query parameter was added in [moby@db9cc91] to
set the `NoOverwriteDirNonDir` option that was implemented in pkg/archive
in [moby@a74799b].

It was added in [PR13171-comment2], following a discussion on the risk
of replacing a directory with a file and vice-versa in [PR13171-comment].

> In my latest changes from yesterday:
>
> - Removed the `GET stat-path` endpoint and added a `HEAD` handler to
>   the `archive-path` endpoint. Updated the api docs to reflect this.
>   Also moved api docs changes from `v1.19` to `v1.20`.
> - Added a `NoOverwriteDirNonDir` flag to `archive.TarOptions` to indicate
>   that we do not want to overwrite a directory with a non-directory (and
>   vice versa) when unpacking an archive.
> - Added a corresponding but optional `noOverwriteDirNonDir` parameter
>   to the `PUT extract-to-dir` endpoint to specify desired behavior.
>
> These changes combine to keep the behavior we want

It's unclear why these were added as an *option* and why it was implemented
as opt-in (not opt-out), as overwriting a file with a directory (or vice-versa)
would generally be unexpected behavior.

[1]: 8c9ad7b818/api/client/cp.go (L345-L346)
[moby@1b2b91b]: 1b2b91ba43
[moby@a74799b]: a74799b701
[moby@db9cc91]: db9cc91a9e
[PR13171-comment]: https://github.com/moby/moby/pull/13171#issuecomment-106559765
[PR13171-comment2]: https://github.com/moby/moby/pull/13171#issuecomment-108538643

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

daemon/archive.go

@@ -57,16 +57,16 @@ 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 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 {
+// be an errdefs.InvalidParameter. It returns an error if unpacking the given
+// content would cause an existing directory to be replaced with a non-directory
+// or vice versa, unless allowOverwriteDirWithFile is set to true.
+func (daemon *Daemon) ContainerExtractToDir(name, path string, copyUIDGID, allowOverwriteDirWithFile bool, content io.Reader) error {
 	ctr, err := daemon.GetContainer(name)
 	if err != nil {
 		return err
 	}
 
-	err = daemon.containerExtractToDir(ctr, path, copyUIDGID, noOverwriteDirNonDir, content)
+	err = daemon.containerExtractToDir(ctr, path, copyUIDGID, allowOverwriteDirWithFile, content)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return containerFileNotFound{path, name}

daemon/archive_tarcopyoptions.go

@@ -6,9 +6,9 @@ import (
 
 // defaultTarCopyOptions is the setting that is used when unpacking an archive
 // for a copy API event.
-func (daemon *Daemon) defaultTarCopyOptions(noOverwriteDirNonDir bool) *archive.TarOptions {
+func (daemon *Daemon) defaultTarCopyOptions(allowOverwriteDirWithFile bool) *archive.TarOptions {
 	return &archive.TarOptions{
-		NoOverwriteDirNonDir: noOverwriteDirNonDir,
+		NoOverwriteDirNonDir: !allowOverwriteDirWithFile,
 		IDMap:                daemon.idMapping,
 	}
 }

daemon/archive_tarcopyoptions_unix.go

@@ -14,9 +14,9 @@ import (
 	"github.com/moby/sys/user"
 )
 
-func (daemon *Daemon) tarCopyOptions(ctr *container.Container, noOverwriteDirNonDir bool) (*archive.TarOptions, error) {
+func (daemon *Daemon) tarCopyOptions(ctr *container.Container, allowOverwriteDirWithFile bool) (*archive.TarOptions, error) {
 	if ctr.Config.User == "" {
-		return daemon.defaultTarCopyOptions(noOverwriteDirNonDir), nil
+		return daemon.defaultTarCopyOptions(allowOverwriteDirWithFile), nil
 	}
 
 	uid, gid, err := getUIDGID(ctr.Config.User)
@@ -25,7 +25,7 @@ func (daemon *Daemon) tarCopyOptions(ctr *container.Container, noOverwriteDirNon
 	}
 
 	return &archive.TarOptions{
-		NoOverwriteDirNonDir: noOverwriteDirNonDir,
+		NoOverwriteDirNonDir: !allowOverwriteDirWithFile,
 		ChownOpts:            &archive.ChownOpts{UID: uid, GID: gid},
 	}, nil
 }

daemon/archive_unix.go

@@ -97,7 +97,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 // 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(container *container.Container, path string, copyUIDGID, noOverwriteDirNonDir bool, content io.Reader) error {
+func (daemon *Daemon) containerExtractToDir(container *container.Container, path string, copyUIDGID, allowOverwriteDirWithFile bool, content io.Reader) error {
 	container.Lock()
 	defer container.Unlock()
 
@@ -131,13 +131,13 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
 			return err
 		}
 
-		options := daemon.defaultTarCopyOptions(noOverwriteDirNonDir)
+		options := daemon.defaultTarCopyOptions(allowOverwriteDirWithFile)
 
 		if copyUIDGID {
 			var err error
 			// tarCopyOptions will appropriately pull in the right uid/gid for the
 			// user/group and will set the options.
-			options, err = daemon.tarCopyOptions(container, noOverwriteDirNonDir)
+			options, err = daemon.tarCopyOptions(container, allowOverwriteDirWithFile)
 			if err != nil {
 				return err
 			}

daemon/archive_windows.go

@@ -148,7 +148,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 // directory and vice versa.
 //
 // FIXME(thaJeztah): copyUIDGID is not supported on Windows, but currently ignored silently
-func (daemon *Daemon) containerExtractToDir(container *container.Container, path string, copyUIDGID, noOverwriteDirNonDir bool, content io.Reader) error {
+func (daemon *Daemon) containerExtractToDir(container *container.Container, path string, copyUIDGID, allowOverwriteDirWithFile bool, content io.Reader) error {
 	container.Lock()
 	defer container.Unlock()
 
@@ -213,7 +213,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
 	// 	return err
 	// }
 
-	options := daemon.defaultTarCopyOptions(noOverwriteDirNonDir)
+	options := daemon.defaultTarCopyOptions(allowOverwriteDirWithFile)
 	if err := chrootarchive.UntarWithRoot(content, resolvedPath, options, container.BaseFS); err != nil {
 		return err
 	}

daemon/server/router/container/backend.go

@@ -23,7 +23,7 @@ type execBackend interface {
 type copyBackend interface {
 	ContainerArchivePath(name string, path string) (content io.ReadCloser, stat *container.PathStat, err error)
 	ContainerExport(ctx context.Context, name string, out io.Writer) error
-	ContainerExtractToDir(name, path string, copyUIDGID, noOverwriteDirNonDir bool, content io.Reader) error
+	ContainerExtractToDir(name, path string, copyUIDGID, allowOverwriteDirWithFile bool, content io.Reader) error
 	ContainerStatPath(name string, path string) (stat *container.PathStat, err error)
 }
 

daemon/server/router/container/copy.go

@@ -92,8 +92,10 @@ func (c *containerRouter) putContainersArchive(ctx context.Context, w http.Respo
 		return err
 	}
 
-	noOverwriteDirNonDir := httputils.BoolValue(r, "noOverwriteDirNonDir")
+	// TODO(thaJeztah): reverse the default: deprecate noOverwriteDirNonDir and add "allowOverwriteDirWithFile" (or similar) argument to opt-in.
+	allowOverwriteDirWithFile := !r.Form.Has("noOverwriteDirNonDir") || !httputils.BoolValue(r, "noOverwriteDirNonDir")
+
 	copyUIDGID := httputils.BoolValue(r, "copyUIDGID")
 
-	return c.backend.ContainerExtractToDir(v.Name, v.Path, copyUIDGID, noOverwriteDirNonDir, r.Body)
+	return c.backend.ContainerExtractToDir(v.Name, v.Path, copyUIDGID, allowOverwriteDirWithFile, r.Body)
 }