Skip to content

Commit

Permalink
fix: wcow: add powershell.exe dir to default PATH for backward compat…
Browse files Browse the repository at this point in the history 
…ibility

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 #4901, microsoft/Windows-Containers#500
Ref #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 #5445

Signed-off-by: Anthony Nandaa <[email protected]>
  • Loading branch information
@profnandaa
profnandaa committed Oct 31, 2024
1 parent 4622a2b commit aa093d3
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 89 deletions.
102 changes: 15 additions & 87 deletions frontend/dockerfile/dockerfile_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ var allTests = integration.TestFuncs(
testHistoryFinalizeTrace,
testEmptyStages,
testLocalCustomSessionID,
testTargetStageNameArg,
testPowershellInDefaultPathOnWindows,
)

// Tests that depend on the `security.*` entitlements
Expand Down Expand Up @@ -1782,7 +1782,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 All @@ -1804,119 +1804,47 @@ COPY Dockerfile .
}
}

func testTargetStageNameArg(t *testing.T, sb integration.Sandbox) {
integration.SkipOnPlatform(t, "windows")
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 alpine AS base
WORKDIR /out
RUN echo -n "value:$TARGETSTAGE" > /out/first
ARG TARGETSTAGE
RUN echo -n "value:$TARGETSTAGE" > /out/second
FROM scratch AS foo
COPY --from=base /out/ /
FROM scratch
COPY --from=base /out/ /
FROM nanoserver
USER ContainerAdministrator
RUN echo %PATH% > env_path.txt
`)

dir := integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

destDir := t.TempDir()

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{
LocalMounts: map[string]fsutil.FS{
dockerui.DefaultLocalNameDockerfile: dir,
dockerui.DefaultLocalNameContext: dir,
},
FrontendAttrs: map[string]string{
"target": "foo",
},
Exports: []client.ExportEntry{
{
Type: client.ExporterLocal,
OutputDir: destDir,
},
},
}, nil)
require.NoError(t, err)

dt, err := os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Equal(t, "value:", string(dt))

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:foo", string(dt))

destDir = t.TempDir()

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

dt, err = os.ReadFile(filepath.Join(destDir, "first"))
require.NoError(t, err)
require.Equal(t, "value:", string(dt))

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:default", string(dt))

// stage name defined in Dockerfile but not passed in request
dockerfile = append(dockerfile, []byte(`
FROM scratch AS final
COPY --from=base /out/ /
`)...)

dir = integration.Tmpdir(
t,
fstest.CreateFile("Dockerfile", dockerfile, 0600),
)

destDir = t.TempDir()

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

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

dt, err = os.ReadFile(filepath.Join(destDir, "second"))
require.NoError(t, err)
require.Equal(t, "value:final", string(dt))
envPath := string(dt)
require.Contains(t, envPath, "C:\\Windows\\System32\\WindowsPowerShell\\v1.0")
}

func testExportMultiPlatform(t *testing.T, sb integration.Sandbox) {
Expand Down
2 changes: 1 addition & 1 deletion util/system/path.go
View file
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
11 changes: 10 additions & 1 deletion util/testutil/integration/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,16 @@ func prepareValueMatrix(tc testConf) []matrixValue {

// Skips tests on platform
func SkipOnPlatform(t *testing.T, goos string) {
if runtime.GOOS == goos {
skip := false
// support for negation
if strings.HasPrefix(goos, "!") {
goos = strings.TrimPrefix(goos, "!")
skip = runtime.GOOS != goos
} else {
skip = runtime.GOOS == goos
}

if skip {
t.Skipf("Skipped on %s", goos)
}
}
Expand Down

0 comments on commit aa093d3

Please sign in to comment.