Skip to content

Commit

Permalink
fix: wcow: add powershell.exe dir to default PATH
Browse files Browse the repository at this point in the history
The current default `PATH` has been set to the most basic subset for both
`nanoserver` and `servercore`.

```go
const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows;
```

The path to `powershell.exe` is conspicuously missing hence breaking the
developer experience for most workloads that depend on PS.

This fix proposes to append `;C:\\Windows\\System32\\WindowsPowerShell\\v1.0`
to that list to support PS scenarios, that make a big chunk of workloads,
especially legacy ones, migrating from on-prem to cloud.

Fixes moby#4901, microsoft/Windows-Containers#500
Ref moby#3158

Implication for nanoserver:
===========================
This does not any way break the experiences for those using `nanoserver` base image,
as much as PS and the `C:\\Windows\\System32\\WindowsPowerShell\\v1.0` path is not present
on `nanoserver`.

Transition users to using `ENV`:
==============================
Users coming from classic `docker build` will need to transition from using `RUN setx /M`
to `ENV` as a way of persisting environment variables in the images.

We can take a hit on not supporting backward compatibility for `RUN setx /M`, as opposed
to not supporting both `RUN setx /M` and `RUN powershell` as it is the case right now.

Alternative considered:
=======================
We thought of an option to ask the Windows platform team
to add a default `Config.Env.PATH` in the config file for the base image.
However, this causes a regression for `RUN setx /M` on the classic `docker build`.
There could be a couple of more people too that might be depending on `Config.Env = null`
as it is currently. See `docker inspect mcr.microsoft.com/windows/servercore:ltsc2022`.

Here is the regression details when we set `ENV` in the base image.

```
PS> type .\Dockerfile
FROM mcr.microsoft.com/windows/servercore:ltsc2022 AS newbase
ENV PATH="C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps"

FROM newbase
RUN setx /M PATH "C:\my\path;%PATH%"
RUN echo %PATH%

PS> docker build --no-cache -t profnandaa/servercore-path-tests .

Sending build context to Docker daemon  2.048kB
Step 1/5 : FROM mcr.microsoft.com/windows/servercore:ltsc2022 AS newbase
---> e64ba0f4256b
Step 2/5 : ENV PATH="C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps"
---> Running in ab3dc4921d7d
---> Removed intermediate container ab3dc4921d7d
---> f57b0a2d0e28
Step 3/5 : FROM newbase
---> f57b0a2d0e28
Step 4/5 : RUN setx /M PATH "%PATH%;C:\my\path;"
---> Running in 6ca6171334a0

SUCCESS: Specified value was saved.
---> Removed intermediate container 6ca6171334a0
---> 6d2870e2f91d
Step 5/5 : RUN echo %PATH%
---> Running in 785633e2a31c
C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps
   ^
   | ~~~~~~~~~ notice "C:\my\path" is missing, meaning `setx /M` is not persisting as before.

---> Removed intermediate container 785633e2a31c
---> ed490181b903
Successfully built ed490181b903
Successfully tagged profnandaa/servercore-path-tests:latest
```

Current `RUN setx /M` behavior:

```
PS> type .\Dockerfile

FROM mcr.microsoft.com/windows/servercore:ltsc2022
RUN setx /M PATH "%PATH%;C:\my\path;"
RUN echo %PATH%

PS> docker build --no-cache -t profnandaa/servercore-path-tests .

Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM mcr.microsoft.com/windows/servercore:ltsc2022
---> e64ba0f4256b
Step 2/3 : RUN setx /M PATH "C:\my\path;%PATH%"
---> Running in 5502dd679495

SUCCESS: Specified value was saved.
---> Removed intermediate container 5502dd679495
---> 0b59f38e2da4
Step 3/3 : RUN echo %PATH%
---> Running in 3bacb0b27bad
C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps;C:\my\path;
                    ^
                    |~~~~ `setx /M` persists.
---> Removed intermediate container 3bacb0b27bad
---> cda1b4cd27ff
Successfully built cda1b4cd27ff
Successfully tagged profnandaa/servercore-path-tests:latest
```

`setx` vs. `ENV` has been discussed in details in moby#5445

Signed-off-by: Anthony Nandaa <[email protected]>
  • Loading branch information
profnandaa committed Nov 6, 2024
1 parent 952ac2a commit af5e909
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 2 deletions.
46 changes: 45 additions & 1 deletion frontend/dockerfile/dockerfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ var allTests = integration.TestFuncs(
testEmptyStages,
testLocalCustomSessionID,
testTargetStageNameArg,
testPowershellInDefaultPathOnWindows,
)

// Tests that depend on the `security.*` entitlements
Expand Down Expand Up @@ -1782,7 +1783,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"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows;C:\\Windows\\System32\\WindowsPowerShell\\v1.0"}},
{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 Expand Up @@ -1919,6 +1920,49 @@ COPY --from=base /out/ /
require.Equal(t, "value:final", string(dt))
}

func testPowershellInDefaultPathOnWindows(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "!windows")

f := getFrontend(t, sb)

// just testing that the powershell path is in PATH
// but not testing powershell itself since it will need
// servercore image that is too bulky for just one single test.
dockerfile := []byte(`
FROM nanoserver
USER ContainerAdministrator
RUN echo %PATH% > env_path.txt
`)
dir := integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)
c, err := client.New(sb.Context(), sb.Address())
require.NoError(t, err)
defer c.Close()

destDir := t.TempDir()
_, err = f.Solve(sb.Context(), c, client.SolveOpt{
Exports: []client.ExportEntry{
{
Type: client.ExporterLocal,
OutputDir: destDir,
},
},
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
}, nil)
require.NoError(t, err)

dt, err := os.ReadFile(filepath.Join(destDir, "env_path.txt"))
require.NoError(t, err)

envPath := string(dt)
require.Contains(t, envPath, "C:\\Windows\\System32\\WindowsPowerShell\\v1.0")
}

func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "windows")
workers.CheckFeatureCompat(t, sb, workers.FeatureOCIExporter, workers.FeatureMultiPlatform)
Expand Down
2 changes: 1 addition & 1 deletion util/system/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const DefaultPathEnvUnix = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/s
// 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"
const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows;C:\\Windows\\System32\\WindowsPowerShell\\v1.0"

func DefaultPathEnv(os string) string {
if os == "windows" {
Expand Down

0 comments on commit af5e909

Please sign in to comment.