Enable usage of lifecycle image for windows #823

ameyer-pivotal · 2020-08-27T19:47:49Z

Signed-off-by: Victoria Henry vhenry@pivotal.io
Signed-off-by: Andrew Meyer meyeran@vmware.com
Signed-off-by: Malini Valliath mvalliath@vmware.com

Summary

Enables use of lifecycle images for Windows builds

codecov · 2020-08-27T20:39:58Z

Codecov Report

Merging #823 into main will decrease coverage by 0.56%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
- Coverage   76.19%   75.64%   -0.55%     
==========================================
  Files          81       81              
  Lines        4141     4187      +46     
==========================================
+ Hits         3155     3167      +12     
- Misses        655      685      +30     
- Partials      331      335       +4

Flag	Coverage Δ
#os_linux	`75.60% <ø> (-0.59%)`	⬇️
#os_macos	`72.04% <ø> (-0.53%)`	⬇️
#os_windows	`100.00% <ø> (?)`
#unit	`75.64% <ø> (-0.55%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

ameyer-pivotal · 2020-08-27T21:14:48Z

@jromero Also note that after this is released, we should update the Windows build guide to remove --trust-builder from the example

dfreilich · 2020-08-31T13:18:05Z

acceptance/acceptance_test.go

@@ -615,8 +615,6 @@ func testAcceptance(
 					var untrustedBuilderName string

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


dfreilich · 2020-08-31T13:19:33Z

build_test.go

@@ -1345,18 +1345,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() {


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

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.

internal/build/container_ops.go

jromero · 2020-09-02T22:26:37Z

internal/container/run.go

+	if info.OSType == "windows" {
+		// wait for logs to show
+		time.Sleep(time.Second)
+	}


Based on our slack conversation can we create an issue to keep track of this work and add a reference to the code so we can track this workaround.

Suggested change

if info.OSType == "windows" {

// wait for logs to show

time.Sleep(time.Second)

}

// FIXME: <issue number>

if info.OSType == "windows" {

// wait for logs to show

time.Sleep(time.Second)

}

No time.Sleep is needed with the subsequent changes to container.Run below.

micahyoung

Everything is ready for review again. I added some notes inline.

The big changes were to:

Add a new ContainerOperation to Detect which, only for Windows, spins up an ephemeral container with the app and layers volumes and fixes their permissions, prior to detection. This fixes an issue where docker on Windows Server 2019 (also our GHA Runner OS) had divergent behavior from Docker Desktop for Windows 10.

# Docker for Windows Server 2019
docker run --rm -v my-vol:c:/my-vol mcr.microsoft.com/windows/nanoserver:1809 cmd /c "echo foo > c:\my-vol\foo.txt"
Access is denied.

# Docker Desktop for Windows 10
docker run --rm -v my-vol:c:/my-vol mcr.microsoft.com/windows/nanoserver:1809 cmd /c "echo foo > c:\my-vol\foo.txt"
# sucess

Refactor container.Run to fix a race condition for all versions of Windows, by making capturing process output with ContainerAttach instead of ContainerLogs. It now follows the pattern in docker/cli run command. See inline comments for details.

micahyoung · 2020-09-16T14:58:08Z

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


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).

micahyoung · 2020-09-16T15:07:46Z

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


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.

micahyoung · 2020-09-16T15:12:08Z

acceptance/testdata/mock_stack/windows/build/Dockerfile

-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



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

micahyoung · 2020-09-16T15:17:15Z

internal/build/container_ops.go

+//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" {


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

micahyoung · 2020-09-16T15:18:30Z

internal/build/container_ops.go

+				cmd += "&&"
+			}
+
+			cmd += fmt.Sprintf(`icacls %s /grant *%s:(OI)(CI)F /t /l /q`, containerPath, paths.WindowsPathSID(uid, gid))


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

micahyoung · 2020-09-16T15:23:07Z

internal/build/lifecycle_execution.go

@@ -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)),


Pass in the OS instead of repeatedly querying from the daemon

micahyoung · 2020-09-17T11:34:40Z

internal/build/container_ops.go

+				// 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),


Added better documentation for xcopy command

micahyoung · 2020-09-17T11:44:05Z

internal/paths/paths.go

@@ -15,31 +15,31 @@ func IsURI(ref string) bool {
 	return schemeRegexp.MatchString(ref)
 }

-func IsDir(path string) (bool, error) {
-	fileInfo, err := os.Stat(path)


avoid clobbering golang path package

micahyoung · 2020-09-17T11:50:02Z

testhelpers/testhelpers.go

+	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
 	}()


