Merge pull request #22181 from Microsoft/jjh/workdir

Windows: Consistent build workdir handling

builder/dockerfile/dispatchers.go

@@ -264,12 +264,37 @@ func workdir(b *Builder, args []string, attributes map[string]bool, original str
 	// This is from the Dockerfile and will not necessarily be in platform
 	// specific semantics, hence ensure it is converted.
 	workdir := filepath.FromSlash(args[0])
-
+	current := filepath.FromSlash(b.runConfig.WorkingDir)
-	if !system.IsAbs(workdir) {
+	if runtime.GOOS == "windows" {
-		current := filepath.FromSlash(b.runConfig.WorkingDir)
+		// Windows is a little more complicated than Linux. This code ensures
-		workdir = filepath.Join(string(os.PathSeparator), current, workdir)
+		// we end up with a workdir which is consistent in terms of platform
+		// semantics. This means C:\somefolder, specifically in the format:
+		// UPPERCASEDriveLetter-Colon-Backslash-FolderName. We are already
+		// guaranteed that `current`, if set, is consistent. This allows us to
+		// cope correctly with any of the following in a Dockerfile:
+		//	WORKDIR a                       --> C:\a
+		//	WORKDIR c:\\foo                 --> C:\foo
+		//	WORKDIR \\foo                   --> C:\foo
+		//	WORKDIR /foo                    --> C:\foo
+		//	WORKDIR c:\\foo \ WORKDIR bar   --> C:\foo --> C:\foo\bar
+		//	WORKDIR C:/foo \ WORKDIR bar    --> C:\foo --> C:\foo\bar
+		//	WORKDIR C:/foo \ WORKDIR \\bar  --> C:\foo --> C:\bar
+		//	WORKDIR /foo \ WORKDIR c:/bar   --> C:\foo --> C:\bar
+		if len(current) == 0 || system.IsAbs(workdir) {
+			if (workdir[0] == os.PathSeparator) ||
+				(len(workdir) > 1 && string(workdir[1]) != ":") ||
+				(len(workdir) == 1) {
+				workdir = filepath.Join(`C:\`, workdir)
+			}
+		} else {
+			workdir = filepath.Join(current, workdir)
+		}
+		workdir = strings.ToUpper(string(workdir[0])) + workdir[1:] // Upper-case drive letter
+	} else {
+		if !filepath.IsAbs(workdir) {
+			workdir = filepath.Join(string(os.PathSeparator), current, workdir)
+		}
 	}
-
 	b.runConfig.WorkingDir = workdir
 
 	return b.commit("", b.runConfig.Cmd, fmt.Sprintf("WORKDIR %v", workdir))

builder/dockerfile/internals.go

@@ -200,13 +200,59 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 
 	// Twiddle the destination when its a relative path - meaning, make it
 	// relative to the WORKINGDIR
-	if !system.IsAbs(dest) {
-		hasSlash := strings.HasSuffix(dest, string(os.PathSeparator))
-		dest = filepath.Join(string(os.PathSeparator), filepath.FromSlash(b.runConfig.WorkingDir), dest)
 
-		// Make sure we preserve any trailing slash
+	endsInSlash := strings.HasSuffix(dest, string(os.PathSeparator))
-		if hasSlash {
+
-			dest += string(os.PathSeparator)
+	if runtime.GOOS == "windows" {
+		// On Windows, this is more complicated. We are guaranteed that the
+		// WorkingDir is already platform consistent meaning in the format
+		// UPPERCASEDriveLetter-Colon-Backslash-Foldername. However, Windows
+		// for now also has the limitation that ADD/COPY can only be done to
+		// the C: (system) drive, not any drives that might be present as a
+		// result of bind mounts.
+		//
+		// So... if the path specified is Linux-style absolute (/foo or \\foo),
+		// we assume it is the system drive. If it is a Windows-style absolute
+		// (DRIVE:\\foo), error if DRIVE is not C. And finally, ensure we
+		// strip any configured working directories drive letter so that it
+		// can be subsequently legitimately converted to a Windows volume-style
+		// pathname.
+
+		// Not a typo - filepath.IsAbs, not system.IsAbs on this next check as
+		// we only want to validate where the DriveColon part has been supplied.
+		if filepath.IsAbs(dest) {
+			if strings.ToUpper(string(dest[0])) != "C" {
+				return fmt.Errorf("Windows does not support %s with a destinations not on the system drive (C:)", cmdName)
+			}
+			dest = dest[2:] // Strip the drive letter
+		}
+
+		// Cannot handle relative where WorkingDir is not the system drive.
+		if len(b.runConfig.WorkingDir) > 0 {
+			if !system.IsAbs(b.runConfig.WorkingDir[2:]) {
+				return fmt.Errorf("Current WorkingDir %s is not platform consistent", b.runConfig.WorkingDir)
+			}
+			if !system.IsAbs(dest) {
+				if string(b.runConfig.WorkingDir[0]) != "C" {
+					return fmt.Errorf("Windows does not support %s with relative paths when WORKDIR is not the system drive", cmdName)
+				}
+
+				dest = filepath.Join(string(os.PathSeparator), b.runConfig.WorkingDir[2:], dest)
+
+				// Make sure we preserve any trailing slash
+				if endsInSlash {
+					dest += string(os.PathSeparator)
+				}
+			}
+		}
+	} else {
+		if !system.IsAbs(dest) {
+			dest = filepath.Join(string(os.PathSeparator), filepath.FromSlash(b.runConfig.WorkingDir), dest)
+
+			// Make sure we preserve any trailing slash
+			if endsInSlash {
+				dest += string(os.PathSeparator)
+			}
 		}
 	}
 

container/container.go

@@ -251,6 +251,13 @@ func (container *Container) GetResourcePath(path string) (string, error) {
 
 	cleanPath := cleanResourcePath(path)
 	r, e := symlink.FollowSymlinkInScope(filepath.Join(container.BaseFS, cleanPath), container.BaseFS)
+
+	// Log this here on the daemon side as there's otherwise no indication apart
+	// from the error being propagated all the way back to the client. This makes
+	// debugging significantly easier and clearly indicates the error comes from the daemon.
+	if e != nil {
+		logrus.Errorf("Failed to FollowSymlinkInScope BaseFS %s cleanPath %s path %s %s\n", container.BaseFS, cleanPath, path, e)
+	}
 	return r, e
 }
 

integration-cli/docker_cli_build_test.go

@@ -2074,22 +2074,13 @@ func (s *DockerSuite) TestBuildRelativeWorkdir(c *check.C) {
 		expected4     string
 		expectedFinal string
 	)
-	// TODO Windows: The expectedFinal needs fixing to match Windows
+
-	// filepath semantics. However, this is a non-trivial change. @jhowardmsft
-	// Short story - need to make the configuration file platform semantically
-	// consistent, so even if `WORKDIR /test2/test3` is specified in a Dockerfile,
-	// the configuration should be stored as C:\test2\test3. Something similar to
-	// 	if runtime.GOOS == "windows" && len(workdir) > 2 && string(workdir[0]) == `\` {
-	//		workdir = "C:" + workdir
-	//  }
-	// in builder\dockerfile\dispatchers.go, function workdir(), but also
-	// ironing out all other cases where this causes other failures.
 	if daemonPlatform == "windows" {
 		expected1 = `C:/`
 		expected2 = `C:/test1`
 		expected3 = `C:/test2`
 		expected4 = `C:/test2/test3`
-		expectedFinal = `\test2\test3`
+		expectedFinal = `C:\test2\test3` // Note inspect is going to return Windows paths, as it's not in busybox
 	} else {
 		expected1 = `/`
 		expected2 = `/test1`
@@ -2117,12 +2108,238 @@ func (s *DockerSuite) TestBuildRelativeWorkdir(c *check.C) {
 	}
 }
 
+// #22181 Regression test. Validates combinations of supported
+// WORKDIR dockerfile directives in Windows and non-Windows semantics.
+func (s *DockerSuite) TestBuildWindowsWorkdirProcessing(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowsworkdirprocessing"
+	_, err := buildImage(name,
+		`FROM busybox
+		WORKDIR a
+		RUN sh -c "[ "$PWD" = "C:/a" ]"
+		WORKDIR c:\\foo
+		RUN sh -c "[ "$PWD" = "C:/foo" ]"
+		WORKDIR \\foo
+		RUN sh -c "[ "$PWD" = "C:/foo" ]"
+		WORKDIR /foo
+		RUN sh -c "[ "$PWD" = "C:/foo" ]"
+		WORKDIR C:/foo
+		WORKDIR bar
+		RUN sh -c "[ "$PWD" = "C:/foo/bar" ]"
+		WORKDIR c:/foo
+		WORKDIR bar
+		RUN sh -c "[ "$PWD" = "C:/foo/bar" ]"
+		WORKDIR c:/foo
+		WORKDIR \\bar
+		RUN sh -c "[ "$PWD" = "C:/bar" ]"
+		WORKDIR /foo
+		WORKDIR c:\\bar
+		RUN sh -c "[ "$PWD" = "C:/bar" ]"
+		`,
+		true)
+	if err != nil {
+		c.Fatal(err)
+	}
+}
+
+// #22181 Regression test. Validates combinations of supported
+// COPY dockerfile directives in Windows and non-Windows semantics.
+func (s *DockerSuite) TestBuildWindowsAddCopyPathProcessing(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowsaddcopypathprocessing"
+	// TODO Windows (@jhowardmsft). Needs a follow-up PR to 22181 to
+	// support backslash such as .\\ being equivalent to ./ and c:\\ being
+	// equivalent to c:/. This is not currently (nor ever has been) supported
+	// by docker on the Windows platform.
+	dockerfile := `
+		FROM busybox
+			# First cases with no workdir, all end up in the root directory of the system drive
+			COPY a1 ./
+			ADD  a2 ./
+			RUN sh -c "[ $(cat c:/a1) = 'helloa1' ]"
+			RUN sh -c "[ $(cat c:/a2) = 'worlda2' ]"
+
+			COPY b1 /
+			ADD  b2 /
+			RUN sh -c "[ $(cat c:/b1) = 'hellob1' ]"
+			RUN sh -c "[ $(cat c:/b2) = 'worldb2' ]"
+
+			COPY c1 c:/
+			ADD  c2 c:/
+			RUN sh -c "[ $(cat c:/c1) = 'helloc1' ]"
+			RUN sh -c "[ $(cat c:/c2) = 'worldc2' ]"
+
+			COPY d1 c:/
+			ADD  d2 c:/
+			RUN sh -c "[ $(cat c:/d1) = 'hellod1' ]"
+			RUN sh -c "[ $(cat c:/d2) = 'worldd2' ]"
+
+			COPY e1 .
+			ADD  e2 .
+			RUN sh -c "[ $(cat c:/e1) = 'helloe1' ]"
+			RUN sh -c "[ $(cat c:/e2) = 'worlde2' ]"
+			
+			# Now with a workdir
+			WORKDIR c:\\wa12
+			COPY wa1 ./
+			ADD wa2 ./
+			RUN sh -c "[ $(cat c:/wa12/wa1) = 'hellowa1' ]"
+			RUN sh -c "[ $(cat c:/wa12/wa2) = 'worldwa2' ]"
+
+			# No trailing slash on COPY/ADD, Linux-style path. 
+			# Results in dir being changed to a file
+			WORKDIR /wb1
+			COPY wb1 .
+			WORKDIR /wb2
+			ADD wb2 .
+			WORKDIR c:/
+			RUN sh -c "[ $(cat c:/wb1) = 'hellowb1' ]"
+			RUN sh -c "[ $(cat c:/wb2) = 'worldwb2' ]"
+
+			# No trailing slash on COPY/ADD, Windows-style path. 
+			# Results in dir being changed to a file
+			WORKDIR /wc1
+			COPY wc1 c:/wc1
+			WORKDIR /wc2
+			ADD wc2 c:/wc2
+			WORKDIR c:/
+			RUN sh -c "[ $(cat c:/wc1) = 'hellowc1' ]"
+			RUN sh -c "[ $(cat c:/wc2) = 'worldwc2' ]"			
+
+			# Trailing slash on COPY/ADD, Windows-style path. 
+			WORKDIR /wd1
+			COPY wd1 c:/wd1/
+			WORKDIR /wd2
+			ADD wd2 c:/wd2/
+			RUN sh -c "[ $(cat c:/wd1/wd1) = 'hellowd1' ]"
+			RUN sh -c "[ $(cat c:/wd2/wd2) = 'worldwd2' ]"
+			`
+	ctx, err := fakeContext(dockerfile, map[string]string{
+		"a1":  "helloa1",
+		"a2":  "worlda2",
+		"b1":  "hellob1",
+		"b2":  "worldb2",
+		"c1":  "helloc1",
+		"c2":  "worldc2",
+		"d1":  "hellod1",
+		"d2":  "worldd2",
+		"e1":  "helloe1",
+		"e2":  "worlde2",
+		"wa1": "hellowa1",
+		"wa2": "worldwa2",
+		"wb1": "hellowb1",
+		"wb2": "worldwb2",
+		"wc1": "hellowc1",
+		"wc2": "worldwc2",
+		"wd1": "hellowd1",
+		"wd2": "worldwd2",
+	})
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer ctx.Close()
+	_, err = buildImageFromContext(name, ctx, false)
+	if err != nil {
+		c.Fatal(err)
+	}
+}
+
+// #22181 Regression test.
+func (s *DockerSuite) TestBuildWindowsCopyFailsNonSystemDrive(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowscopyfailsnonsystemdrive"
+	dockerfile := `
+		FROM busybox
+		cOpY foo d:/
+		`
+	ctx, err := fakeContext(dockerfile, map[string]string{"foo": "hello"})
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer ctx.Close()
+	_, err = buildImageFromContext(name, ctx, false)
+	if err == nil {
+		c.Fatal(err)
+	}
+	if !strings.Contains(err.Error(), "Windows does not support COPY with a destinations not on the system drive (C:)") {
+		c.Fatal(err)
+	}
+}
+
+// #22181 Regression test.
+func (s *DockerSuite) TestBuildWindowsCopyFailsWorkdirNonSystemDrive(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowscopyfailsworkdirsystemdrive"
+	dockerfile := `
+		FROM busybox
+		WORKDIR d:/
+		cOpY foo .
+		`
+	ctx, err := fakeContext(dockerfile, map[string]string{"foo": "hello"})
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer ctx.Close()
+	_, err = buildImageFromContext(name, ctx, false)
+	if err == nil {
+		c.Fatal(err)
+	}
+	if !strings.Contains(err.Error(), "Windows does not support COPY with relative paths when WORKDIR is not the system drive") {
+		c.Fatal(err)
+	}
+}
+
+// #22181 Regression test.
+func (s *DockerSuite) TestBuildWindowsAddFailsNonSystemDrive(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowsaddfailsnonsystemdrive"
+	dockerfile := `
+		FROM busybox
+		AdD foo d:/
+		`
+	ctx, err := fakeContext(dockerfile, map[string]string{"foo": "hello"})
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer ctx.Close()
+	_, err = buildImageFromContext(name, ctx, false)
+	if err == nil {
+		c.Fatal(err)
+	}
+	if !strings.Contains(err.Error(), "Windows does not support ADD with a destinations not on the system drive (C:)") {
+		c.Fatal(err)
+	}
+}
+
+// #22181 Regression test.
+func (s *DockerSuite) TestBuildWindowsAddFailsWorkdirNonSystemDrive(c *check.C) {
+	testRequires(c, DaemonIsWindows)
+	name := "testbuildwindowsaddfailsworkdirsystemdrive"
+	dockerfile := `
+		FROM busybox
+		WORKDIR d:/
+		AdD foo .
+		`
+	ctx, err := fakeContext(dockerfile, map[string]string{"foo": "hello"})
+	if err != nil {
+		c.Fatal(err)
+	}
+	defer ctx.Close()
+	_, err = buildImageFromContext(name, ctx, false)
+	if err == nil {
+		c.Fatal(err)
+	}
+	if !strings.Contains(err.Error(), "Windows does not support ADD with relative paths when WORKDIR is not the system drive") {
+		c.Fatal(err)
+	}
+}
+
 func (s *DockerSuite) TestBuildWorkdirWithEnvVariables(c *check.C) {
 	name := "testbuildworkdirwithenvvariables"
 
 	var expected string
 	if daemonPlatform == "windows" {
-		expected = `\test1\test2`
+		expected = `C:\test1\test2`
 	} else {
 		expected = `/test1/test2`
 	}