Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't set a default PATH for Windows #3158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Don't set a default PATH for Windows #3158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions client/build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"os"
"path/filepath"
"runtime"
"strconv"
"strings"
"syscall"
Expand Down Expand Up @@ -1363,21 +1364,22 @@ func testClientGatewayContainerPlatformPATH(t *testing.T, sb integration.Sandbox
Platform *pb.Platform
Expected string
}{{
"default path",
nil,
utilsystem.DefaultPathEnvUnix,
Name: "default path",
Expected: utilsystem.DefaultPathEnvUnix,
}, {
"linux path",
&pb.Platform{OS: "linux"},
utilsystem.DefaultPathEnvUnix,
Name: "linux path",
Platform: &pb.Platform{OS: "linux"},
Expected: utilsystem.DefaultPathEnvUnix,
}, {
"windows path",
&pb.Platform{OS: "windows"},
utilsystem.DefaultPathEnvWindows,
Name: "windows path",
Platform: &pb.Platform{OS: "windows"},
}}

for _, tt := range tests {
t.Run(tt.Name, func(t *testing.T) {
if tt.Platform != nil && tt.Platform.OS != runtime.GOOS {
t.Skipf("skip %s test on %s", tt.Platform.OS, runtime.GOOS)
}
ctr, err := c.NewContainer(ctx, client.NewContainerRequest{
Mounts: []client.Mount{{
Dest: "/",
Expand Down
4 changes: 3 additions & 1 deletion client/llb/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,9 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, []
} else if e.constraints.Platform != nil {
os = e.constraints.Platform.OS
}
env = env.SetDefault("PATH", system.DefaultPathEnv(os))
if v, ok := system.DefaultPathEnv(os); ok {
env = env.SetDefault("PATH", v)
}
} else {
addCap(&e.constraints, pb.CapExecMetaSetsDefaultPath)
}
Expand Down
4 changes: 3 additions & 1 deletion exporter/containerimage/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,9 @@ func defaultImageConfig() ([]byte, error) {
img.Variant = pl.Variant
img.RootFS.Type = "layers"
img.Config.WorkingDir = "/"
img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(pl.OS)}
if env, ok := system.DefaultPathEnv(pl.OS); ok {
img.Config.Env = []string{"PATH=" + env}
}
dt, err := json.Marshal(img)
return dt, errors.Wrap(err, "failed to create empty image config")
}
Expand Down
4 changes: 3 additions & 1 deletion frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
if d.platform != nil {
osName = d.platform.OS
}
d.image.Config.Env = append(d.image.Config.Env, "PATH="+system.DefaultPathEnv(osName))
if env, ok := system.DefaultPathEnv(osName); ok {
d.image.Config.Env = append(d.image.Config.Env, "PATH="+env)
}
}

// initialize base metadata from image conf
Expand Down
4 changes: 3 additions & 1 deletion frontend/dockerfile/dockerfile2llb/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ func emptyImage(platform ocispecs.Platform) dockerspec.DockerOCIImage {
img.Variant = platform.Variant
img.RootFS.Type = "layers"
img.Config.WorkingDir = "/"
img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(platform.OS)}
if env, ok := system.DefaultPathEnv(platform.OS); ok {
img.Config.Env = []string{"PATH=" + env}
}
return img
}
2 changes: 1 addition & 1 deletion frontend/dockerfile/dockerfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,7 +1655,7 @@ COPY Dockerfile .
entrypoint []string
env []string
}{
{p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows"}},
{p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}},
{p: "linux/amd64", entrypoint: []string{"/bin/sh", "-c", "foo bar"}, env: []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}},
} {
t.Run(exp.p, func(t *testing.T) {
Expand Down
4 changes: 3 additions & 1 deletion frontend/gateway/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,9 @@ func (gwCtr *gatewayContainer) Start(ctx context.Context, req client.StartReques
if procInfo.Meta.Cwd == "" {
procInfo.Meta.Cwd = "/"
}
procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnv(gwCtr.platform.OS))
if env, ok := utilsystem.DefaultPathEnv(gwCtr.platform.OS); ok {
procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", env)
}
if req.Tty {
procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "TERM", "xterm")
}
Expand Down
4 changes: 3 additions & 1 deletion solver/llbsolver/ops/exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,9 @@ func (e *ExecOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu
if e.platform != nil {
currentOS = e.platform.OS
}
meta.Env = addDefaultEnvvar(meta.Env, "PATH", utilsystem.DefaultPathEnv(currentOS))
if env, ok := utilsystem.DefaultPathEnv(currentOS); ok {
meta.Env = addDefaultEnvvar(meta.Env, "PATH", env)
}

secretEnv, err := e.loadSecretEnv(ctx, g)
if err != nil {
Expand Down
34 changes: 27 additions & 7 deletions util/system/path.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,36 @@ import (
// ':' character .
const DefaultPathEnvUnix = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"

// DefaultPathEnvWindows is windows style list of directories to search for
// executables. Each directory is separated from the next by a colon
// ';' character .
const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows"
thaJeztah marked this conversation as resolved.
Show resolved Hide resolved
// DefaultPathEnvWindows is deliberately empty on Windows and the default path
// must be set by the image. BuildKit has no context of what the default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand where that "must be set by the image" is coming from. Yes, in wcow environment variables can be part of the rootfs tars because of the registry files, but they can also be in image configuration.

I don't see any reason to add these exceptions to the codebase where they need to be understood by the devs and also by people writing the Dockerfiles. To have a reasonable behavior that works with ENV in Dockerfiles the images should define PATH in the config and users who wish to change it should do it with ENV command, the same way as the Linux users do. Yes, there is another method to do it but it doesn't mean it needs to be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows is... different.

First of all, Windows doesn't support FROM scratch, so every Windows image will have a "Windows" flavour as base image. It's up to that base image to define the PATH (in whatever form it does).

So unless your creating a base image (which means you're either Working for Microsoft, or violating your Windows License), the PATH should be defined by the base image, so the "default" is determined by the base image.

Of course, as an image author you may override that default, but BuildKit itself should not try to come up with a default, because there isn't any.

To be fair, I don't think we should've set a default for non-Windows images either (and also have let it up to the base image (if any)), but that ship sailed. Changing that would be a breaking change, but so is #1747, which changed the default to be different from any container system created in the past 7 Years).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the base image should set the PATH. The default we have should only be a fallback that is almost never used(it is almost never used in Linux as well). People who create Windows images should set the PATH in the image config the same way people who create Linux images do, not ask tools to add and maintain a bunch of exceptions for them. This is also confusing for the users when ENV PATH=$PATH has unexpected behavior.

Copy link
Member Author

@thaJeztah thaJeztah Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that (but don't ask me about details, @TBBle or @kevpar may be able to explain it better), that setting a PATH breaks things on Windows.

For example, take the image config for mcr.microsoft.com/windows/servercore:ltsc2022 does NOT have a PATH;

docker buildx imagetools inspect --raw mcr.microsoft.com/windows/servercore:ltsc2022@sha256:06bb218927a8a95b424acfad0e0e6ae0fb5694c0b510f9930ed5881075eaec2f | jq .
{
  "architecture": "amd64",
  "config": {
    "Hostname": "",
    "Domainname": "",
    "User": "",
    "AttachStdin": false,
    "AttachStdout": false,
    "AttachStderr": false,
    "Tty": false,
    "OpenStdin": false,
    "StdinOnce": false,
    "Env": null,
    "Cmd": [
      "c:\\windows\\system32\\cmd.exe"
    ],
    "Image": "",
    "Volumes": null,
    "WorkingDir": "",
    "Entrypoint": null,
    "OnBuild": null,
    "Labels": null
  },
  "created": "2022-10-07T22:13:48.3974633Z",
  "history": [
    {
      "created": "2022-04-22T01:12:09.4542389Z",
      "created_by": "Apply image 10.0.20348.643"
    },
    {
      "created": "2022-10-07T22:13:48.3974633Z",
      "created_by": "Install update 10.0.20348.1129"
    }
  ],
  "os": "windows",
  "os.version": "10.0.20348.1129",
  "rootfs": {
    "type": "layers",
    "diff_ids": [
      "sha256:b9bda40f596a8f8648c218b23e552acf9498c039eb0a48c222cfcd5a9af9e841",
      "sha256:7a72bd51b9c8b0510fea5addf0b46e5de102813083abe316d4603a050e7ad473"
    ]
  }
}

When I tried to update the classic builder to use the default PATH from this repository, things break. Not exactly sure, but I somewhat expect that in this case either cmd or PowerShell define the path, but when setting a PATH as environment variable, that overrides that default, rendering the image broken;

Step 1/13 : ARG WINDOWS_BASE_IMAGE=mcr.microsoft.com/windows/servercore
Step 2/13 : ARG WINDOWS_BASE_IMAGE_TAG=ltsc2022
Step 3/13 : ARG BUSYBOX_VERSION=FRP-3329-gcf0fa4d13
Step 4/13 : ARG BUSYBOX_SHA256SUM=bfaeb88638e580fc522a68e69072e305308f9747563e51fa085eec60ca39a5ae
Step 5/13 : FROM ${WINDOWS_BASE_IMAGE}:${WINDOWS_BASE_IMAGE_TAG}
ltsc2022: Pulling from windows/servercore
97f65a0ec59e: Pulling fs layer
4486102fd382: Pulling fs layer
4486102fd382: Verifying Checksum
4486102fd382: Download complete
97f65a0ec59e: Verifying Checksum
97f65a0ec59e: Download complete
97f65a0ec59e: Pull complete
4486102fd382: Pull complete
Digest: sha256:2846453bcfaee661d563e29d6513191dccf7280bb826b2da9d6e7226fba2db94
Status: Downloaded newer image for mcr.microsoft.com/windows/servercore:ltsc2022
 ---> 2caccf6918d4
Step 6/13 : RUN mkdir C:\tmp && mkdir C:\bin
 ---> Running in 320b82116e6d
Removing intermediate container 320b82116e6d
 ---> 5e9277f6f4b4
Step 7/13 : ARG BUSYBOX_VERSION
 ---> Running in a85cdf300ed4
Removing intermediate container a85cdf300ed4
 ---> a0d354994a80
Step 8/13 : ARG BUSYBOX_SHA256SUM
 ---> Running in 99f510599835
Removing intermediate container 99f510599835
 ---> ae8d3ccee635
Step 9/13 : ADD https://frippery.org/files/busybox/busybox-w32-${BUSYBOX_VERSION}.exe /bin/busybox.exe

 ---> 0c2ef53e2a7c
Step 10/13 : RUN powershell     if ((Get-FileHash -Path /bin/busybox.exe -Algorithm SHA256).Hash -ne $Env:BUSYBOX_SHA256SUM) {         Throw \"Checksum validation failed\"     }
 ---> Running in 65d36517920b
'powershell' is not recognized as an internal or external command,
operable program or batch file.
The command 'cmd /S /C powershell     if ((Get-FileHash -Path /bin/busybox.exe -Algorithm SHA256).Hash -ne $Env:BUSYBOX_SHA256SUM) {         Throw \"Checksum validation failed\"     }' returned a non-zero code: 1

I guess this is where the (LONG ) discussions we had about moby/moby#29048 come to play; the PATH environment variable cannot be read from the environment, but must be read back from the process that's running (e.g. PowerShell or cmd.exe).

So probably the only options for the builder are;

  • Don't set PATH and acknowledge that "we don't know"
  • Execute the container's process and ask it what the variable is (implement GETENV)

Copy link
Collaborator

@TBBle TBBle Oct 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The breakage above happens because PowerShell lives in C:\WINDOWS\System32\WindowsPowerShell\v1.0\ which is in-the path on systems with PowerShell present (and hence not on nanoserver). See also microsoft/terminal#7418 for other env-var traps in process launching (again, with PowerShell).

moby/moby#31525 (comment) seems to be the best reference for how we got here. The ENV in Dockerfile and the registry-held persistent env-vars (both machine and user) are parallel sources of truth, and the simplest workaround is to just not use ENV PATH in Windows Dockerfiles, in favour of RUN cmd /c setx /m PATH=${PATH};.... (Or RUN reg <options that append to a multi-string value> to save you using ${PATH}, etc.)

This is most-compatible with installers that add paths to the registry as well. Once you've ENV PATH'd once, you then need to ENV PATH after each installer executes too as you lose access to all past and future registry-based env-path changes.

That's not a great workaround, as it puts extra load on the Dockerfile writer, and requires RUN, which blocks non-executable-environment layer-editing tools. That's also true of GETENV as described, although if some brave soul wants to use a hive-loading library, e.g., https://github.com/gabriel-samfira/go-hivex, to implement GETENV (or really env-substitution) without booting a container, that option exists and I think it less-awful than it might seem at-first. (Still not great though).

Along those lines, making Windows-platform builders transform Dockerfile ENV into a RUN setx instead of writing to the container image config would resolve down to a single source of truth, with the same limitations as above and with extra build-time cost of launching another container for the ENV command, which would be surprising to long-time users.

As I understand, there's no good way to tell a Windows process being launched to append an env-var to the registry-provided values, since the Windows process launch takes a single set of env-vars, and it's up to the launching process to assemble them from either copying the existing environment, or requesting a new environment block from the OS and hence using only the registry (basically only explorer and similar process launchers do the latter, and we can't do it outside a running container anyway). This is where Windows's approach differs from Linux, where if you need to append to the path, you add an export PATH=${PATH}; to the end of .bashrc (user) or /etc/environment (system), and hence it's trivially additive across container image layers as those are processed after the Dockerfile ENV, while on Windows, Dockerfile ENV hides the system and user env-vars.

A more esoteric solution would be some wild inversion of the above, which I am not currently volunteering to write: At the end of every RUN, the Windows registry env-vars are exported into the container config ENV block (matching the merge/override behaviours as they are processed in Windows, perhaps with a secret RUN env where we don't keep the scratch layer, or using a hive-reading library). That way Dockerfile ENV is the canonical way to make changes, but changes by installers or setx /m are correctly captured as well.

(Year-later edit: This last idea is me being 6 years late to the table with GETENV as mentioned in the previous comment, and it's already been rejected for performance reasons).

The similar issue around stale env-vars in Windows Terminal tabs has been around for years without successful resolution. It's slightly more complex because its two sources of truth overlap (one contains an old version of the other, so there's a three-way merge in the mix), where here a fairly simple merge is sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding my 2 cents:

There is also the annoying fact that a potential installer may set just the system PATH or the user PATH...or both. So the PATH will differ based on what we set in the USER stanza.

For example:

USER ContainerAdministrator
RUN echo %PATH%
USER gabriel
RUN echo %PATH%

The two RUN commands above may yield different results, and that may be something that is expected by the consumers of the image.

Not setting the PATH as an env var is, I think, desirable on Windows.

Copy link

@puetzk puetzk Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really don't want to do setx /m PATH=${PATH};... either, because that takes expanded value of PATH from the current process environment (where it includes appended per-user values, expanded substitutions of any REG_EXPAND_SZ %OTHER% references, etc) and puts that whole string back into HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment. So your per-user stuff has leaked over to other users, any references are now copied-by-value, your own user values will be appended a second time (and thus become duplicated if setx is used like this a second time...).

// path should be.
//
// Deprecated: Windows images must not have a default PATH set.
const DefaultPathEnvWindows = ""
Comment on lines +20 to +21
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marked this as deprecated, because there is no default to be set


func DefaultPathEnv(os string) string {
// DefaultPathEnv returns the default value for the PATH environment-variable
// if available for the given operating-system. The second return variable
// indicates whether a default exists.
//
// On Linux/Unix, this returns [DefaultPathEnvUnix]. On Windows, no default
// exists, and PATH must not be set as its image-dependent, and must be
// configured through other means.
//
// More details about default PATH values for Windows images can be found
// in the following GitHub discussions:
//
// - [moby/moby#13833]
// - [moby/buildkit#3158]
// - [containerd/containerd#9118]
//
// [moby/moby#13833]: https://github.com/moby/moby/pull/13833
// [moby/buildkit#3158]: https://github.com/moby/buildkit/pull/3158
// [containerd/containerd#9118]: https://github.com/containerd/containerd/pull/9118
func DefaultPathEnv(os string) (string, bool) {
if os == "windows" {
return DefaultPathEnvWindows
return "", false
}
return DefaultPathEnvUnix
return DefaultPathEnvUnix, true
}
Comment on lines +23 to 46
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And updated this function to return a bool to indicate whether there is a default; also tried to capture some of this in the function's GoDoc, and links to relevant discussions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like doing so broke tests; is PATH already set somehow? (but to a linux default path?)

Screenshot 2023-11-06 at 09 46 07

Copy link
Member Author

@thaJeztah thaJeztah Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, hm, right, so test is running a busybox image on Linux;

buildkit/client/build_test.go

Lines 1370 to 1371 in 6a0cd7c

func testClientGatewayContainerPlatformPATH(t *testing.T, sb integration.Sandbox) {
requiresLinux(t)

And checks the output of echo $PATH, with the expectation that that is changed because of the windows argument 🤔;

buildkit/client/build_test.go

Lines 1418 to 1421 in 6a0cd7c

pid1, err := ctr.Start(ctx, client.StartRequest{
Args: []string{"/bin/sh", "-c", "echo -n $PATH"},
Stdout: &nopCloser{output},
})

Not sure what should be tested there; at most, we could test if the $PATH matches the original PATH env from the image's config (if set)

Copy link
Collaborator

@TBBle TBBle Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah. Seems like an inappropriate test to me, since we don't actually support RUN steps for Windows images on non-Windows AFAIK. In fact, why doesn't c.NewContainer or ctr.Start fail here? It seems weird to me that this ever got this far, but maybe the "we don't support this" or "inconsistent request for Windows platform on a Linux-annotated image" failure-handling is at a higher level.

We will probably want Windows-side tests to ensure that we aren't breaking PATH (including covering setx /M and setx across a couple of users to ensure we aren't leaking PATH changes across users. Since we don't run the integration test suite on Windows yet, we can probably defer adding that test, and just drop the "windows path" branch of this one.


// NormalizePath cleans the path based on the operating system the path is meant for.
Expand Down
Loading