From 5b2130c403be90f3d96915bdc245d17f2f174d74 Mon Sep 17 00:00:00 2001 From: Victoria Henry Date: Thu, 27 Aug 2020 12:43:32 -0700 Subject: [PATCH 1/6] Enable usage of lifecycle image for windows Signed-off-by: Andrew Meyer Signed-off-by: Malini Valliath Signed-off-by: Victoria Henry Signed-off-by: Anthony Emengo --- acceptance/acceptance_test.go | 6 +---- build.go | 4 --- build_test.go | 12 --------- internal/build/container_ops.go | 48 +++++++++++++++++++++------------ internal/container/run.go | 12 +++++++++ 5 files changed, 44 insertions(+), 38 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index aa941894db..53eac1938c 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -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") - var err error untrustedBuilderName, err = createBuilder( t, @@ -630,9 +628,6 @@ func testAcceptance( }) it.After(func() { - if dockerHostOS() == "windows" { - return - } h.DockerRmi(dockerCli, untrustedBuilderName) }) @@ -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, diff --git a/build.go b/build.go index 81e3c4c889..cc84c76a25 100644 --- a/build.go +++ b/build.go @@ -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)) } diff --git a/build_test.go b/build_test.go index 991b3565b0..02c176bc6f 100644 --- a/build_test.go +++ b/build_test.go @@ -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() { - 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{ diff --git a/internal/build/container_ops.go b/internal/build/container_ops.go index aa659f4990..fa16aa5e6b 100644 --- a/internal/build/container_ops.go +++ b/internal/build/container_ops.go @@ -19,7 +19,6 @@ import ( "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 @@ -32,13 +31,12 @@ func CopyDir(src, dst string, uid, gid int, fileFilter func(string) bool) Contai return err } 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) + reader, err := createReader(src, winPathToTarPath(dst), 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) + return copyWindows(ctx, ctrClient, containerID, reader, dst, stdout, stderr) } reader, err := createReader(src, dst, uid, gid, fileFilter) if err != nil { @@ -71,24 +69,26 @@ func copyDir(ctx context.Context, ctrClient client.CommonAPIClient, containerID return err } -// copyDirWindows provides an alternate, Windows container-specific implementation of copyDir. +// copyWindows provides an alternate, Windows container-specific implementation of copyDir. // This implementation is needed because copying directly to a mounted volume is currently buggy // 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 copyWindows(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)) + fileOrDir := "d" + findDst := dst + if strings.HasSuffix(dst, ".toml") { + fileOrDir = "f" + pathElements := strings.Split(dst, `\`) + findDst = strings.Join(pathElements[:len(pathElements)-1], `\`) // parent of file } - baseName := pathElements[len(pathElements)-1] - mnt, err := findMount(info, dst) + mnt, err := findMount(info, findDst) if err != nil { return err } @@ -97,10 +97,9 @@ 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", + fmt.Sprintf(`echo %s|xcopy /e /h /y /c /b c:\windows\%s %s`, fileOrDir, dst[3:], dst), }, WorkingDir: "/", User: windowsContainerAdmin, @@ -116,7 +115,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") } @@ -149,14 +148,29 @@ func WriteStackToml(dstPath string, stack builder.StackMetadata) ContainerOperat } tarBuilder := archive.TarBuilder{} + + info, err := ctrClient.Info(ctx) + if err != nil { + return err + } + if info.OSType == "windows" { + tarBuilder.AddFile(winPathToTarPath(dstPath), 0755, archive.NormalizedDateTime, buf.Bytes()) + reader := tarBuilder.Reader(archive.DefaultTarWriterFactory()) + defer reader.Close() + return copyWindows(ctx, ctrClient, containerID, reader, dstPath, stdout, stderr) + } + tarBuilder.AddFile(dstPath, 0755, archive.NormalizedDateTime, buf.Bytes()) reader := tarBuilder.Reader(archive.DefaultTarWriterFactory()) defer reader.Close() - return ctrClient.CopyToContainer(ctx, containerID, "/", reader, types.CopyToContainerOptions{}) } } +func winPathToTarPath(path string) string { + return strings.ReplaceAll(path, `\`, "/")[2:] // strip volume, convert slashes +} + func createReader(src, dst string, uid, gid int, fileFilter func(string) bool) (io.ReadCloser, error) { fi, err := os.Stat(src) if err != nil { diff --git a/internal/container/run.go b/internal/container/run.go index 218776a4cd..dd2f8b464d 100644 --- a/internal/container/run.go +++ b/internal/container/run.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "time" "github.com/docker/docker/api/types" dcontainer "github.com/docker/docker/api/types/container" @@ -18,6 +19,17 @@ func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, if err := docker.ContainerStart(ctx, ctrID, types.ContainerStartOptions{}); err != nil { return errors.Wrap(err, "container start") } + + info, err := docker.Info(ctx) + if err != nil { + return errors.Wrap(err, "getting docker info") + } + + if info.OSType == "windows" { + // wait for logs to show + time.Sleep(time.Second) + } + logs, err := docker.ContainerLogs(ctx, ctrID, types.ContainerLogsOptions{ ShowStdout: true, ShowStderr: true, From 6ef83d289fb1b38fc50df9b21bbf9cdb6cff765c Mon Sep 17 00:00:00 2001 From: Micah Young Date: Wed, 2 Sep 2020 18:15:35 -0400 Subject: [PATCH 2/6] Add test coverage for windows container ops Signed-off-by: Micah Young --- internal/build/container_ops.go | 12 +- internal/build/container_ops_test.go | 186 ++++++++++++++---- .../build/testdata/fake-app/fake-app-symlink | 1 + 3 files changed, 162 insertions(+), 37 deletions(-) create mode 120000 internal/build/testdata/fake-app/fake-app-symlink diff --git a/internal/build/container_ops.go b/internal/build/container_ops.go index fa16aa5e6b..afc8245397 100644 --- a/internal/build/container_ops.go +++ b/internal/build/container_ops.go @@ -79,7 +79,6 @@ func copyWindows(ctx context.Context, ctrClient client.CommonAPIClient, containe if err != nil { return err } - fileOrDir := "d" findDst := dst if strings.HasSuffix(dst, ".toml") { @@ -99,7 +98,14 @@ func copyWindows(ctx context.Context, ctrClient client.CommonAPIClient, containe Cmd: []string{ "cmd", "/c", - fmt.Sprintf(`echo %s|xcopy /e /h /y /c /b c:\windows\%s %s`, fileOrDir, dst[3:], dst), + + //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(`echo %s|xcopy /e /h /b /x /y c:\windows\%s %s`, fileOrDir, dst[3:], dst), }, WorkingDir: "/", User: windowsContainerAdmin, @@ -135,7 +141,7 @@ 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. diff --git a/internal/build/container_ops_test.go b/internal/build/container_ops_test.go index 61abcfe0e2..719dbdd423 100644 --- a/internal/build/container_ops_test.go +++ b/internal/build/container_ops_test.go @@ -7,9 +7,13 @@ import ( "math/rand" "path/filepath" "runtime" + "strings" "testing" "time" + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/mount" + dcontainer "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" "github.com/heroku/color" @@ -35,22 +39,30 @@ func TestContainerOperations(t *testing.T) { ctrClient, err = client.NewClientWithOpts(client.FromEnv, client.WithVersion("1.38")) h.AssertNil(t, err) - info, err := ctrClient.Info(context.TODO()) - h.AssertNil(t, err) - h.SkipIf(t, info.OSType == "windows", "These tests are not yet compatible with Windows-based containers") - spec.Run(t, "container-ops", testContainerOps, spec.Report(report.Terminal{}), spec.Sequential()) } func testContainerOps(t *testing.T, when spec.G, it spec.S) { var ( - imageName string - outBuf, errBuf bytes.Buffer + imageName string + outBuf, errBuf bytes.Buffer + isWindowsDaemon bool ) it.Before(func() { imageName = "container-ops.test-" + h.RandString(10) - h.CreateImage(t, ctrClient, imageName, `FROM busybox`) + info, err := ctrClient.Info(context.TODO()) + h.AssertNil(t, err) + isWindowsDaemon = info.OSType == "windows" + + dockerfileContent := `FROM busybox` + if isWindowsDaemon { + dockerfileContent = `FROM mcr.microsoft.com/windows/nanoserver:1809` + } + + h.CreateImage(t, ctrClient, imageName, dockerfileContent) + + h.AssertNil(t, err) }) it.After(func() { @@ -59,11 +71,22 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { when("#CopyDir", func() { it("writes contents with proper owner/permissions", func() { - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), "/some-location", 123, 456, nil) + containerDir := "/some-location" + if isWindowsDaemon { + containerDir = `c:\some-location` + } + + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, nil) ctx := context.Background() - ctr, err := createContainer(ctx, imageName, "ls", "-al", "/some-location") + ctrCmd := []string{"ls", "-al", "/some-location"} + if isWindowsDaemon { + ctrCmd = []string{"cmd", "/c", `dir /q /s c:\some-location`} + } + + ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -71,26 +94,48 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - perms := "-rw-r--r--" - if runtime.GOOS == "windows" { - perms = "-rwxrwxrwx" - } + output := strings.ReplaceAll(outBuf.String(), "\r", "") - output := outBuf.String() - h.AssertContainsMatch(t, output, fmt.Sprintf(` + if isWindowsDaemon { + h.AssertContainsMatch(t, output, ` +(.*) ... . +(.*) ... .. +(.*) 17 ... fake-app-file +(.*) ... fake-app-symlink \[fake-app-file\] +(.*) 0 ... file-to-ignore +`) + } else { + perms := "-rw-r--r--" + if runtime.GOOS == "windows" { + perms = "-rwxrwxrwx" + } + h.AssertContainsMatch(t, output, fmt.Sprintf(` %s 1 123 456 (.*) fake-app-file +%s 1 123 456 (.*) fake-app-symlink -> fake-app-file %s 1 123 456 (.*) file-to-ignore -`, perms, perms)) +`, perms, "lrwxrwxrwx", perms)) + } }) it("writes contents ignoring from file filter", func() { - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), "/some-location", 123, 456, func(filename string) bool { + containerDir := "/some-location" + if isWindowsDaemon { + containerDir = `c:\some-location` + } + + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, func(filename string) bool { return filepath.Base(filename) != "file-to-ignore" }) - ctx := context.Background() - ctr, err := createContainer(ctx, imageName, "ls", "-al", "/some-location") + ctrCmd := []string{"ls", "-al", "/some-location"} + if isWindowsDaemon { + ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-location`} + } + + ctx := context.Background() + ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -98,16 +143,28 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := outBuf.String() + output := strings.ReplaceAll(outBuf.String(), "\r", "") + h.AssertNotContains(t, output, "file-to-ignore") }) it("writes contents from zip file", func() { - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app.zip"), "/some-location", 123, 456, nil) - ctx := context.Background() + containerDir := "/some-location" + if isWindowsDaemon { + containerDir = `c:\some-location` + } - ctr, err := createContainer(ctx, imageName, "ls", "-al", "/some-location") + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app.zip"), containerDir, 123, 456, nil) + + ctrCmd := []string{"ls", "-al", "/some-location"} + if isWindowsDaemon { + ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-location`} + } + + ctx := context.Background() + ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -115,16 +172,32 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := outBuf.String() - h.AssertContainsMatch(t, output, ` + output := strings.ReplaceAll(outBuf.String(), "\r", "") + + if isWindowsDaemon { + h.AssertContainsMatch(t, output, ` +(.*) ... . +(.*) ... .. +(.*) 17 ... fake-app-file +`) + } else { + h.AssertContainsMatch(t, output, ` -rw-r--r-- 1 123 456 (.*) fake-app-file `) + } }) }) when("#WriteStackToml", func() { it("writes file", func() { - writeOp := build.WriteStackToml("/some/stack.toml", builder.StackMetadata{ + containerDir := "/some" + containerPath := "/some/stack.toml" + if isWindowsDaemon { + containerDir = `c:\some` + containerPath = `c:\some\stack.toml` + } + + writeOp := build.WriteStackToml(containerPath, builder.StackMetadata{ RunImage: builder.RunImageMetadata{ Image: "image-1", Mirrors: []string{ @@ -133,9 +206,16 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { }, }, }) + + ctrCmd := []string{"ls", "-al", "/some/stack.toml"} + if isWindowsDaemon { + ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some\stack.toml`} + } + ctx := context.Background() - ctr, err := createContainer(ctx, imageName, "ls", "-al", "/some/stack.toml") + ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -143,12 +223,24 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := outBuf.String() - h.AssertContains(t, output, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) + output := strings.ReplaceAll(outBuf.String(), "\r", "") + + if isWindowsDaemon { + h.AssertContains(t, output, `12/31/1979 05:00 PM 69 ... stack.toml`) + } else { + h.AssertContains(t, output, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) + } }) it("has expected contents", func() { - writeOp := build.WriteStackToml("/some/stack.toml", builder.StackMetadata{ + containerDir := "/some" + containerPath := "/some/stack.toml" + if isWindowsDaemon { + containerDir = `c:\some` + containerPath = `c:\some\stack.toml` + } + + writeOp := build.WriteStackToml(containerPath, builder.StackMetadata{ RunImage: builder.RunImageMetadata{ Image: "image-1", Mirrors: []string{ @@ -157,9 +249,16 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { }, }, }) + + ctrCmd := []string{"cat", "/some/stack.toml"} + if isWindowsDaemon { + ctrCmd = []string{"cmd", "/c", "type", `c:\some\stack.toml`} + } + ctx := context.Background() - ctr, err := createContainer(ctx, imageName, "cat", "/some/stack.toml") + ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -167,7 +266,7 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := outBuf.String() + output := strings.ReplaceAll(outBuf.String(), "\r", "") h.AssertContains(t, output, `[run-image] image = "image-1" mirrors = ["mirror-1", "mirror-2"] @@ -176,9 +275,28 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { }) } -func createContainer(ctx context.Context, imageName string, cmd ...string) (dcontainer.ContainerCreateCreatedBody, error) { +func createContainer(ctx context.Context, imageName string, containerDir string, cmd ...string) (dcontainer.ContainerCreateCreatedBody, error) { return ctrClient.ContainerCreate(ctx, &dcontainer.Config{Image: imageName, Cmd: cmd}, - &dcontainer.HostConfig{}, nil, "", + &dcontainer.HostConfig{ + Binds: []string{fmt.Sprintf("%s:%s", fmt.Sprintf("tests-volume-%s", h.RandString(5)), filepath.ToSlash(containerDir))}, + }, nil, "", ) } + +func cleanupContainer(ctx context.Context, ctrID string) { + inspect, err := ctrClient.ContainerInspect(ctx, ctrID) + if err != nil { + return + } + + // remove container + ctrClient.ContainerRemove(ctx, ctrID, types.ContainerRemoveOptions{}) + + // remove volumes + for _, m := range inspect.Mounts { + if m.Type == mount.TypeVolume { + ctrClient.VolumeRemove(ctx, m.Name, true) + } + } +} diff --git a/internal/build/testdata/fake-app/fake-app-symlink b/internal/build/testdata/fake-app/fake-app-symlink new file mode 120000 index 0000000000..89264cea27 --- /dev/null +++ b/internal/build/testdata/fake-app/fake-app-symlink @@ -0,0 +1 @@ +fake-app-file \ No newline at end of file From b2cdd902ccf0ea5d71368412a92d015b1c08d128 Mon Sep 17 00:00:00 2001 From: Micah Young Date: Tue, 8 Sep 2020 10:56:39 -0400 Subject: [PATCH 3/6] Fix test expectations for GHA Signed-off-by: Micah Young --- internal/build/container_ops_test.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/build/container_ops_test.go b/internal/build/container_ops_test.go index 719dbdd423..63c81e8336 100644 --- a/internal/build/container_ops_test.go +++ b/internal/build/container_ops_test.go @@ -105,15 +105,20 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { (.*) 0 ... file-to-ignore `) } else { - perms := "-rw-r--r--" if runtime.GOOS == "windows" { - perms = "-rwxrwxrwx" + // LCOW does not currently support symlinks + h.AssertContainsMatch(t, output, ` +-rwxrwxrwx 1 123 456 (.*) fake-app-file +-rwxrwxrwx 1 123 456 (.*) fake-app-symlink +-rwxrwxrwx 1 123 456 (.*) file-to-ignore +`) + } else { + h.AssertContainsMatch(t, output, ` +-rw-r--r-- 1 123 456 (.*) fake-app-file +lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file +-rw-r--r-- 1 123 456 (.*) file-to-ignore +`) } - h.AssertContainsMatch(t, output, fmt.Sprintf(` -%s 1 123 456 (.*) fake-app-file -%s 1 123 456 (.*) fake-app-symlink -> fake-app-file -%s 1 123 456 (.*) file-to-ignore -`, perms, "lrwxrwxrwx", perms)) } }) @@ -226,7 +231,7 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { output := strings.ReplaceAll(outBuf.String(), "\r", "") if isWindowsDaemon { - h.AssertContains(t, output, `12/31/1979 05:00 PM 69 ... stack.toml`) + h.AssertContains(t, output, `01/01/1980 12:00 AM 69 ... stack.toml`) } else { h.AssertContains(t, output, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) } From 21ba14b1c6ff33a19258a6aa69f4c773da6930a8 Mon Sep 17 00:00:00 2001 From: Micah Young Date: Thu, 10 Sep 2020 06:14:06 -0400 Subject: [PATCH 4/6] Change container.Run to work for Windows and Linux - Short-lived containers did not flush logs correctly - New implementation does ContainerAttach then ContainerStart like the [docker CLI run command])(https://github.com/docker/cli/blob/f8696535e9e516a5a404661697fef6f1228ada64/cli/command/container/run.go) reproducible outside tests - Use test helper for waiting on logs - Fix test polution from shared buffers Signed-off-by: Micah Young --- internal/build/container_ops_test.go | 67 +++++++++++++++++++--------- internal/container/run.go | 30 +++++-------- testhelpers/testhelpers.go | 27 ++++------- 3 files changed, 64 insertions(+), 60 deletions(-) diff --git a/internal/build/container_ops_test.go b/internal/build/container_ops_test.go index 63c81e8336..68b472e189 100644 --- a/internal/build/container_ops_test.go +++ b/internal/build/container_ops_test.go @@ -45,7 +45,6 @@ func TestContainerOperations(t *testing.T) { func testContainerOps(t *testing.T, when spec.G, it spec.S) { var ( imageName string - outBuf, errBuf bytes.Buffer isWindowsDaemon bool ) @@ -88,16 +87,15 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := strings.ReplaceAll(outBuf.String(), "\r", "") - if isWindowsDaemon { - h.AssertContainsMatch(t, output, ` + assertLogsContainMatch(t, &outBuf, &errBuf, ` (.*) ... . (.*) ... .. (.*) 17 ... fake-app-file @@ -107,13 +105,13 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { } else { if runtime.GOOS == "windows" { // LCOW does not currently support symlinks - h.AssertContainsMatch(t, output, ` + assertLogsContainMatch(t, &outBuf, &errBuf, ` -rwxrwxrwx 1 123 456 (.*) fake-app-file -rwxrwxrwx 1 123 456 (.*) fake-app-symlink -rwxrwxrwx 1 123 456 (.*) file-to-ignore `) } else { - h.AssertContainsMatch(t, output, ` + assertLogsContainMatch(t, &outBuf, &errBuf, ` -rw-r--r-- 1 123 456 (.*) fake-app-file lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file -rw-r--r-- 1 123 456 (.*) file-to-ignore @@ -142,15 +140,16 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := strings.ReplaceAll(outBuf.String(), "\r", "") + assertLogsContainMatch(t, &outBuf, &errBuf, "fake-app-file") - h.AssertNotContains(t, output, "file-to-ignore") + h.AssertNotContains(t, outBuf.String(), "file-to-ignore") }) it("writes contents from zip file", func() { @@ -171,22 +170,21 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := strings.ReplaceAll(outBuf.String(), "\r", "") - if isWindowsDaemon { - h.AssertContainsMatch(t, output, ` + assertLogsContainMatch(t, &outBuf, &errBuf, ` (.*) ... . (.*) ... .. (.*) 17 ... fake-app-file `) } else { - h.AssertContainsMatch(t, output, ` + assertLogsContainMatch(t, &outBuf, &errBuf, ` -rw-r--r-- 1 123 456 (.*) fake-app-file `) } @@ -222,18 +220,17 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + var outBuf, errBuf bytes.Buffer err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := strings.ReplaceAll(outBuf.String(), "\r", "") - if isWindowsDaemon { - h.AssertContains(t, output, `01/01/1980 12:00 AM 69 ... stack.toml`) + assertLogsContainMatch(t, &outBuf, &errBuf, `01/01/1980 12:00 AM 69 ... stack.toml`) } else { - h.AssertContains(t, output, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) + assertLogsContainMatch(t, &outBuf, &errBuf, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) } }) @@ -257,7 +254,7 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file ctrCmd := []string{"cat", "/some/stack.toml"} if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", "type", `c:\some\stack.toml`} + ctrCmd = []string{"cmd", "/c", `type c:\some\stack.toml`} } ctx := context.Background() @@ -265,26 +262,52 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + var outBuf, errBuf bytes.Buffer err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - output := strings.ReplaceAll(outBuf.String(), "\r", "") - h.AssertContains(t, output, `[run-image] + assertLogsContainMatch(t, &outBuf, &errBuf, `\[run-image\] image = "image-1" - mirrors = ["mirror-1", "mirror-2"] + mirrors = \["mirror-1", "mirror-2"\] `) }) }) } +func assertLogsContainMatch(t *testing.T, outBuf *bytes.Buffer, errBuf *bytes.Buffer, template string) { + t.Helper() + + h.AssertEq(t, errBuf.String(), "") + + output := strings.ReplaceAll(outBuf.String(), "\r", "") + + h.AssertContainsMatch(t, output, template) +} + func createContainer(ctx context.Context, imageName string, containerDir string, cmd ...string) (dcontainer.ContainerCreateCreatedBody, error) { + info, err := ctrClient.Info(ctx) + if err != nil { + return dcontainer.ContainerCreateCreatedBody{}, err + } + + isolationType := dcontainer.IsolationDefault + containerUser := "" // default + if info.OSType == "windows" { + isolationType = dcontainer.IsolationProcess + } + return ctrClient.ContainerCreate(ctx, - &dcontainer.Config{Image: imageName, Cmd: cmd}, + &dcontainer.Config{ + Image: imageName, + Cmd: cmd, + User: containerUser, + }, &dcontainer.HostConfig{ - Binds: []string{fmt.Sprintf("%s:%s", fmt.Sprintf("tests-volume-%s", h.RandString(5)), filepath.ToSlash(containerDir))}, + Binds: []string{fmt.Sprintf("%s:%s", fmt.Sprintf("tests-volume-%s", h.RandString(5)), filepath.ToSlash(containerDir))}, + Isolation: isolationType, }, nil, "", ) } diff --git a/internal/container/run.go b/internal/container/run.go index dd2f8b464d..7830189cae 100644 --- a/internal/container/run.go +++ b/internal/container/run.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "time" "github.com/docker/docker/api/types" dcontainer "github.com/docker/docker/api/types/container" @@ -16,32 +15,23 @@ import ( func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, errOut io.Writer) error { bodyChan, errChan := docker.ContainerWait(ctx, ctrID, dcontainer.WaitConditionNextExit) - if err := docker.ContainerStart(ctx, ctrID, types.ContainerStartOptions{}); err != nil { - return errors.Wrap(err, "container start") - } - - info, err := docker.Info(ctx) + logs, err := docker.ContainerAttach(ctx, ctrID, types.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + }) if err != nil { - return errors.Wrap(err, "getting docker info") - } - - if info.OSType == "windows" { - // wait for logs to show - time.Sleep(time.Second) + return err } - logs, err := docker.ContainerLogs(ctx, ctrID, types.ContainerLogsOptions{ - ShowStdout: true, - ShowStderr: true, - Follow: true, - }) - if err != nil { - return errors.Wrap(err, "container logs stdout") + if err := docker.ContainerStart(ctx, ctrID, types.ContainerStartOptions{}); err != nil { + return errors.Wrap(err, "container start") } copyErr := make(chan error) go func() { - _, err := stdcopy.StdCopy(out, errOut, logs) + _, err := stdcopy.StdCopy(out, errOut, logs.Reader) + copyErr <- err }() diff --git a/testhelpers/testhelpers.go b/testhelpers/testhelpers.go index 67c35ddde5..1ae8847f91 100644 --- a/testhelpers/testhelpers.go +++ b/testhelpers/testhelpers.go @@ -580,31 +580,22 @@ func SkipUnless(t *testing.T, expression bool, reason string) { func RunContainer(ctx context.Context, dockerCli client.CommonAPIClient, id string, stdout io.Writer, stderr io.Writer) error { bodyChan, errChan := dockerCli.ContainerWait(ctx, id, container.WaitConditionNextExit) - if err := dockerCli.ContainerStart(ctx, id, dockertypes.ContainerStartOptions{}); err != nil { - return errors.Wrap(err, "container start") - } - - info, err := dockerCli.Info(ctx) + logs, err := dockerCli.ContainerAttach(ctx, id, dockertypes.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + }) if err != nil { - return errors.Wrap(err, "getting docker info") - } - if info.OSType == "windows" { - // wait for logs to show - time.Sleep(time.Second) + return err } - logs, err := dockerCli.ContainerLogs(ctx, id, dockertypes.ContainerLogsOptions{ - ShowStdout: true, - ShowStderr: true, - Follow: true, - }) - if err != nil { - return errors.Wrap(err, "container logs stdout") + if err := dockerCli.ContainerStart(ctx, id, dockertypes.ContainerStartOptions{}); err != nil { + return errors.Wrap(err, "container start") } copyErr := make(chan error) go func() { - _, err := stdcopy.StdCopy(stdout, stderr, logs) + _, err := stdcopy.StdCopy(stdout, stderr, logs.Reader) copyErr <- err }() From 1c7bc2c883da064df2b56c3652bd7fedce71085a Mon Sep 17 00:00:00 2001 From: Micah Young Date: Tue, 15 Sep 2020 05:37:20 -0400 Subject: [PATCH 5/6] Fix volume permissions for Windows Windows permissions attempt to match imgutil's very limited permission model. Both implemtations do the following for setting user permissions: * UID/GID==0 => BUILTIN\Administrators, * otherwise BUILTIN\Users A more complex permission model will be introduced to the spec with a fix for [lifecycle#343](https://github.com/buildpacks/lifecycle/issues/343) but for now, this seemed like the best approach to accomodate any image config.User whose UID/GID cant be expressed as integers. Other details: - Only use xcopy for directories, which correctly sets permissions for copied files, particulary when workspace and layers are not volumes (also removes awkward pipe option syntax). - Change acceptance test apps and buildpacks to only use directory symlinks/junctions. Windows doesn't allow non-admin users to create symlinks, only junctions. Signed-off-by: Micah Young --- acceptance/testdata/mock_app/run | 2 +- acceptance/testdata/mock_app/run.bat | 2 +- .../0.2/read-env-buildpack/bin/build | 4 +- .../0.2/read-env-buildpack/bin/build.bat | 4 +- .../0.2/simple-layers-buildpack/bin/build | 6 +- .../0.2/simple-layers-buildpack/bin/build.bat | 12 +- .../mock_stack/windows/build/Dockerfile | 6 +- .../mock_stack/windows/run/Dockerfile | 6 +- internal/build/container_ops.go | 134 +++++++---- internal/build/container_ops_test.go | 221 ++++++++++-------- internal/build/lifecycle_execution.go | 24 +- internal/build/lifecycle_execution_test.go | 5 +- internal/build/phase_test.go | 17 +- internal/container/run.go | 6 +- internal/paths/paths.go | 32 +++ 15 files changed, 305 insertions(+), 176 deletions(-) diff --git a/acceptance/testdata/mock_app/run b/acceptance/testdata/mock_app/run index a1653205f5..93630c7ef4 100755 --- a/acceptance/testdata/mock_app/run +++ b/acceptance/testdata/mock_app/run @@ -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 diff --git a/acceptance/testdata/mock_app/run.bat b/acceptance/testdata/mock_app/run.bat index 1183af17ec..4fb4646cc2 100644 --- a/acceptance/testdata/mock_app/run.bat +++ b/acceptance/testdata/mock_app/run.bat @@ -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" diff --git a/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build b/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build index 24cb499670..fc20479ce9 100755 --- a/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build +++ b/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build @@ -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 echo "launch = true" > "$launch_dir/env1-launch-layer.toml" fi @@ -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 diff --git a/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build.bat b/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build.bat index bc0b4f0717..dd5e96d5cf 100644 --- a/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build.bat +++ b/acceptance/testdata/mock_buildpacks/0.2/read-env-buildpack/bin/build.bat @@ -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 ) @@ -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 ) diff --git a/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build b/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build index e2a7c1bb23..01568be0f2 100755 --- a/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build +++ b/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build @@ -16,7 +16,7 @@ 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 @@ -24,12 +24,12 @@ 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 diff --git a/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build.bat b/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build.bat index 8da9e3ec6c..2047dc714e 100644 --- a/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build.bat +++ b/acceptance/testdata/mock_buildpacks/0.2/simple-layers-buildpack/bin/build.bat @@ -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 diff --git a/acceptance/testdata/mock_stack/windows/build/Dockerfile b/acceptance/testdata/mock_stack/windows/build/Dockerfile index 0c0e063e23..37029d1293 100644 --- a/acceptance/testdata/mock_stack/windows/build/Dockerfile +++ b/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 USER ContainerAdministrator diff --git a/acceptance/testdata/mock_stack/windows/run/Dockerfile b/acceptance/testdata/mock_stack/windows/run/Dockerfile index 1e8bd9316a..76bcfbe6b7 100644 --- a/acceptance/testdata/mock_stack/windows/run/Dockerfile +++ b/acceptance/testdata/mock_stack/windows/run/Dockerfile @@ -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 diff --git a/internal/build/container_ops.go b/internal/build/container_ops.go index afc8245397..bcb9e03511 100644 --- a/internal/build/container_ops.go +++ b/internal/build/container_ops.go @@ -8,7 +8,6 @@ import ( "io/ioutil" "os" "runtime" - "strings" "github.com/BurntSushi/toml" "github.com/docker/docker/api/types" @@ -16,6 +15,8 @@ import ( "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" @@ -24,25 +25,22 @@ import ( 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 - } - if info.OSType == "windows" { - reader, err := createReader(src, winPathToTarPath(dst), uid, gid, fileFilter) - if err != nil { - return errors.Wrapf(err, "create tar archive from '%s'", src) - } - defer reader.Close() - return copyWindows(ctx, ctrClient, containerID, reader, dst, stdout, stderr) + tarPath := dst + if os == "windows" { + tarPath = paths.WindowsToPosixPath(dst) } - 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) } } @@ -69,25 +67,20 @@ func copyDir(ctx context.Context, ctrClient client.CommonAPIClient, containerID return err } -// copyWindows provides an alternate, Windows container-specific implementation of copyDir. +// copyDirWindows provides an alternate, Windows container-specific implementation of copyDir. // This implementation is needed because copying directly to a mounted volume is currently buggy // 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 copyWindows(ctx context.Context, ctrClient client.CommonAPIClient, containerID string, reader 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 } - fileOrDir := "d" - findDst := dst - if strings.HasSuffix(dst, ".toml") { - fileOrDir = "f" - pathElements := strings.Split(dst, `\`) - findDst = strings.Join(pathElements[:len(pathElements)-1], `\`) // parent of file - } - mnt, err := findMount(info, findDst) + baseName := paths.WindowsBasename(dst) + + mnt, err := findMount(info, dst) if err != nil { return err } @@ -105,7 +98,7 @@ func copyWindows(ctx context.Context, ctrClient client.CommonAPIClient, containe // b - copy symlinks, do not dereference // x - copy attributes // y - suppress prompting - fmt.Sprintf(`echo %s|xcopy /e /h /b /x /y c:\windows\%s %s`, fileOrDir, dst[3:], dst), + fmt.Sprintf(`xcopy c:\windows\%s %s /e /h /b /x /y`, baseName, dst), }, WorkingDir: "/", User: windowsContainerAdmin, @@ -145,7 +138,7 @@ func findMount(info types.ContainerJSON, dst string) (types.MountPoint, error) { } // 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) @@ -155,28 +148,24 @@ func WriteStackToml(dstPath string, stack builder.StackMetadata) ContainerOperat tarBuilder := archive.TarBuilder{} - info, err := ctrClient.Info(ctx) - if err != nil { - return err - } - if info.OSType == "windows" { - tarBuilder.AddFile(winPathToTarPath(dstPath), 0755, archive.NormalizedDateTime, buf.Bytes()) - reader := tarBuilder.Reader(archive.DefaultTarWriterFactory()) - defer reader.Close() - return copyWindows(ctx, ctrClient, containerID, reader, dstPath, stdout, stderr) + tarPath := dstPath + if os == "windows" { + tarPath = paths.WindowsToPosixPath(dstPath) } - tarBuilder.AddFile(dstPath, 0755, archive.NormalizedDateTime, buf.Bytes()) + tarBuilder.AddFile(tarPath, 0755, archive.NormalizedDateTime, buf.Bytes()) reader := tarBuilder.Reader(archive.DefaultTarWriterFactory()) defer reader.Close() + + if os == "windows" { + dirName := paths.WindowsDirname(dstPath) + return copyDirWindows(ctx, ctrClient, containerID, reader, dirName, stdout, stderr) + } + return ctrClient.CopyToContainer(ctx, containerID, "/", reader, types.CopyToContainerOptions{}) } } -func winPathToTarPath(path string) string { - return strings.ReplaceAll(path, `\`, "/")[2:] // strip volume, convert slashes -} - func createReader(src, dst string, uid, gid int, fileFilter func(string) bool) (io.ReadCloser, error) { fi, err := os.Stat(src) if err != nil { @@ -194,3 +183,68 @@ 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" { + 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)) + } + + 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, + ) + } +} diff --git a/internal/build/container_ops_test.go b/internal/build/container_ops_test.go index 68b472e189..b0fb38e282 100644 --- a/internal/build/container_ops_test.go +++ b/internal/build/container_ops_test.go @@ -44,18 +44,19 @@ func TestContainerOperations(t *testing.T) { func testContainerOps(t *testing.T, when spec.G, it spec.S) { var ( - imageName string - isWindowsDaemon bool + imageName string + osType string ) it.Before(func() { imageName = "container-ops.test-" + h.RandString(10) + info, err := ctrClient.Info(context.TODO()) h.AssertNil(t, err) - isWindowsDaemon = info.OSType == "windows" + osType = info.OSType dockerfileContent := `FROM busybox` - if isWindowsDaemon { + if osType == "windows" { dockerfileContent = `FROM mcr.microsoft.com/windows/nanoserver:1809` } @@ -70,23 +71,23 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { when("#CopyDir", func() { it("writes contents with proper owner/permissions", func() { - containerDir := "/some-location" - if isWindowsDaemon { - containerDir = `c:\some-location` + containerDir := "/some-vol" + if osType == "windows" { + containerDir = `c:\some-vol` } - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, nil) - ctx := context.Background() - - ctrCmd := []string{"ls", "-al", "/some-location"} - if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", `dir /q /s c:\some-location`} + ctrCmd := []string{"ls", "-al", "/some-vol"} + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `dir /q /s c:\some-vol`} } - ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) + ctx := context.Background() + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, osType, nil) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -94,8 +95,9 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - if isWindowsDaemon { - assertLogsContainMatch(t, &outBuf, &errBuf, ` + h.AssertEq(t, errBuf.String(), "") + if osType == "windows" { + h.AssertContainsMatch(t, strings.ReplaceAll(outBuf.String(), "\r", ""), ` (.*) ... . (.*) ... .. (.*) 17 ... fake-app-file @@ -105,13 +107,13 @@ func testContainerOps(t *testing.T, when spec.G, it spec.S) { } else { if runtime.GOOS == "windows" { // LCOW does not currently support symlinks - assertLogsContainMatch(t, &outBuf, &errBuf, ` + h.AssertContainsMatch(t, outBuf.String(), ` -rwxrwxrwx 1 123 456 (.*) fake-app-file -rwxrwxrwx 1 123 456 (.*) fake-app-symlink -rwxrwxrwx 1 123 456 (.*) file-to-ignore `) } else { - assertLogsContainMatch(t, &outBuf, &errBuf, ` + h.AssertContainsMatch(t, outBuf.String(), ` -rw-r--r-- 1 123 456 (.*) fake-app-file lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file -rw-r--r-- 1 123 456 (.*) file-to-ignore @@ -121,25 +123,25 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file }) it("writes contents ignoring from file filter", func() { - containerDir := "/some-location" - if isWindowsDaemon { - containerDir = `c:\some-location` + containerDir := "/some-vol" + if osType == "windows" { + containerDir = `c:\some-vol` } - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, func(filename string) bool { - return filepath.Base(filename) != "file-to-ignore" - }) - - ctrCmd := []string{"ls", "-al", "/some-location"} - if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-location`} + ctrCmd := []string{"ls", "-al", "/some-vol"} + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-vol`} } ctx := context.Background() - ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app"), containerDir, 123, 456, osType, func(filename string) bool { + return filepath.Base(filename) != "file-to-ignore" + }) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -147,29 +149,29 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - assertLogsContainMatch(t, &outBuf, &errBuf, "fake-app-file") - + h.AssertEq(t, errBuf.String(), "") + h.AssertContains(t, outBuf.String(), "fake-app-file") h.AssertNotContains(t, outBuf.String(), "file-to-ignore") }) it("writes contents from zip file", func() { - containerDir := "/some-location" - if isWindowsDaemon { - containerDir = `c:\some-location` + containerDir := "/some-vol" + if osType == "windows" { + containerDir = `c:\some-vol` } - copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app.zip"), containerDir, 123, 456, nil) - - ctrCmd := []string{"ls", "-al", "/some-location"} - if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-location`} + ctrCmd := []string{"ls", "-al", "/some-vol"} + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some-vol`} } ctx := context.Background() - ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) h.AssertNil(t, err) defer cleanupContainer(ctx, ctr.ID) + copyDirOp := build.CopyDir(filepath.Join("testdata", "fake-app.zip"), containerDir, 123, 456, osType, nil) + var outBuf, errBuf bytes.Buffer err = copyDirOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) @@ -177,14 +179,15 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - if isWindowsDaemon { - assertLogsContainMatch(t, &outBuf, &errBuf, ` + h.AssertEq(t, errBuf.String(), "") + if osType == "windows" { + h.AssertContainsMatch(t, strings.ReplaceAll(outBuf.String(), "\r", ""), ` (.*) ... . (.*) ... .. (.*) 17 ... fake-app-file `) } else { - assertLogsContainMatch(t, &outBuf, &errBuf, ` + h.AssertContainsMatch(t, outBuf.String(), ` -rw-r--r-- 1 123 456 (.*) fake-app-file `) } @@ -193,13 +196,22 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file when("#WriteStackToml", func() { it("writes file", func() { - containerDir := "/some" - containerPath := "/some/stack.toml" - if isWindowsDaemon { - containerDir = `c:\some` - containerPath = `c:\some\stack.toml` + containerDir := "/layers-vol" + containerPath := "/layers-vol/stack.toml" + if osType == "windows" { + containerDir = `c:\layers-vol` + containerPath = `c:\layers-vol\stack.toml` } + ctrCmd := []string{"ls", "-al", "/layers-vol/stack.toml"} + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `dir /q /n c:\layers-vol\stack.toml`} + } + ctx := context.Background() + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) + h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) + writeOp := build.WriteStackToml(containerPath, builder.StackMetadata{ RunImage: builder.RunImageMetadata{ Image: "image-1", @@ -208,17 +220,7 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file "mirror-2", }, }, - }) - - ctrCmd := []string{"ls", "-al", "/some/stack.toml"} - if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", `dir /q /s /n c:\some\stack.toml`} - } - - ctx := context.Background() - ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) - h.AssertNil(t, err) - defer cleanupContainer(ctx, ctr.ID) + }, osType) var outBuf, errBuf bytes.Buffer err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) @@ -227,21 +229,32 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - if isWindowsDaemon { - assertLogsContainMatch(t, &outBuf, &errBuf, `01/01/1980 12:00 AM 69 ... stack.toml`) + h.AssertEq(t, errBuf.String(), "") + if osType == "windows" { + h.AssertContains(t, outBuf.String(), `01/01/1980 12:00 AM 69 ... stack.toml`) } else { - assertLogsContainMatch(t, &outBuf, &errBuf, `-rwxr-xr-x 1 root root 69 Jan 1 1980 /some/stack.toml`) + h.AssertContains(t, outBuf.String(), `-rwxr-xr-x 1 root root 69 Jan 1 1980 /layers-vol/stack.toml`) } }) it("has expected contents", func() { - containerDir := "/some" - containerPath := "/some/stack.toml" - if isWindowsDaemon { - containerDir = `c:\some` - containerPath = `c:\some\stack.toml` + containerDir := "/layers-vol" + containerPath := "/layers-vol/stack.toml" + if osType == "windows" { + containerDir = `c:\layers-vol` + containerPath = `c:\layers-vol\stack.toml` + } + + ctrCmd := []string{"cat", "/layers-vol/stack.toml"} + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `type c:\layers-vol\stack.toml`} } + ctx := context.Background() + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) + h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) + writeOp := build.WriteStackToml(containerPath, builder.StackMetadata{ RunImage: builder.RunImageMetadata{ Image: "image-1", @@ -250,17 +263,7 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file "mirror-2", }, }, - }) - - ctrCmd := []string{"cat", "/some/stack.toml"} - if isWindowsDaemon { - ctrCmd = []string{"cmd", "/c", `type c:\some\stack.toml`} - } - - ctx := context.Background() - ctr, err := createContainer(ctx, imageName, containerDir, ctrCmd...) - h.AssertNil(t, err) - defer cleanupContainer(ctx, ctr.ID) + }, osType) var outBuf, errBuf bytes.Buffer err = writeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) @@ -269,33 +272,62 @@ lrwxrwxrwx 1 123 456 (.*) fake-app-symlink -> fake-app-file err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) h.AssertNil(t, err) - assertLogsContainMatch(t, &outBuf, &errBuf, `\[run-image\] + h.AssertEq(t, errBuf.String(), "") + h.AssertContains(t, outBuf.String(), `[run-image] image = "image-1" - mirrors = \["mirror-1", "mirror-2"\] + mirrors = ["mirror-1", "mirror-2"] `) }) }) -} -func assertLogsContainMatch(t *testing.T, outBuf *bytes.Buffer, errBuf *bytes.Buffer, template string) { - t.Helper() + when("#EnsureVolumeAccess", func() { + it("changes owner of volume", func() { + h.SkipIf(t, osType != "windows", "no-op for linux") - h.AssertEq(t, errBuf.String(), "") + ctx := context.Background() - output := strings.ReplaceAll(outBuf.String(), "\r", "") + ctrCmd := []string{"ls", "-al", "/my-volume"} + containerDir := "/my-volume" + if osType == "windows" { + ctrCmd = []string{"cmd", "/c", `icacls c:\my-volume`} + containerDir = `c:\my-volume` + } - h.AssertContainsMatch(t, output, template) -} + ctr, err := createContainer(ctx, imageName, containerDir, osType, ctrCmd...) + h.AssertNil(t, err) + defer cleanupContainer(ctx, ctr.ID) -func createContainer(ctx context.Context, imageName string, containerDir string, cmd ...string) (dcontainer.ContainerCreateCreatedBody, error) { - info, err := ctrClient.Info(ctx) - if err != nil { - return dcontainer.ContainerCreateCreatedBody{}, err - } + inspect, err := ctrClient.ContainerInspect(ctx, ctr.ID) + if err != nil { + return + } + + // use container's current volumes + var ctrVolumes []string + for _, m := range inspect.Mounts { + if m.Type == mount.TypeVolume { + ctrVolumes = append(ctrVolumes, m.Name) + } + } + + var outBuf, errBuf bytes.Buffer + + // reuse same volume twice to demonstrate multiple ops + initVolumeOp := build.EnsureVolumeAccess(123, 456, osType, ctrVolumes[0], ctrVolumes[0]) + err = initVolumeOp(ctrClient, ctx, ctr.ID, &outBuf, &errBuf) + h.AssertNil(t, err) + err = container.Run(ctx, ctrClient, ctr.ID, &outBuf, &errBuf) + h.AssertNil(t, err) + + h.AssertEq(t, errBuf.String(), "") + h.AssertContains(t, outBuf.String(), `BUILTIN\Users:(OI)(CI)(F)`) + }) + }) +} +func createContainer(ctx context.Context, imageName, containerDir, osType string, cmd ...string) (dcontainer.ContainerCreateCreatedBody, error) { isolationType := dcontainer.IsolationDefault - containerUser := "" // default - if info.OSType == "windows" { + if osType == "windows" { isolationType = dcontainer.IsolationProcess } @@ -303,7 +335,6 @@ func createContainer(ctx context.Context, imageName string, containerDir string, &dcontainer.Config{ Image: imageName, Cmd: cmd, - User: containerUser, }, &dcontainer.HostConfig{ Binds: []string{fmt.Sprintf("%s:%s", fmt.Sprintf("tests-volume-%s", h.RandString(5)), filepath.ToSlash(containerDir))}, diff --git a/internal/build/lifecycle_execution.go b/internal/build/lifecycle_execution.go index 0814d2d091..03fd87c9b6 100644 --- a/internal/build/lifecycle_execution.go +++ b/internal/build/lifecycle_execution.go @@ -42,6 +42,11 @@ func NewLifecycleExecution(logger logging.Logger, docker client.CommonAPIClient, return nil, err } + osType, err := opts.Builder.Image().OS() + if err != nil { + return nil, err + } + exec := &LifecycleExecution{ logger: logger, docker: docker, @@ -49,16 +54,10 @@ func NewLifecycleExecution(logger logging.Logger, docker client.CommonAPIClient, appVolume: paths.FilterReservedNames("pack-app-" + randString(10)), platformAPI: latestSupportedPlatformAPI, opts: opts, + os: osType, + mountPaths: mountPathsForOS(osType), } - os, err := opts.Builder.Image().OS() - if err != nil { - return nil, err - } - - exec.os = os - exec.mountPaths = mountPathsForOS(os) - return exec, nil } @@ -196,7 +195,7 @@ func (l *LifecycleExecution) Create( WithArgs(repoName), WithNetwork(networkMode), WithBinds(append(volumes, fmt.Sprintf("%s:%s", cacheName, l.mountPaths.cacheDir()))...), - WithContainerOperations(CopyDir(l.opts.AppPath, l.mountPaths.appDir(), l.opts.Builder.UID(), l.opts.Builder.GID(), l.opts.FileFilter)), + WithContainerOperations(CopyDir(l.opts.AppPath, l.mountPaths.appDir(), l.opts.Builder.UID(), l.opts.Builder.GID(), l.os, l.opts.FileFilter)), } if publish { @@ -232,7 +231,10 @@ func (l *LifecycleExecution) Detect(ctx context.Context, networkMode string, vol ), WithNetwork(networkMode), WithBinds(volumes...), - WithContainerOperations(CopyDir(l.opts.AppPath, l.mountPaths.appDir(), l.opts.Builder.UID(), l.opts.Builder.GID(), l.opts.FileFilter)), + WithContainerOperations( + EnsureVolumeAccess(l.opts.Builder.UID(), l.opts.Builder.GID(), l.os, l.layersVolume, l.appVolume), + CopyDir(l.opts.AppPath, l.mountPaths.appDir(), l.opts.Builder.UID(), l.opts.Builder.GID(), l.os, l.opts.FileFilter), + ), ) detect := phaseFactory.New(configProvider) @@ -389,7 +391,7 @@ func (l *LifecycleExecution) newExport(repoName, runImage string, publish bool, WithRoot(), WithNetwork(networkMode), WithBinds(fmt.Sprintf("%s:%s", cacheName, l.mountPaths.cacheDir())), - WithContainerOperations(WriteStackToml(l.mountPaths.stackPath(), l.opts.Builder.Stack())), + WithContainerOperations(WriteStackToml(l.mountPaths.stackPath(), l.opts.Builder.Stack(), l.os)), } if publish { diff --git a/internal/build/lifecycle_execution_test.go b/internal/build/lifecycle_execution_test.go index e8849c3b20..6a233cf83b 100644 --- a/internal/build/lifecycle_execution_test.go +++ b/internal/build/lifecycle_execution_test.go @@ -626,8 +626,9 @@ func testLifecycleExecution(t *testing.T, when spec.G, it spec.S) { configProvider := fakePhaseFactory.NewCalledWithProvider[lastCallIndex] h.AssertSliceContains(t, configProvider.HostConfig().Binds, expectedBind) - h.AssertEq(t, len(configProvider.ContainerOps()), 1) - h.AssertFunctionName(t, configProvider.ContainerOps()[0], "CopyDir") + h.AssertEq(t, len(configProvider.ContainerOps()), 2) + h.AssertFunctionName(t, configProvider.ContainerOps()[0], "EnsureVolumeAccess") + h.AssertFunctionName(t, configProvider.ContainerOps()[1], "CopyDir") }) }) diff --git a/internal/build/phase_test.go b/internal/build/phase_test.go index 03f613b40f..fbf5d24112 100644 --- a/internal/build/phase_test.go +++ b/internal/build/phase_test.go @@ -71,6 +71,7 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { outBuf, errBuf bytes.Buffer docker client.CommonAPIClient logger logging.Logger + osType string ) it.Before(func() { @@ -79,6 +80,11 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { var err error docker, err = client.NewClientWithOpts(client.FromEnv, client.WithVersion("1.38")) h.AssertNil(t, err) + + info, err := ctrClient.Info(context.Background()) + h.AssertNil(t, err) + osType = info.OSType + lifecycleExec, err = CreateFakeLifecycleExecution(logger, docker, filepath.Join("testdata", "fake-app"), repoName) h.AssertNil(t, err) phaseFactory = build.NewDefaultPhaseFactory(lifecycleExec) @@ -140,6 +146,7 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { "/workspace", lifecycleExec.Builder().UID(), lifecycleExec.Builder().GID(), + osType, nil, ), ), @@ -164,7 +171,7 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { when("app is a dir", func() { it("preserves original mod times", func() { - assertAppModTimePreserved(t, lifecycleExec, phaseFactory, &outBuf, &errBuf) + assertAppModTimePreserved(t, lifecycleExec, phaseFactory, &outBuf, &errBuf, osType) }) }) @@ -175,7 +182,7 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) phaseFactory = build.NewDefaultPhaseFactory(lifecycleExec) - assertAppModTimePreserved(t, lifecycleExec, phaseFactory, &outBuf, &errBuf) + assertAppModTimePreserved(t, lifecycleExec, phaseFactory, &outBuf, &errBuf, osType) }) }) @@ -215,7 +222,7 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { lifecycleExec, build.WithArgs("read", "/workspace/fake-app-file"), build.WithContainerOperations( - build.CopyDir(lifecycleExec.AppPath(), "/workspace", 0, 0, nil), + build.CopyDir(lifecycleExec.AppPath(), "/workspace", 0, 0, osType, nil), ), )) h.AssertNil(t, err) @@ -367,14 +374,14 @@ func testPhase(t *testing.T, when spec.G, it spec.S) { }) } -func assertAppModTimePreserved(t *testing.T, lifecycle *build.LifecycleExecution, phaseFactory build.PhaseFactory, outBuf *bytes.Buffer, errBuf *bytes.Buffer) { +func assertAppModTimePreserved(t *testing.T, lifecycle *build.LifecycleExecution, phaseFactory build.PhaseFactory, outBuf *bytes.Buffer, errBuf *bytes.Buffer, osType string) { t.Helper() readPhase := phaseFactory.New(build.NewPhaseConfigProvider( phaseName, lifecycle, build.WithArgs("read", "/workspace/fake-app-file"), build.WithContainerOperations( - build.CopyDir(lifecycle.AppPath(), "/workspace", 0, 0, nil), + build.CopyDir(lifecycle.AppPath(), "/workspace", 0, 0, osType, nil), ), )) assertRunSucceeds(t, readPhase, outBuf, errBuf) diff --git a/internal/container/run.go b/internal/container/run.go index 7830189cae..04bedc0652 100644 --- a/internal/container/run.go +++ b/internal/container/run.go @@ -15,7 +15,7 @@ import ( func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, errOut io.Writer) error { bodyChan, errChan := docker.ContainerWait(ctx, ctrID, dcontainer.WaitConditionNextExit) - logs, err := docker.ContainerAttach(ctx, ctrID, types.ContainerAttachOptions{ + resp, err := docker.ContainerAttach(ctx, ctrID, types.ContainerAttachOptions{ Stream: true, Stdout: true, Stderr: true, @@ -23,6 +23,7 @@ func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, if err != nil { return err } + defer resp.Close() if err := docker.ContainerStart(ctx, ctrID, types.ContainerStartOptions{}); err != nil { return errors.Wrap(err, "container start") @@ -30,7 +31,7 @@ func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, copyErr := make(chan error) go func() { - _, err := stdcopy.StdCopy(out, errOut, logs.Reader) + _, err := stdcopy.StdCopy(out, errOut, resp.Reader) copyErr <- err }() @@ -43,5 +44,6 @@ func Run(ctx context.Context, docker client.CommonAPIClient, ctrID string, out, case err := <-errChan: return err } + return <-copyErr } diff --git a/internal/paths/paths.go b/internal/paths/paths.go index 813e22b21c..cc7d959663 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -104,3 +104,35 @@ func FilterReservedNames(path string) string { return path } + +func WindowsDirname(path string) string { + pathElements := strings.Split(path, `\`) + if len(pathElements) < 1 { + return "" + } + + dirName := strings.Join(pathElements[:len(pathElements)-1], `\`) + + return dirName +} + +func WindowsBasename(path string) string { + pathElements := strings.Split(path, `\`) + if len(pathElements) < 1 { + return "" + } + + return pathElements[len(pathElements)-1] +} + +func WindowsToPosixPath(path string) string { + return strings.ReplaceAll(path, `\`, "/")[2:] // strip volume, convert slashes +} + +//WindowsPathSID returns the appropriate SID for a given UID and GID +func WindowsPathSID(uid, gid int) string { + if uid == 0 && gid == 0 { + return "S-1-5-32-544" // BUILTIN\Administrators + } + return "S-1-5-32-545" // BUILTIN\Users +} From e8a6b830fb6de50c203a112807276f90e6faa710 Mon Sep 17 00:00:00 2001 From: Micah Young Date: Thu, 17 Sep 2020 07:01:59 -0400 Subject: [PATCH 6/6] Clean up path functions - Add unit tests for Windows path functions Signed-off-by: Malini Valliath Signed-off-by: Micah Young --- internal/build/container_ops.go | 7 ++-- internal/paths/paths.go | 54 +++++++++++++++++-------------- internal/paths/paths_test.go | 57 +++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 28 deletions(-) diff --git a/internal/build/container_ops.go b/internal/build/container_ops.go index bcb9e03511..8d1b43002e 100644 --- a/internal/build/container_ops.go +++ b/internal/build/container_ops.go @@ -29,7 +29,7 @@ func CopyDir(src, dst string, uid, gid int, os string, fileFilter func(string) b return func(ctrClient client.CommonAPIClient, ctx context.Context, containerID string, stdout, stderr io.Writer) error { tarPath := dst if os == "windows" { - tarPath = paths.WindowsToPosixPath(dst) + tarPath = paths.WindowsToSlash(dst) } reader, err := createReader(src, tarPath, uid, gid, fileFilter) @@ -150,7 +150,7 @@ func WriteStackToml(dstPath string, stack builder.StackMetadata, os string) Cont tarPath := dstPath if os == "windows" { - tarPath = paths.WindowsToPosixPath(dstPath) + tarPath = paths.WindowsToSlash(dstPath) } tarBuilder.AddFile(tarPath, 0755, archive.NormalizedDateTime, buf.Bytes()) @@ -158,7 +158,7 @@ func WriteStackToml(dstPath string, stack builder.StackMetadata, os string) Cont defer reader.Close() if os == "windows" { - dirName := paths.WindowsDirname(dstPath) + dirName := paths.WindowsDir(dstPath) return copyDirWindows(ctx, ctrClient, containerID, reader, dirName, stdout, stderr) } @@ -217,7 +217,6 @@ func EnsureVolumeAccess(uid, gid int, os string, volumeNames ...string) Containe // /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)) } diff --git a/internal/paths/paths.go b/internal/paths/paths.go index cc7d959663..083de50a8c 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -15,8 +15,8 @@ func IsURI(ref string) bool { return schemeRegexp.MatchString(ref) } -func IsDir(path string) (bool, error) { - fileInfo, err := os.Stat(path) +func IsDir(p string) (bool, error) { + fileInfo, err := os.Stat(p) if err != nil { return false, err } @@ -24,22 +24,22 @@ func IsDir(path string) (bool, error) { return fileInfo.IsDir(), nil } -func FilePathToURI(path string) (string, error) { +func FilePathToURI(p string) (string, error) { var err error - if !filepath.IsAbs(path) { - path, err = filepath.Abs(path) + if !filepath.IsAbs(p) { + p, err = filepath.Abs(p) if err != nil { return "", err } } if runtime.GOOS == "windows" { - if strings.HasPrefix(path, `\\`) { - return "file://" + filepath.ToSlash(strings.TrimPrefix(path, `\\`)), nil + if strings.HasPrefix(p, `\\`) { + return "file://" + filepath.ToSlash(strings.TrimPrefix(p, `\\`)), nil } - return "file:///" + filepath.ToSlash(path), nil + return "file:///" + filepath.ToSlash(p), nil } - return "file://" + path, nil + return "file://" + p, nil } // examples: @@ -87,7 +87,7 @@ func ToAbsolute(uri, relativeTo string) (string, error) { return uri, nil } -func FilterReservedNames(path string) string { +func FilterReservedNames(p string) string { // The following keys are reserved on Windows // https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#win32-file-namespaces reservedNameConversions := map[string]string{ @@ -99,37 +99,43 @@ func FilterReservedNames(path string) string { "prn": "p_r_n", } for k, v := range reservedNameConversions { - path = strings.Replace(path, k, v, -1) + p = strings.Replace(p, k, v, -1) } - return path + return p } -func WindowsDirname(path string) string { - pathElements := strings.Split(path, `\`) - if len(pathElements) < 1 { - return "" - } +//WindowsDir is equivalent to path.Dir or filepath.Dir but always for Windows paths +//reproduced because Windows implementation is not exported +func WindowsDir(p string) string { + pathElements := strings.Split(p, `\`) dirName := strings.Join(pathElements[:len(pathElements)-1], `\`) return dirName } -func WindowsBasename(path string) string { - pathElements := strings.Split(path, `\`) - if len(pathElements) < 1 { - return "" - } +//WindowsBasename is equivalent to path.Basename or filepath.Basename but always for Windows paths +//reproduced because Windows implementation is not exported +func WindowsBasename(p string) string { + pathElements := strings.Split(p, `\`) return pathElements[len(pathElements)-1] } -func WindowsToPosixPath(path string) string { - return strings.ReplaceAll(path, `\`, "/")[2:] // strip volume, convert slashes +//WindowsToSlash is equivalent to path.ToSlash or filepath.ToSlash but always for Windows paths +//reproduced because Windows implementation is not exported +func WindowsToSlash(p string) string { + slashPath := strings.ReplaceAll(p, `\`, "/") // convert slashes + if len(slashPath) < 2 { + return "" + } + + return slashPath[2:] // strip volume } //WindowsPathSID returns the appropriate SID for a given UID and GID +//This the basic logic for path permissions in Pack and Lifecycle func WindowsPathSID(uid, gid int) string { if uid == 0 && gid == 0 { return "S-1-5-32-544" // BUILTIN\Administrators diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index e9e240f599..a96c970577 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -154,4 +154,61 @@ func testPaths(t *testing.T, when spec.G, it spec.S) { }) }) }) + + when("#WindowsDir", func() { + it("returns the path directory", func() { + path := WindowsDir(`C:\layers\file.txt`) + h.AssertEq(t, path, `C:\layers`) + }) + + it("returns empty for empty", func() { + path := WindowsBasename("") + h.AssertEq(t, path, "") + }) + }) + + when("#WindowsBasename", func() { + it("returns the path basename", func() { + path := WindowsBasename(`C:\layers\file.txt`) + h.AssertEq(t, path, `file.txt`) + }) + + it("returns empty for empty", func() { + path := WindowsBasename("") + h.AssertEq(t, path, "") + }) + }) + + when("#WindowsToSlash", func() { + it("returns the path; backward slashes converted to forward with volume stripped ", func() { + path := WindowsToSlash(`C:\layers\file.txt`) + h.AssertEq(t, path, `/layers/file.txt`) + }) + + it("returns / for volume", func() { + path := WindowsToSlash(`c:\`) + h.AssertEq(t, path, `/`) + }) + + it("returns empty for empty", func() { + path := WindowsToSlash("") + h.AssertEq(t, path, "") + }) + }) + + when("#WindowsPathSID", func() { + when("UID and GID are both 0", func() { + it(`returns the built-in BUILTIN\Administrators SID`, func() { + sid := WindowsPathSID(0, 0) + h.AssertEq(t, sid, "S-1-5-32-544") + }) + }) + + when("UID and GID are both non-zero", func() { + it(`returns the built-in BUILTIN\Users SID`, func() { + sid := WindowsPathSID(99, 99) + h.AssertEq(t, sid, "S-1-5-32-545") + }) + }) + }) }