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.

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

Enable usage of lifecycle image for windows #823

Merged
merged 6 commits into from Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 1 addition & 5 deletions acceptance/acceptance_test.go
Expand Up @@ -616,8 +616,6 @@ func testAcceptance(
var untrustedBuilderName string

it.Before(func() {
h.SkipIf(t, dockerHostOS() == "windows", "untrusted builders are not yet supported for windows builds")
Copy link
Member

Choose a reason for hiding this comment

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

🥳


var err error
untrustedBuilderName, err = createBuilder(
t,
Expand All @@ -630,9 +628,6 @@ func testAcceptance(
})

it.After(func() {
if dockerHostOS() == "windows" {
return
}
h.DockerRmi(dockerCli, untrustedBuilderName)
})

Expand Down Expand Up @@ -2360,6 +2355,7 @@ func assertMockAppRunsWithOutput(t *testing.T, assert h.AssertionManager, repoNa
ctrID := runDockerImageExposePort(t, assert, containerName, repoName)
defer dockerCli.ContainerKill(context.TODO(), containerName, "SIGKILL")
defer dockerCli.ContainerRemove(context.TODO(), containerName, dockertypes.ContainerRemoveOptions{Force: true})

logs, err := dockerCli.ContainerLogs(context.TODO(), ctrID, dockertypes.ContainerLogsOptions{
ShowStdout: true,
ShowStderr: true,
Expand Down
2 changes: 1 addition & 1 deletion acceptance/testdata/mock_app/run
Expand Up @@ -6,7 +6,7 @@ port="${1-8080}"

echo "listening on port $port"

resp=$(echo "HTTP/1.1 200 OK\n" && cat "$PWD"/*-dep /contents*.txt)
resp=$(echo "HTTP/1.1 200 OK\n" && cat "$PWD"/*-deps/*-dep /contents*.txt)
while true; do
nc -l -p "$port" -c "echo \"$resp\""
done
2 changes: 1 addition & 1 deletion acceptance/testdata/mock_app/run.bat
Expand Up @@ -3,6 +3,6 @@
set port=8080
if [%1] neq [] set port=%1

C:\util\server.exe -p %port% -g "%cd%\*-dep, c:\contents*.txt"
C:\util\server.exe -p %port% -g "%cd%\*-deps\*-dep, c:\contents*.txt"


Expand Up @@ -15,7 +15,7 @@ if [[ -f "$platform_dir/env/ENV1_CONTENTS" ]]; then
mkdir "$launch_dir/env1-launch-layer"
contents=$(cat "$platform_dir/env/ENV1_CONTENTS")
echo "$contents" > "$launch_dir/env1-launch-layer/env1-launch-dep"
ln -snf "$launch_dir/env1-launch-layer/env1-launch-dep" env1-launch-dep
ln -snf "$launch_dir/env1-launch-layer" env1-launch-deps
Copy link
Member

Choose a reason for hiding this comment

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

We ended up changing this to match the required behavior for Windows, that you can't create file symlinks as the lowest-privileged user ContainerUser. Now Linux symlinks the deps directory instead of the file (and Windows creates directory Junctions, more details below).

echo "launch = true" > "$launch_dir/env1-launch-layer.toml"
fi

Expand All @@ -25,7 +25,7 @@ if [[ -f "$platform_dir/env/ENV2_CONTENTS" ]]; then
mkdir "$launch_dir/env2-launch-layer"
contents=$(cat "$platform_dir/env/ENV2_CONTENTS")
echo "$contents" > "$launch_dir/env2-launch-layer/env2-launch-dep"
ln -snf "$launch_dir/env2-launch-layer/env2-launch-dep" env2-launch-dep
ln -snf "$launch_dir/env2-launch-layer" env2-launch-deps
echo "launch = true" > "$launch_dir/env2-launch-layer.toml"
fi

Expand Down
Expand Up @@ -10,7 +10,7 @@ if exist %platform_dir%\env\ENV1_CONTENTS (
mkdir %launch_dir%\env1-launch-layer
set /p contents=<%platform_dir%\env\ENV1_CONTENTS
echo !contents!> %launch_dir%\env1-launch-layer\env1-launch-dep
mklink env1-launch-dep %launch_dir%\env1-launch-layer\env1-launch-dep
mklink /j env1-launch-deps %launch_dir%\env1-launch-layer
echo launch = true> %launch_dir%\env1-launch-layer.toml
Copy link
Member

Choose a reason for hiding this comment

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

On Windows, ContainerUser (or any non-admin user) cannot make directory symlinks, but can only make directory junctions. These tests were previously being run as ContainerAdministrator, which isn't very representative of a normal container. I changed the tests fixtures (Linux and Windows) to link the parent directories of the deps instead of the deps themselves.

)

Expand All @@ -20,7 +20,7 @@ if exist %platform_dir%\env\ENV2_CONTENTS (
mkdir %launch_dir%\env2-launch-layer
set /p contents=<%platform_dir%\env\ENV2_CONTENTS
echo !contents!> %launch_dir%\env2-launch-layer\env2-launch-dep
mklink env2-launch-dep %launch_dir%\env2-launch-layer\env2-launch-dep
mklink /j env2-launch-deps %launch_dir%\env2-launch-layer
echo launch = true> %launch_dir%\env2-launch-layer.toml
)

Expand Down
Expand Up @@ -16,20 +16,20 @@ echo "Color: Styled"

mkdir "$launch_dir/launch-layer"
echo "Launch Dep Contents" > "$launch_dir/launch-layer/launch-dep"
ln -snf "$launch_dir/launch-layer/launch-dep" launch-dep
ln -snf "$launch_dir/launch-layer" launch-deps
echo "launch = true" > "$launch_dir/launch-layer.toml"

## makes a cached launch layer
if [[ ! -f "$launch_dir/cached-launch-layer.toml" ]]; then
echo "making cached launch layer"
mkdir "$launch_dir/cached-launch-layer"
echo "Cached Dep Contents" > "$launch_dir/cached-launch-layer/cached-dep"
ln -snf "$launch_dir/cached-launch-layer/cached-dep" cached-dep
ln -snf "$launch_dir/cached-launch-layer" cached-deps
echo "launch = true" > "$launch_dir/cached-launch-layer.toml"
echo "cache = true" >> "$launch_dir/cached-launch-layer.toml"
else
echo "reusing cached launch layer"
ln -snf "$launch_dir/cached-launch-layer/cached-dep" cached-dep
ln -snf "$launch_dir/cached-launch-layer" cached-deps
fi

## adds a process
Expand Down
Expand Up @@ -4,23 +4,23 @@ echo --- Build: Simple Layers Buildpack
set launch_dir=%1

:: makes a launch layer
echo making launch layer
echo making launch layer %launch_dir%\launch-layer
mkdir %launch_dir%\launch-layer
echo Launch Dep Contents > "%launch_dir%\launch-layer\launch-dep
mklink launch-dep %launch_dir%\launch-layer\launch-dep
mklink /j launch-deps %launch_dir%\launch-layer
echo launch = true > %launch_dir%\launch-layer.toml

:: makes a cached launch layer
if not exist %launch_dir%\cached-launch-layer.toml (
echo making cached launch layer
echo making cached launch layer %launch_dir%\cached-launch-layer
mkdir %launch_dir%\cached-launch-layer
echo Cached Dep Contents > %launch_dir%\cached-launch-layer\cached-dep
mklink cached-dep %launch_dir%\cached-launch-layer\cached-dep
mklink /j cached-deps %launch_dir%\cached-launch-layer
echo launch = true > %launch_dir%\cached-launch-layer.toml
echo cache = true >> %launch_dir%\cached-launch-layer.toml
) else (
echo reusing cached launch layer
mklink cached-dep %launch_dir%\cached-launch-layer\cached-dep
echo reusing cached launch layer %launch_dir%\cached-launch-layer
mklink /j cached-deps %launch_dir%\cached-launch-layer
)

:: adds a process
Expand Down
6 changes: 3 additions & 3 deletions acceptance/testdata/mock_stack/windows/build/Dockerfile
@@ -1,8 +1,8 @@
FROM mcr.microsoft.com/windows/nanoserver:1809

# placeholder values until correct values are deteremined
ENV CNB_USER_ID=0
ENV CNB_GROUP_ID=0
# non-zero sets all user-owned directories to BUILTIN\Users
ENV CNB_USER_ID=1
ENV CNB_GROUP_ID=1

Copy link
Member

Choose a reason for hiding this comment

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

Change test stacks to use a UID/GID that more closely match the pack user that's created. This will make imgutil write appropriate layer permissions and the new EnsureVolumeAccess to do the same.

This is also closer to the current samples nanoserver stack

USER ContainerAdministrator

Expand Down
6 changes: 3 additions & 3 deletions acceptance/testdata/mock_stack/windows/run/Dockerfile
Expand Up @@ -9,9 +9,9 @@ FROM mcr.microsoft.com/windows/nanoserver:1809

COPY --from=gobuild /util/server.exe /util/server.exe

# placeholder values until correct values are deteremined
ENV CNB_USER_ID=0
ENV CNB_GROUP_ID=0
# non-zero sets all user-owned directories to BUILTIN\Users
ENV CNB_USER_ID=1
ENV CNB_GROUP_ID=1

USER ContainerAdministrator

Expand Down
4 changes: 0 additions & 4 deletions build.go
Expand Up @@ -311,10 +311,6 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
}

func lifecycleImageSupported(builderOS string, lifecycleVersion *builder.Version) bool {
if builderOS == "windows" {
return false
}

return lifecycleVersion.Equal(builder.VersionMustParse(prevLifecycleVersionSupportingImage)) ||
!lifecycleVersion.LessThan(semver.MustParse(minLifecycleVersionSupportingImage))
}
Expand Down
12 changes: 0 additions & 12 deletions build_test.go
Expand Up @@ -1447,18 +1447,6 @@ func testBuild(t *testing.T, when spec.G, it spec.S) {
})

when("builder is untrusted", func() {
when("building Windows containers", func() {
it("errors and mentions that builder must be trusted", func() {
Copy link
Member

Choose a reason for hiding this comment

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

is there another test in the section that tries to build Windows containers? If not, can we add one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests wouldn't exercise any new code paths due to mocking. Now that we've removed guard that returned an error for windows containers, there are no more windows-specific code paths.

defaultBuilderImage.SetPlatform("windows", "", "")
h.AssertError(t, subject.Build(context.TODO(), BuildOptions{
Image: "some/app",
Builder: defaultBuilderName,
Publish: true,
TrustBuilder: false,
}), "does not have an associated lifecycle image. Builder must be trusted.")
})
})

when("lifecycle image is available", func() {
it("uses the 5 phases with the lifecycle image", func() {
h.AssertNil(t, subject.Build(context.TODO(), BuildOptions{
Expand Down
133 changes: 103 additions & 30 deletions internal/build/container_ops.go
Expand Up @@ -8,43 +8,39 @@ import (
"io/ioutil"
"os"
"runtime"
"strings"

"github.com/BurntSushi/toml"
"github.com/docker/docker/api/types"
dcontainer "github.com/docker/docker/api/types/container"
"github.com/docker/docker/client"
"github.com/pkg/errors"

"github.com/buildpacks/pack/internal/paths"

"github.com/buildpacks/pack/internal/archive"
"github.com/buildpacks/pack/internal/builder"
"github.com/buildpacks/pack/internal/container"
"github.com/buildpacks/pack/internal/style"
)

type ContainerOperation func(ctrClient client.CommonAPIClient, ctx context.Context, containerID string, stdout, stderr io.Writer) error

// CopyDir copies a local directory (src) to the destination on the container while filtering files and changing it's UID/GID.
func CopyDir(src, dst string, uid, gid int, fileFilter func(string) bool) ContainerOperation {
func CopyDir(src, dst string, uid, gid int, os string, fileFilter func(string) bool) ContainerOperation {
return func(ctrClient client.CommonAPIClient, ctx context.Context, containerID string, stdout, stderr io.Writer) error {
info, err := ctrClient.Info(ctx)
if err != nil {
return err
tarPath := dst
if os == "windows" {
tarPath = paths.WindowsToSlash(dst)
}
if info.OSType == "windows" {
readerDst := strings.ReplaceAll(dst, `\`, "/")[2:] // Strip volume, convert slashes to conform to TAR format
reader, err := createReader(src, readerDst, uid, gid, fileFilter)
if err != nil {
return errors.Wrapf(err, "create tar archive from '%s'", src)
}
defer reader.Close()
return copyDirWindows(ctx, ctrClient, containerID, reader, dst, stdout, stderr)
}
reader, err := createReader(src, dst, uid, gid, fileFilter)

reader, err := createReader(src, tarPath, uid, gid, fileFilter)
if err != nil {
return errors.Wrapf(err, "create tar archive from '%s'", src)
}
defer reader.Close()

if os == "windows" {
return copyDirWindows(ctx, ctrClient, containerID, reader, dst, stdout, stderr)
}
return copyDir(ctx, ctrClient, containerID, reader)
}
}
Expand Down Expand Up @@ -76,17 +72,13 @@ func copyDir(ctx context.Context, ctrClient client.CommonAPIClient, containerID
// for Windows containers and does not work. Instead, we perform the copy from inside a container
// using xcopy.
// See: https://github.com/moby/moby/issues/40771
func copyDirWindows(ctx context.Context, ctrClient client.CommonAPIClient, containerID string, appReader io.Reader, dst string, stdout, stderr io.Writer) error {
func copyDirWindows(ctx context.Context, ctrClient client.CommonAPIClient, containerID string, reader io.Reader, dst string, stdout, stderr io.Writer) error {
info, err := ctrClient.ContainerInspect(ctx, containerID)
if err != nil {
return err
}

pathElements := strings.Split(dst, `\`)
if len(pathElements) < 1 {
return fmt.Errorf("cannot determine base name for destination path: %s", style.Symbol(dst))
}
baseName := pathElements[len(pathElements)-1]
baseName := paths.WindowsBasename(dst)

mnt, err := findMount(info, dst)
if err != nil {
Expand All @@ -97,10 +89,16 @@ func copyDirWindows(ctx context.Context, ctrClient client.CommonAPIClient, conta
&dcontainer.Config{
Image: info.Image,
Cmd: []string{
"xcopy",
fmt.Sprintf(`c:\windows\%s`, baseName),
dst,
"/e", "/h", "/y", "/c", "/b",
"cmd",
"/c",

//xcopy args
// e - recursively create subdirectories
// h - copy hidden and system files
// b - copy symlinks, do not dereference
// x - copy attributes
// y - suppress prompting
fmt.Sprintf(`xcopy c:\windows\%s %s /e /h /b /x /y`, baseName, dst),
Copy link
Member

Choose a reason for hiding this comment

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

Added better documentation for xcopy command

},
WorkingDir: "/",
User: windowsContainerAdmin,
Expand All @@ -116,7 +114,7 @@ func copyDirWindows(ctx context.Context, ctrClient client.CommonAPIClient, conta
}
defer ctrClient.ContainerRemove(context.Background(), ctr.ID, types.ContainerRemoveOptions{Force: true})

err = ctrClient.CopyToContainer(ctx, ctr.ID, "/windows", appReader, types.CopyToContainerOptions{})
err = ctrClient.CopyToContainer(ctx, ctr.ID, "/windows", reader, types.CopyToContainerOptions{})
if err != nil {
return errors.Wrap(err, "copy app to container")
}
Expand All @@ -136,11 +134,11 @@ func findMount(info types.ContainerJSON, dst string) (types.MountPoint, error) {
return m, nil
}
}
return types.MountPoint{}, errors.New("no matching mount found")
return types.MountPoint{}, fmt.Errorf("no matching mount found for %s", dst)
}

// WriteStackToml writes a `stack.toml` based on the StackMetadata provided to the destination path.
func WriteStackToml(dstPath string, stack builder.StackMetadata) ContainerOperation {
func WriteStackToml(dstPath string, stack builder.StackMetadata, os string) ContainerOperation {
return func(ctrClient client.CommonAPIClient, ctx context.Context, containerID string, stdout, stderr io.Writer) error {
buf := &bytes.Buffer{}
err := toml.NewEncoder(buf).Encode(stack)
Expand All @@ -149,10 +147,21 @@ func WriteStackToml(dstPath string, stack builder.StackMetadata) ContainerOperat
}

tarBuilder := archive.TarBuilder{}
tarBuilder.AddFile(dstPath, 0755, archive.NormalizedDateTime, buf.Bytes())

tarPath := dstPath
if os == "windows" {
tarPath = paths.WindowsToSlash(dstPath)
}

tarBuilder.AddFile(tarPath, 0755, archive.NormalizedDateTime, buf.Bytes())
reader := tarBuilder.Reader(archive.DefaultTarWriterFactory())
defer reader.Close()

if os == "windows" {
dirName := paths.WindowsDir(dstPath)
return copyDirWindows(ctx, ctrClient, containerID, reader, dirName, stdout, stderr)
}

return ctrClient.CopyToContainer(ctx, containerID, "/", reader, types.CopyToContainerOptions{})
}
}
Expand All @@ -174,3 +183,67 @@ func createReader(src, dst string, uid, gid int, fileFilter func(string) bool) (

return archive.ReadZipAsTar(src, dst, uid, gid, -1, false, fileFilter), nil
}

//EnsureVolumeAccess grants full access permissions to volumes for UID/GID-based user
//When UID/GID are 0 it grants explicit full access to BUILTIN\Administrators and any other UID/GID grants full access to BUILTIN\Users
//Changing permissions on volumes through stopped containers does not work on Docker for Windows so we start the container and make change using icacls
//See: https://github.com/moby/moby/issues/40771
func EnsureVolumeAccess(uid, gid int, os string, volumeNames ...string) ContainerOperation {
return func(ctrClient client.CommonAPIClient, ctx context.Context, containerID string, stdout, stderr io.Writer) error {
if os != "windows" {
Copy link
Member

Choose a reason for hiding this comment

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

no-op for Linux, but I wonder if Linux and Windows should eventually be setting appropriate permissions for arbitrary volumes passed in with pack build --volume

return nil
}

containerInfo, err := ctrClient.ContainerInspect(ctx, containerID)
if err != nil {
return err
}

cmd := ""
binds := []string{}
for i, volumeName := range volumeNames {
containerPath := fmt.Sprintf("c:/volume-mnt-%d", i)
binds = append(binds, fmt.Sprintf("%s:%s", volumeName, containerPath))

if cmd != "" {
cmd += "&&"
}

//icacls args
// /grant - add new permissions instead of replacing
// (OI) - object inherit
// (CI) - container inherit
// F - full access
// /t - recursively apply
// /l - perform on a symbolic link itself versus its target
// /q - suppress success messages
cmd += fmt.Sprintf(`icacls %s /grant *%s:(OI)(CI)F /t /l /q`, containerPath, paths.WindowsPathSID(uid, gid))
Copy link
Member

Choose a reason for hiding this comment

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

Uses the icacls command which is present on nanoserver and servercore and is likely present for any base image.

}

ctr, err := ctrClient.ContainerCreate(ctx,
&dcontainer.Config{
Image: containerInfo.Image,
Cmd: []string{"cmd", "/c", cmd},
WorkingDir: "/",
User: windowsContainerAdmin,
},
&dcontainer.HostConfig{
Binds: binds,
Isolation: dcontainer.IsolationProcess,
},
nil, "",
)
if err != nil {
return err
}
defer ctrClient.ContainerRemove(context.Background(), ctr.ID, types.ContainerRemoveOptions{Force: true})

return container.Run(
ctx,
ctrClient,
ctr.ID,
ioutil.Discard, // Suppress icacls output
stderr,
)
}
}