From 9f3e5eead5accb9fb405398af700d97e830a3b5b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 7 Oct 2022 23:57:09 +0200 Subject: [PATCH 1/4] pkg/system: deprecate DefaultPathEnv, move to oci This patch: - Deprecates pkg/system.DefaultPathEnv - Moves the implementation inside oci - Adds TODOs to align the default in the Builder with the one used elsewhere Signed-off-by: Sebastiaan van Stijn --- builder/dockerfile/dispatchers_test.go | 5 +++-- builder/dockerfile/evaluator.go | 4 +++- container/container.go | 4 ++-- oci/defaults.go | 18 ++++++++++++++++++ pkg/system/path.go | 15 --------------- pkg/system/path_deprecated.go | 18 ++++++++++++++++++ plugin/v2/plugin_linux.go | 3 +-- 7 files changed, 45 insertions(+), 22 deletions(-) create mode 100644 pkg/system/path_deprecated.go diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index baf70cf811..65a8279415 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -14,7 +14,7 @@ import ( "github.com/docker/docker/api/types/strslice" "github.com/docker/docker/builder" "github.com/docker/docker/image" - "github.com/docker/docker/pkg/system" + "github.com/docker/docker/oci" "github.com/docker/go-connections/nat" "github.com/moby/buildkit/frontend/dockerfile/instructions" "github.com/moby/buildkit/frontend/dockerfile/parser" @@ -128,7 +128,8 @@ func TestFromScratch(t *testing.T) { assert.NilError(t, err) assert.Check(t, sb.state.hasFromImage()) assert.Check(t, is.Equal("", sb.state.imageID)) - expected := "PATH=" + system.DefaultPathEnv(runtime.GOOS) + // TODO(thaJeztah): use github.com/moby/buildkit/util/system.DefaultPathEnv() once https://github.com/moby/buildkit/pull/3158 is resolved. + expected := "PATH=" + oci.DefaultPathEnv(runtime.GOOS) assert.Check(t, is.DeepEqual([]string{expected}, sb.state.runConfig.Env)) } diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index f03a0490f9..9d5601528b 100644 --- a/builder/dockerfile/evaluator.go +++ b/builder/dockerfile/evaluator.go @@ -28,6 +28,7 @@ import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" "github.com/docker/docker/errdefs" + "github.com/docker/docker/oci" "github.com/docker/docker/pkg/system" "github.com/docker/docker/runconfig/opts" "github.com/moby/buildkit/frontend/dockerfile/instructions" @@ -236,7 +237,8 @@ func (s *dispatchState) beginStage(stageName string, image builder.Image) error // Add the default PATH to runConfig.ENV if one exists for the operating system and there // is no PATH set. Note that Windows containers on Windows won't have one as it's set by HCS func (s *dispatchState) setDefaultPath() { - defaultPath := system.DefaultPathEnv(s.operatingSystem) + // TODO(thaJeztah): use github.com/moby/buildkit/util/system.DefaultPathEnv() once https://github.com/moby/buildkit/pull/3158 is resolved. + defaultPath := oci.DefaultPathEnv(s.operatingSystem) if defaultPath == "" { return } diff --git a/container/container.go b/container/container.go index d09a130968..53c452159d 100644 --- a/container/container.go +++ b/container/container.go @@ -28,10 +28,10 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" libcontainerdtypes "github.com/docker/docker/libcontainerd/types" + "github.com/docker/docker/oci" "github.com/docker/docker/pkg/containerfs" "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/restartmanager" "github.com/docker/docker/volume" volumemounts "github.com/docker/docker/volume/mounts" @@ -737,7 +737,7 @@ func (container *Container) CreateDaemonEnvironment(tty bool, linkedEnv []string env := make([]string, 0, envSize) if runtime.GOOS != "windows" { - env = append(env, "PATH="+system.DefaultPathEnv(ctrOS)) + env = append(env, "PATH="+oci.DefaultPathEnv(ctrOS)) env = append(env, "HOSTNAME="+container.Config.Hostname) if tty { env = append(env, "TERM=xterm") diff --git a/oci/defaults.go b/oci/defaults.go index b79892ddc2..03afad4c62 100644 --- a/oci/defaults.go +++ b/oci/defaults.go @@ -9,6 +9,24 @@ import ( func iPtr(i int64) *int64 { return &i } +const defaultUnixPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + +// DefaultPathEnv is unix style list of directories to search for +// executables. Each directory is separated from the next by a colon +// ':' character . +// For Windows containers, an empty string is returned as the default +// path will be set by the container, and Docker has no context of what the +// default path should be. +// +// TODO(thaJeztah) align Windows default with BuildKit; see https://github.com/moby/buildkit/pull/1747 +// TODO(thaJeztah) use defaults from containerd (but align it with BuildKit; see https://github.com/moby/buildkit/pull/1747) +func DefaultPathEnv(os string) string { + if os == "windows" { + return "" + } + return defaultUnixPathEnv +} + // DefaultSpec returns the default spec used by docker for the current Platform func DefaultSpec() specs.Spec { if runtime.GOOS == "windows" { diff --git a/pkg/system/path.go b/pkg/system/path.go index 5c79f60985..0974373d59 100644 --- a/pkg/system/path.go +++ b/pkg/system/path.go @@ -1,20 +1,5 @@ package system // import "github.com/docker/docker/pkg/system" -const defaultUnixPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" - -// DefaultPathEnv is unix style list of directories to search for -// executables. Each directory is separated from the next by a colon -// ':' character . -// For Windows containers, an empty string is returned as the default -// path will be set by the container, and Docker has no context of what the -// default path should be. -func DefaultPathEnv(os string) string { - if os == "windows" { - return "" - } - return defaultUnixPathEnv -} - // CheckSystemDriveAndRemoveDriveLetter verifies that a path, if it includes a drive letter, // is the system drive. // On Linux: this is a no-op. diff --git a/pkg/system/path_deprecated.go b/pkg/system/path_deprecated.go new file mode 100644 index 0000000000..5c95026c3d --- /dev/null +++ b/pkg/system/path_deprecated.go @@ -0,0 +1,18 @@ +package system + +const defaultUnixPathEnv = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + +// DefaultPathEnv is unix style list of directories to search for +// executables. Each directory is separated from the next by a colon +// ':' character . +// For Windows containers, an empty string is returned as the default +// path will be set by the container, and Docker has no context of what the +// default path should be. +// +// Deprecated: use oci.DefaultPathEnv +func DefaultPathEnv(os string) string { + if os == "windows" { + return "" + } + return defaultUnixPathEnv +} diff --git a/plugin/v2/plugin_linux.go b/plugin/v2/plugin_linux.go index 4ad582cd83..0c86b88fc3 100644 --- a/plugin/v2/plugin_linux.go +++ b/plugin/v2/plugin_linux.go @@ -8,7 +8,6 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/oci" - "github.com/docker/docker/pkg/system" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" ) @@ -114,7 +113,7 @@ func (p *Plugin) InitSpec(execRoot string) (*specs.Spec, error) { } envs := make([]string, 1, len(p.PluginObj.Settings.Env)+1) - envs[0] = "PATH=" + system.DefaultPathEnv(runtime.GOOS) + envs[0] = "PATH=" + oci.DefaultPathEnv(runtime.GOOS) envs = append(envs, p.PluginObj.Settings.Env...) args := append(p.PluginObj.Config.Entrypoint, p.PluginObj.Settings.Args...) From ad371893f22ae7b13e34fb6630387347dd834eb3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 00:08:42 +0200 Subject: [PATCH 2/4] pkg/system: move GetLongPathName to integration-cli It's only used for an integration test, and has no external consumers. Signed-off-by: Sebastiaan van Stijn --- integration-cli/docker_cli_build_test.go | 3 +-- integration-cli/utils_unix_test.go | 11 ++++++++++ integration-cli/utils_windows_test.go | 27 ++++++++++++++++++++++++ pkg/system/path_unix.go | 7 ------ pkg/system/path_windows.go | 26 ----------------------- 5 files changed, 39 insertions(+), 35 deletions(-) create mode 100644 integration-cli/utils_unix_test.go create mode 100644 integration-cli/utils_windows_test.go diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index ed04a99ce9..34969a5a7b 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -19,7 +19,6 @@ import ( "github.com/docker/docker/integration-cli/cli" "github.com/docker/docker/integration-cli/cli/build" "github.com/docker/docker/pkg/archive" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/fakecontext" "github.com/docker/docker/testutil/fakegit" @@ -3583,7 +3582,7 @@ func (s *DockerCLIBuildSuite) TestBuildSymlinkBreakout(c *testing.T) { assert.NilError(c, err) // See https://github.com/moby/moby/pull/37770 for reason for next line. - tmpdir, err = system.GetLongPathName(tmpdir) + tmpdir, err = getLongPathName(tmpdir) assert.NilError(c, err) defer os.RemoveAll(tmpdir) diff --git a/integration-cli/utils_unix_test.go b/integration-cli/utils_unix_test.go new file mode 100644 index 0000000000..abb7d2ff83 --- /dev/null +++ b/integration-cli/utils_unix_test.go @@ -0,0 +1,11 @@ +//go:build !windows +// +build !windows + +package main + +// getLongPathName converts Windows short pathnames to full pathnames. +// For example C:\Users\ADMIN~1 --> C:\Users\Administrator. +// It is a no-op on non-Windows platforms +func getLongPathName(path string) (string, error) { + return path, nil +} diff --git a/integration-cli/utils_windows_test.go b/integration-cli/utils_windows_test.go new file mode 100644 index 0000000000..64eee19e6c --- /dev/null +++ b/integration-cli/utils_windows_test.go @@ -0,0 +1,27 @@ +package main + +import "golang.org/x/sys/windows" + +// getLongPathName converts Windows short pathnames to full pathnames. +// For example C:\Users\ADMIN~1 --> C:\Users\Administrator. +// It is a no-op on non-Windows platforms +func getLongPathName(path string) (string, error) { + // See https://groups.google.com/forum/#!topic/golang-dev/1tufzkruoTg + p, err := windows.UTF16FromString(path) + if err != nil { + return "", err + } + b := p // GetLongPathName says we can reuse buffer + n, err := windows.GetLongPathName(&p[0], &b[0], uint32(len(b))) + if err != nil { + return "", err + } + if n > uint32(len(b)) { + b = make([]uint16, n) + _, err = windows.GetLongPathName(&p[0], &b[0], uint32(len(b))) + if err != nil { + return "", err + } + } + return windows.UTF16ToString(b), nil +} diff --git a/pkg/system/path_unix.go b/pkg/system/path_unix.go index 3778cf06d8..3976acf906 100644 --- a/pkg/system/path_unix.go +++ b/pkg/system/path_unix.go @@ -3,13 +3,6 @@ package system // import "github.com/docker/docker/pkg/system" -// GetLongPathName converts Windows short pathnames to full pathnames. -// For example C:\Users\ADMIN~1 --> C:\Users\Administrator. -// It is a no-op on non-Windows platforms -func GetLongPathName(path string) (string, error) { - return path, nil -} - // checkSystemDriveAndRemoveDriveLetter is the non-Windows implementation // of CheckSystemDriveAndRemoveDriveLetter func checkSystemDriveAndRemoveDriveLetter(path string) (string, error) { diff --git a/pkg/system/path_windows.go b/pkg/system/path_windows.go index 708702ee1c..90a2430372 100644 --- a/pkg/system/path_windows.go +++ b/pkg/system/path_windows.go @@ -4,34 +4,8 @@ import ( "fmt" "path/filepath" "strings" - - "golang.org/x/sys/windows" ) -// GetLongPathName converts Windows short pathnames to full pathnames. -// For example C:\Users\ADMIN~1 --> C:\Users\Administrator. -// It is a no-op on non-Windows platforms -func GetLongPathName(path string) (string, error) { - // See https://groups.google.com/forum/#!topic/golang-dev/1tufzkruoTg - p, err := windows.UTF16FromString(path) - if err != nil { - return "", err - } - b := p // GetLongPathName says we can reuse buffer - n, err := windows.GetLongPathName(&p[0], &b[0], uint32(len(b))) - if err != nil { - return "", err - } - if n > uint32(len(b)) { - b = make([]uint16, n) - _, err = windows.GetLongPathName(&p[0], &b[0], uint32(len(b))) - if err != nil { - return "", err - } - } - return windows.UTF16ToString(b), nil -} - // checkSystemDriveAndRemoveDriveLetter is the Windows implementation // of CheckSystemDriveAndRemoveDriveLetter func checkSystemDriveAndRemoveDriveLetter(path string) (string, error) { From c4872b451979284ba64942f3a4df953bdea8cff1 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 7 Oct 2022 23:27:47 +0200 Subject: [PATCH 3/4] pkg/system: CheckSystemDriveAndRemoveDriveLetter: fix error format Signed-off-by: Sebastiaan van Stijn --- pkg/system/path_windows.go | 4 ++-- pkg/system/path_windows_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/system/path_windows.go b/pkg/system/path_windows.go index 90a2430372..495149cd7e 100644 --- a/pkg/system/path_windows.go +++ b/pkg/system/path_windows.go @@ -10,13 +10,13 @@ import ( // of CheckSystemDriveAndRemoveDriveLetter func checkSystemDriveAndRemoveDriveLetter(path string) (string, error) { if len(path) == 2 && string(path[1]) == ":" { - return "", fmt.Errorf("No relative path specified in %q", path) + return "", fmt.Errorf("no relative path specified in %q", path) } if !filepath.IsAbs(path) || len(path) < 2 { return filepath.FromSlash(path), nil } if string(path[1]) == ":" && !strings.EqualFold(string(path[0]), "c") { - return "", fmt.Errorf("The specified path is not on the system drive (C:)") + return "", fmt.Errorf("the specified path is not on the system drive (C:)") } return filepath.FromSlash(path[2:]), nil } diff --git a/pkg/system/path_windows_test.go b/pkg/system/path_windows_test.go index c7cc902065..a6791dc41e 100644 --- a/pkg/system/path_windows_test.go +++ b/pkg/system/path_windows_test.go @@ -11,7 +11,7 @@ import ( func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { // Fails if not C drive. _, err := CheckSystemDriveAndRemoveDriveLetter(`d:\`) - if err == nil || err.Error() != "The specified path is not on the system drive (C:)" { + if err == nil || err.Error() != "the specified path is not on the system drive (C:)" { t.Fatalf("Expected error for d:") } @@ -68,7 +68,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:`); err == nil { t.Fatalf("c: should fail") } - if err.Error() != `No relative path specified in "c:"` { + if err.Error() != `no relative path specified in "c:"` { t.Fatalf(path, err) } @@ -76,7 +76,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { if path, err = CheckSystemDriveAndRemoveDriveLetter(`d:`); err == nil { t.Fatalf("c: should fail") } - if err.Error() != `No relative path specified in "d:"` { + if err.Error() != `no relative path specified in "d:"` { t.Fatalf(path, err) } } From fb7797320148efb84fc8ddade741a9ba27efd82c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 15:28:31 +0200 Subject: [PATCH 4/4] pkg/system: move CheckSystemDriveAndRemoveDriveLetter to pkg/archive This one is a "bit" fuzzy, as it may not be _directly_ related to `archive`, but it's always used _in combination_ with the archive package, so moving it there. Signed-off-by: Sebastiaan van Stijn --- container/archive_windows.go | 5 ++--- daemon/archive_windows.go | 3 +-- pkg/{system => archive}/path.go | 2 +- pkg/{system => archive}/path_unix.go | 2 +- pkg/{system => archive}/path_windows.go | 2 +- pkg/{system => archive}/path_windows_test.go | 5 +---- 6 files changed, 7 insertions(+), 12 deletions(-) rename pkg/{system => archive}/path.go (93%) rename pkg/{system => archive}/path_unix.go (79%) rename pkg/{system => archive}/path_windows.go (90%) rename pkg/{system => archive}/path_windows_test.go (95%) diff --git a/container/archive_windows.go b/container/archive_windows.go index 6631fc69d6..b859493da0 100644 --- a/container/archive_windows.go +++ b/container/archive_windows.go @@ -6,7 +6,6 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/archive" - "github.com/docker/docker/pkg/system" "github.com/pkg/errors" ) @@ -18,8 +17,8 @@ func (container *Container) ResolvePath(path string) (resolvedPath, absPath stri if container.BaseFS == "" { return "", "", errors.New("ResolvePath: BaseFS of container " + container.ID + " is unexpectedly empty") } - // Check if a drive letter supplied, it must be the system drive. - path, err = system.CheckSystemDriveAndRemoveDriveLetter(path) + // Check if a drive letter supplied, it must be the system drive. No-op except on Windows + path, err = archive.CheckSystemDriveAndRemoveDriveLetter(path) if err != nil { return "", "", err } diff --git a/daemon/archive_windows.go b/daemon/archive_windows.go index ba6e17c98f..2e7742c61c 100644 --- a/daemon/archive_windows.go +++ b/daemon/archive_windows.go @@ -14,7 +14,6 @@ import ( "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/ioutils" - "github.com/docker/docker/pkg/system" ) // containerStatPath stats the filesystem resource at the specified path in this @@ -172,7 +171,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path path = filepath.FromSlash(path) // Check if a drive letter supplied, it must be the system drive. No-op except on Windows - path, err = system.CheckSystemDriveAndRemoveDriveLetter(path) + path, err = archive.CheckSystemDriveAndRemoveDriveLetter(path) if err != nil { return err } diff --git a/pkg/system/path.go b/pkg/archive/path.go similarity index 93% rename from pkg/system/path.go rename to pkg/archive/path.go index 0974373d59..888a697581 100644 --- a/pkg/system/path.go +++ b/pkg/archive/path.go @@ -1,4 +1,4 @@ -package system // import "github.com/docker/docker/pkg/system" +package archive // CheckSystemDriveAndRemoveDriveLetter verifies that a path, if it includes a drive letter, // is the system drive. diff --git a/pkg/system/path_unix.go b/pkg/archive/path_unix.go similarity index 79% rename from pkg/system/path_unix.go rename to pkg/archive/path_unix.go index 3976acf906..0b135aea75 100644 --- a/pkg/system/path_unix.go +++ b/pkg/archive/path_unix.go @@ -1,7 +1,7 @@ //go:build !windows // +build !windows -package system // import "github.com/docker/docker/pkg/system" +package archive // checkSystemDriveAndRemoveDriveLetter is the non-Windows implementation // of CheckSystemDriveAndRemoveDriveLetter diff --git a/pkg/system/path_windows.go b/pkg/archive/path_windows.go similarity index 90% rename from pkg/system/path_windows.go rename to pkg/archive/path_windows.go index 495149cd7e..7e18c8e449 100644 --- a/pkg/system/path_windows.go +++ b/pkg/archive/path_windows.go @@ -1,4 +1,4 @@ -package system // import "github.com/docker/docker/pkg/system" +package archive import ( "fmt" diff --git a/pkg/system/path_windows_test.go b/pkg/archive/path_windows_test.go similarity index 95% rename from pkg/system/path_windows_test.go rename to pkg/archive/path_windows_test.go index a6791dc41e..27d7c9a8f5 100644 --- a/pkg/system/path_windows_test.go +++ b/pkg/archive/path_windows_test.go @@ -1,7 +1,4 @@ -//go:build windows -// +build windows - -package system // import "github.com/docker/docker/pkg/system" +package archive import ( "testing"