Merge pull request #37982 from Microsoft/jjh/archive

pkg/archive fixes, and port most unit tests to Windows

pkg/archive/archive_test.go

@@ -305,7 +305,7 @@ func TestUntarPathWithInvalidSrc(t *testing.T) {
 }
 
 func TestUntarPath(t *testing.T) {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	tmpFolder, err := ioutil.TempDir("", "docker-archive-test")
 	assert.NilError(t, err)
 	defer os.RemoveAll(tmpFolder)
@@ -436,7 +436,7 @@ func TestCopyWithTarInvalidSrc(t *testing.T) {
 }
 
 func TestCopyWithTarInexistentDestWillCreateIt(t *testing.T) {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	tempFolder, err := ioutil.TempDir("", "docker-archive-test")
 	if err != nil {
 		t.Fatal(nil)
@@ -608,10 +608,6 @@ func TestCopyFileWithTarSrcFile(t *testing.T) {
 }
 
 func TestTarFiles(t *testing.T) {
-	// TODO Windows: Figure out how to port this test.
-	if runtime.GOOS == "windows" {
-		t.Skip("Failing on Windows")
-	}
 	// try without hardlinks
 	if err := checkNoChanges(1000, false); err != nil {
 		t.Fatal(err)
@@ -690,10 +686,6 @@ func tarUntar(t *testing.T, origin string, options *TarOptions) ([]Change, error
 }
 
 func TestTarUntar(t *testing.T) {
-	// TODO Windows: Figure out how to fix this test.
-	if runtime.GOOS == "windows" {
-		t.Skip("Failing on Windows")
-	}
 	origin, err := ioutil.TempDir("", "docker-test-untar-origin")
 	if err != nil {
 		t.Fatal(err)
@@ -722,7 +714,7 @@ func TestTarUntar(t *testing.T) {
 			t.Fatalf("Error tar/untar for compression %s: %s", c.Extension(), err)
 		}
 
-		if len(changes) != 1 || changes[0].Path != "/3" {
+		if len(changes) != 1 || changes[0].Path != string(filepath.Separator)+"3" {
 			t.Fatalf("Unexpected differences after tarUntar: %v", changes)
 		}
 	}
@@ -780,10 +772,6 @@ func TestTarWithOptionsChownOptsAlwaysOverridesIdPair(t *testing.T) {
 }
 
 func TestTarWithOptions(t *testing.T) {
-	// TODO Windows: Figure out how to fix this test.
-	if runtime.GOOS == "windows" {
-		t.Skip("Failing on Windows")
-	}
 	origin, err := ioutil.TempDir("", "docker-test-untar-origin")
 	if err != nil {
 		t.Fatal(err)
@@ -942,10 +930,6 @@ func BenchmarkTarUntarWithLinks(b *testing.B) {
 }
 
 func TestUntarInvalidFilenames(t *testing.T) {
-	// TODO Windows: Figure out how to fix this test.
-	if runtime.GOOS == "windows" {
-		t.Skip("Passes but hits breakoutError: platform and architecture is not supported")
-	}
 	for i, headers := range [][]*tar.Header{
 		{
 			{
@@ -970,9 +954,7 @@ func TestUntarInvalidFilenames(t *testing.T) {
 }
 
 func TestUntarHardlinkToSymlink(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	skip.If(t, runtime.GOOS == "windows", "hardlinks on Windows")
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	for i, headers := range [][]*tar.Header{
 		{
 			{
@@ -1001,10 +983,6 @@ func TestUntarHardlinkToSymlink(t *testing.T) {
 }
 
 func TestUntarInvalidHardlink(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	if runtime.GOOS == "windows" {
-		t.Skip("hardlinks on Windows")
-	}
 	for i, headers := range [][]*tar.Header{
 		{ // try reading victim/hello (../)
 			{
@@ -1085,10 +1063,6 @@ func TestUntarInvalidHardlink(t *testing.T) {
 }
 
 func TestUntarInvalidSymlink(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	if runtime.GOOS == "windows" {
-		t.Skip("hardlinks on Windows")
-	}
 	for i, headers := range [][]*tar.Header{
 		{ // try reading victim/hello (../)
 			{
@@ -1254,7 +1228,7 @@ func TestReplaceFileTarWrapper(t *testing.T) {
 // TestPrefixHeaderReadable tests that files that could be created with the
 // version of this package that was built with <=go17 are still readable.
 func TestPrefixHeaderReadable(t *testing.T) {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	// https://gist.github.com/stevvooe/e2a790ad4e97425896206c0816e1a882#file-out-go
 	var testFile = []byte("\x1f\x8b\x08\x08\x44\x21\x68\x59\x00\x03\x74\x2e\x74\x61\x72\x00\x4b\xcb\xcf\x67\xa0\x35\x30\x80\x00\x86\x06\x10\x47\x01\xc1\x37\x40\x00\x54\xb6\xb1\xa1\xa9\x99\x09\x48\x25\x1d\x40\x69\x71\x49\x62\x91\x02\xe5\x76\xa1\x79\x84\x21\x91\xd6\x80\x72\xaf\x8f\x82\x51\x30\x0a\x46\x36\x00\x00\xf0\x1c\x1e\x95\x00\x06\x00\x00")
 
@@ -1312,7 +1286,7 @@ func appendModifier(path string, header *tar.Header, content io.Reader) (*tar.He
 }
 
 func readFileFromArchive(t *testing.T, archive io.ReadCloser, name string, expectedCount int, doc string) string {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	destDir, err := ioutil.TempDir("", "docker-test-destDir")
 	assert.NilError(t, err)
 	defer os.RemoveAll(destDir)

pkg/archive/changes.go

@@ -63,12 +63,16 @@ func (c changesByPath) Less(i, j int) bool { return c[i].Path < c[j].Path }
 func (c changesByPath) Len() int           { return len(c) }
 func (c changesByPath) Swap(i, j int)      { c[j], c[i] = c[i], c[j] }
 
-// Gnu tar and the go tar writer don't have sub-second mtime
-// precision, which is problematic when we apply changes via tar
-// files, we handle this by comparing for exact times, *or* same
+// Gnu tar doesn't have sub-second mtime precision. The go tar
+// writer (1.10+) does when using PAX format, but we round times to seconds
+// to ensure archives have the same hashes for backwards compatibility.
+// See https://github.com/moby/moby/pull/35739/commits/fb170206ba12752214630b269a40ac7be6115ed4.
+//
+// Non-sub-second is problematic when we apply changes via tar
+// files. We handle this by comparing for exact times, *or* same
 // second count and either a or b having exactly 0 nanoseconds
 func sameFsTime(a, b time.Time) bool {
-	return a == b ||
+	return a.Equal(b) ||
 		(a.Unix() == b.Unix() &&
 			(a.Nanosecond() == 0 || b.Nanosecond() == 0))
 }

pkg/archive/changes_test.go

@@ -5,8 +5,10 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"path/filepath"
 	"runtime"
 	"sort"
+	"syscall"
 	"testing"
 	"time"
 
@@ -23,7 +25,24 @@ func max(x, y int) int {
 }
 
 func copyDir(src, dst string) error {
-	return exec.Command("cp", "-a", src, dst).Run()
+	if runtime.GOOS != "windows" {
+		return exec.Command("cp", "-a", src, dst).Run()
+	}
+
+	// Could have used xcopy src dst /E /I /H /Y /B. However, xcopy has the
+	// unfortunate side effect of not preserving timestamps of newly created
+	// directories in the target directory, so we don't get accurate changes.
+	// Use robocopy instead. Note this isn't available in microsoft/nanoserver.
+	// But it has gotchas. See https://weblogs.sqlteam.com/robv/archive/2010/02/17/61106.aspx
+	err := exec.Command("robocopy", filepath.FromSlash(src), filepath.FromSlash(dst), "/SL", "/COPYALL", "/MIR").Run()
+	if exiterr, ok := err.(*exec.ExitError); ok {
+		if status, ok := exiterr.Sys().(syscall.WaitStatus); ok {
+			if status.ExitStatus()&24 == 0 {
+				return nil
+			}
+		}
+	}
+	return err
 }
 
 type FileType uint32
@@ -113,11 +132,6 @@ func TestChangeString(t *testing.T) {
 }
 
 func TestChangesWithNoChanges(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
-	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
-	}
 	rwLayer, err := ioutil.TempDir("", "docker-changes-test")
 	assert.NilError(t, err)
 	defer os.RemoveAll(rwLayer)
@@ -133,11 +147,6 @@ func TestChangesWithNoChanges(t *testing.T) {
 }
 
 func TestChangesWithChanges(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
-	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
-	}
 	// Mock the readonly layer
 	layer, err := ioutil.TempDir("", "docker-changes-test-layer")
 	assert.NilError(t, err)
@@ -167,21 +176,20 @@ func TestChangesWithChanges(t *testing.T) {
 	assert.NilError(t, err)
 
 	expectedChanges := []Change{
-		{"/dir1", ChangeModify},
-		{"/dir1/file1-1", ChangeModify},
-		{"/dir1/file1-2", ChangeDelete},
-		{"/dir1/subfolder", ChangeModify},
-		{"/dir1/subfolder/newFile", ChangeAdd},
+		{filepath.FromSlash("/dir1"), ChangeModify},
+		{filepath.FromSlash("/dir1/file1-1"), ChangeModify},
+		{filepath.FromSlash("/dir1/file1-2"), ChangeDelete},
+		{filepath.FromSlash("/dir1/subfolder"), ChangeModify},
+		{filepath.FromSlash("/dir1/subfolder/newFile"), ChangeAdd},
 	}
 	checkChanges(expectedChanges, changes, t)
 }
 
 // See https://github.com/docker/docker/pull/13590
 func TestChangesWithChangesGH13590(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
+	// TODO Windows. Needs further investigation to identify the failure
 	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
+		t.Skip("needs more investigation")
 	}
 	baseLayer, err := ioutil.TempDir("", "docker-changes-test.")
 	assert.NilError(t, err)
@@ -238,11 +246,6 @@ func TestChangesWithChangesGH13590(t *testing.T) {
 
 // Create a directory, copy it, make sure we report no changes between the two
 func TestChangesDirsEmpty(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
-	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
-	}
 	src, err := ioutil.TempDir("", "docker-changes-test")
 	assert.NilError(t, err)
 	defer os.RemoveAll(src)
@@ -325,11 +328,6 @@ func mutateSampleDir(t *testing.T, root string) {
 }
 
 func TestChangesDirsMutated(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
-	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
-	}
 	src, err := ioutil.TempDir("", "docker-changes-test")
 	assert.NilError(t, err)
 	createSampleDir(t, src)
@@ -347,20 +345,37 @@ func TestChangesDirsMutated(t *testing.T) {
 	sort.Sort(changesByPath(changes))
 
 	expectedChanges := []Change{
-		{"/dir1", ChangeDelete},
-		{"/dir2", ChangeModify},
-		{"/dirnew", ChangeAdd},
-		{"/file1", ChangeDelete},
-		{"/file2", ChangeModify},
-		{"/file3", ChangeModify},
-		{"/file4", ChangeModify},
-		{"/file5", ChangeModify},
-		{"/filenew", ChangeAdd},
-		{"/symlink1", ChangeDelete},
-		{"/symlink2", ChangeModify},
-		{"/symlinknew", ChangeAdd},
+		{filepath.FromSlash("/dir1"), ChangeDelete},
+		{filepath.FromSlash("/dir2"), ChangeModify},
 	}
 
+	// Note there is slight difference between the Linux and Windows
+	// implementations here. Due to https://github.com/moby/moby/issues/9874,
+	// and the fix at https://github.com/moby/moby/pull/11422, Linux does not
+	// consider a change to the directory time as a change. Windows on NTFS
+	// does. See https://github.com/moby/moby/pull/37982 for more information.
+	//
+	// Note also: https://github.com/moby/moby/pull/37982#discussion_r223523114
+	// that differences are ordered in the way the test is currently written, hence
+	// this is in the middle of the list of changes rather than at the start or
+	// end. Potentially can be addressed later.
+	if runtime.GOOS == "windows" {
+		expectedChanges = append(expectedChanges, Change{filepath.FromSlash("/dir3"), ChangeModify})
+	}
+
+	expectedChanges = append(expectedChanges, []Change{
+		{filepath.FromSlash("/dirnew"), ChangeAdd},
+		{filepath.FromSlash("/file1"), ChangeDelete},
+		{filepath.FromSlash("/file2"), ChangeModify},
+		{filepath.FromSlash("/file3"), ChangeModify},
+		{filepath.FromSlash("/file4"), ChangeModify},
+		{filepath.FromSlash("/file5"), ChangeModify},
+		{filepath.FromSlash("/filenew"), ChangeAdd},
+		{filepath.FromSlash("/symlink1"), ChangeDelete},
+		{filepath.FromSlash("/symlink2"), ChangeModify},
+		{filepath.FromSlash("/symlinknew"), ChangeAdd},
+	}...)
+
 	for i := 0; i < max(len(changes), len(expectedChanges)); i++ {
 		if i >= len(expectedChanges) {
 			t.Fatalf("unexpected change %s\n", changes[i].String())
@@ -373,7 +388,7 @@ func TestChangesDirsMutated(t *testing.T) {
 				t.Fatalf("Wrong change for %s, expected %s, got %s\n", changes[i].Path, changes[i].String(), expectedChanges[i].String())
 			}
 		} else if changes[i].Path < expectedChanges[i].Path {
-			t.Fatalf("unexpected change %s\n", changes[i].String())
+			t.Fatalf("unexpected change %q %q\n", changes[i].String(), expectedChanges[i].Path)
 		} else {
 			t.Fatalf("no change for expected change %s != %s\n", expectedChanges[i].String(), changes[i].String())
 		}
@@ -381,10 +396,13 @@ func TestChangesDirsMutated(t *testing.T) {
 }
 
 func TestApplyLayer(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
+	// TODO Windows. This is very close to working, but it fails with changes
+	// to \symlinknew and \symlink2. The destination has an updated
+	// Access/Modify/Change/Birth date to the source (~3/100th sec different).
+	// Needs further investigation as to why, but I currently believe this is
+	// just the way NTFS works. I don't think it's a bug in this test or archive.
 	if runtime.GOOS == "windows" {
-		t.Skip("symlinks on Windows")
+		t.Skip("needs further investigation")
 	}
 	src, err := ioutil.TempDir("", "docker-changes-test")
 	assert.NilError(t, err)
@@ -417,10 +435,10 @@ func TestApplyLayer(t *testing.T) {
 }
 
 func TestChangesSizeWithHardlinks(t *testing.T) {
-	// TODO Windows. There may be a way of running this, but turning off for now
-	// as createSampleDir uses symlinks.
+	// TODO Windows. Needs further investigation. Likely in ChangeSizes not
+	// coping correctly with hardlinks on Windows.
 	if runtime.GOOS == "windows" {
-		t.Skip("hardlinks on Windows")
+		t.Skip("needs further investigation")
 	}
 	srcDir, err := ioutil.TempDir("", "docker-test-srcDir")
 	assert.NilError(t, err)
@@ -481,7 +499,7 @@ func TestChangesSize(t *testing.T) {
 }
 
 func checkChanges(expectedChanges, changes []Change, t *testing.T) {
-	skip.If(t, os.Getuid() != 0, "skipping test that requires root")
+	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	sort.Sort(changesByPath(expectedChanges))
 	sort.Sort(changesByPath(changes))
 	for i := 0; i < max(len(changes), len(expectedChanges)); i++ {

pkg/archive/changes_unix.go

@@ -16,7 +16,13 @@ func statDifferent(oldStat *system.StatT, newStat *system.StatT) bool {
 		oldStat.UID() != newStat.UID() ||
 		oldStat.GID() != newStat.GID() ||
 		oldStat.Rdev() != newStat.Rdev() ||
-		// Don't look at size for dirs, its not a good measure of change
+		// Don't look at size or modification time for dirs, its not a good
+		// measure of change. See https://github.com/moby/moby/issues/9874
+		// for a description of the issue with modification time, and
+		// https://github.com/moby/moby/pull/11422 for the change.
+		// (Note that in the Windows implementation of this function,
+		// modification time IS taken as a change). See
+		// https://github.com/moby/moby/pull/37982 for more information.
 		(oldStat.Mode()&unix.S_IFDIR != unix.S_IFDIR &&
 			(!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) || (oldStat.Size() != newStat.Size()))) {
 		return true

pkg/archive/changes_windows.go

@@ -7,9 +7,13 @@ import (
 )
 
 func statDifferent(oldStat *system.StatT, newStat *system.StatT) bool {
+	// Note there is slight difference between the Linux and Windows
+	// implementations here. Due to https://github.com/moby/moby/issues/9874,
+	// and the fix at https://github.com/moby/moby/pull/11422, Linux does not
+	// consider a change to the directory time as a change. Windows on NTFS
+	// does. See https://github.com/moby/moby/pull/37982 for more information.
 
-	// Don't look at size for dirs, its not a good measure of change
-	if oldStat.Mtim() != newStat.Mtim() ||
+	if !sameFsTime(oldStat.Mtim(), newStat.Mtim()) ||
 		oldStat.Mode() != newStat.Mode() ||
 		oldStat.Size() != newStat.Size() && !oldStat.Mode().IsDir() {
 		return true

pkg/archive/diff.go

@@ -240,11 +240,13 @@ func applyLayerHandler(dest string, layer io.Reader, options *TarOptions, decomp
 	dest = filepath.Clean(dest)
 
 	// We need to be able to set any perms
-	oldmask, err := system.Umask(0)
-	if err != nil {
-		return 0, err
+	if runtime.GOOS != "windows" {
+		oldmask, err := system.Umask(0)
+		if err != nil {
+			return 0, err
+		}
+		defer system.Umask(oldmask)
 	}
-	defer system.Umask(oldmask) // ignore err, ErrNotSupportedPlatform
 
 	if decompress {
 		decompLayer, err := DecompressStream(layer)

pkg/archive/diff_test.go

@@ -7,17 +7,12 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
-	"runtime"
 	"testing"
 
 	"github.com/docker/docker/pkg/ioutils"
 )
 
 func TestApplyLayerInvalidFilenames(t *testing.T) {
-	// TODO Windows: Figure out how to fix this test.
-	if runtime.GOOS == "windows" {
-		t.Skip("Passes but hits breakoutError: platform and architecture is not supported")
-	}
 	for i, headers := range [][]*tar.Header{
 		{
 			{
@@ -42,9 +37,6 @@ func TestApplyLayerInvalidFilenames(t *testing.T) {
 }
 
 func TestApplyLayerInvalidHardlink(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.Skip("TypeLink support on Windows")
-	}
 	for i, headers := range [][]*tar.Header{
 		{ // try reading victim/hello (../)
 			{
@@ -125,9 +117,6 @@ func TestApplyLayerInvalidHardlink(t *testing.T) {
 }
 
 func TestApplyLayerInvalidSymlink(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.Skip("TypeSymLink support on Windows")
-	}
 	for i, headers := range [][]*tar.Header{
 		{ // try reading victim/hello (../)
 			{
@@ -208,11 +197,6 @@ func TestApplyLayerInvalidSymlink(t *testing.T) {
 }
 
 func TestApplyLayerWhiteouts(t *testing.T) {
-	// TODO Windows: Figure out why this test fails
-	if runtime.GOOS == "windows" {
-		t.Skip("Failing on Windows")
-	}
-
 	wd, err := ioutil.TempDir("", "graphdriver-test-whiteouts")
 	if err != nil {
 		return
@@ -339,7 +323,9 @@ func makeTestLayer(paths []string) (rc io.ReadCloser, err error) {
 		}
 	}()
 	for _, p := range paths {
-		if p[len(p)-1] == filepath.Separator {
+		// Source files are always in Unix format. But we use filepath on
+		// creation to be platform agnostic.
+		if p[len(p)-1] == '/' {
 			if err = os.MkdirAll(filepath.Join(tmpDir, p), 0700); err != nil {
 				return
 			}
@@ -374,9 +360,10 @@ func readDirContents(root string) ([]string, error) {
 			return err
 		}
 		if info.IsDir() {
-			rel = rel + "/"
+			rel = rel + string(filepath.Separator)
 		}
-		files = append(files, rel)
+		// Append in Unix semantics
+		files = append(files, filepath.ToSlash(rel))
 		return nil
 	})
 	if err != nil {