Use ContainerAttach + ContainerStart instead of ContainerStart + ContainerLogs. The current implementation has race conditions for Windows and is more heavy-weight for all OSes as the stdout/stderr have to flow through the logs subsystem. This implementation follows the docker/cli run implementation which effectively does the same (though with additional signal handling and tty options).

This fixes Windows, potentially avoids race conditions for both platforms by attaching before start, and I feel like having the Attach handle gives us more option for signalling in the future.

This also removes the need for the time.Sleep in the original PR attempt

jromero · 2020-09-17T17:55:16Z

This overall looks great! Thank you for the added effort. There seems to be a small conflict. Once that is resolved we can happily bring this in.

Signed-off-by: Andrew Meyer <meyeran@vmware.com> Signed-off-by: Malini Valliath <mvalliath@vmware.com> Signed-off-by: Victoria Henry <vhenry@pivotal.io> Signed-off-by: Anthony Emengo <aemengo@vmware.com>

Signed-off-by: Micah Young <ymicah@vmware.com>

- 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 <ymicah@vmware.com>

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](buildpacks/lifecycle#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 <ymicah@vmware.com>

- Add unit tests for Windows path functions Signed-off-by: Malini Valliath <mvalliath@pivotal.io> Signed-off-by: Micah Young <ymicah@vmware.com>

micahyoung · 2020-09-17T20:39:43Z

@jromero should be RTM/review now, I think.

ameyer-pivotal requested a review from a team as a code owner August 27, 2020 19:47

ameyer-pivotal mentioned this pull request Aug 27, 2020

Add Windows build guide buildpacks/docs#201

Merged

dfreilich reviewed Aug 31, 2020

View reviewed changes

micahyoung reviewed Sep 2, 2020

View reviewed changes

internal/build/container_ops.go Outdated Show resolved Hide resolved

jromero suggested changes Sep 2, 2020

View reviewed changes

micahyoung reviewed Sep 17, 2020

View reviewed changes

TisVictress and others added 6 commits September 17, 2020 16:24

Enable usage of lifecycle image for windows

5b2130c

Signed-off-by: Andrew Meyer <meyeran@vmware.com> Signed-off-by: Malini Valliath <mvalliath@vmware.com> Signed-off-by: Victoria Henry <vhenry@pivotal.io> Signed-off-by: Anthony Emengo <aemengo@vmware.com>

Add test coverage for windows container ops

6ef83d2

Signed-off-by: Micah Young <ymicah@vmware.com>

Fix test expectations for GHA

b2cdd90

Signed-off-by: Micah Young <ymicah@vmware.com>

Clean up path functions

e8a6b83

- Add unit tests for Windows path functions Signed-off-by: Malini Valliath <mvalliath@pivotal.io> Signed-off-by: Micah Young <ymicah@vmware.com>

jromero approved these changes Sep 17, 2020

View reviewed changes

jromero merged commit 1d20341 into buildpacks:main Sep 17, 2020

jromero added os/windows type/enhancement Issue that requests a new feature or improvement. labels Sep 17, 2020

jromero added this to the 0.14.0 milestone Sep 17, 2020

micahyoung deleted the feature/use-windows-lifecycle branch September 18, 2020 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable usage of lifecycle image for windows #823

Enable usage of lifecycle image for windows #823

ameyer-pivotal commented Aug 27, 2020

codecov bot commented Aug 27, 2020 •

edited

ameyer-pivotal commented Aug 27, 2020

dfreilich Aug 31, 2020

dfreilich Aug 31, 2020

ameyer-pivotal Sep 2, 2020

jromero Sep 2, 2020

micahyoung Sep 17, 2020

micahyoung left a comment •

edited

micahyoung Sep 16, 2020

micahyoung Sep 16, 2020

micahyoung Sep 16, 2020

micahyoung Sep 16, 2020

micahyoung Sep 16, 2020

micahyoung Sep 16, 2020

micahyoung Sep 17, 2020

micahyoung Sep 17, 2020

micahyoung Sep 17, 2020 •

edited

jromero commented Sep 17, 2020

micahyoung commented Sep 17, 2020

Enable usage of lifecycle image for windows #823

Enable usage of lifecycle image for windows #823

Conversation

ameyer-pivotal commented Aug 27, 2020

Summary

codecov bot commented Aug 27, 2020 • edited

Codecov Report

ameyer-pivotal commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahyoung left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahyoung Sep 17, 2020 • edited

Choose a reason for hiding this comment

jromero commented Sep 17, 2020

micahyoung commented Sep 17, 2020

codecov bot commented Aug 27, 2020 •

edited

micahyoung left a comment •

edited

micahyoung Sep 17, 2020 •

edited