Merge pull request #2688 from nalind/moar-dockerignore-1.16

[release-1.16] ADD and COPY: descend into excluded directories, sometimes

add.go

@@ -137,6 +137,29 @@ func getURL(src, mountpoint, renameTarget string, writer io.Writer) error {
 	return errors.Wrapf(err, "error writing content from %q to tar stream", src)
 }
 
+// includeDirectoryAnyway returns true if "path" is a prefix for an exception
+// known to "pm".  If "path" is a directory that "pm" claims matches its list
+// of patterns, but "pm"'s list of exclusions contains a pattern for which
+// "path" is a prefix, then IncludeDirectoryAnyway() will return true.
+// This is not always correct, because it relies on the directory part of any
+// exception paths to be specified without wildcards.
+func includeDirectoryAnyway(path string, pm *fileutils.PatternMatcher) bool {
+	if !pm.Exclusions() {
+		return false
+	}
+	prefix := strings.TrimPrefix(path, string(os.PathSeparator)) + string(os.PathSeparator)
+	for _, pattern := range pm.Patterns() {
+		if !pattern.Exclusion() {
+			continue
+		}
+		spec := strings.TrimPrefix(pattern.String(), string(os.PathSeparator))
+		if strings.HasPrefix(spec, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 // Add copies the contents of the specified sources into the container's root
 // filesystem, optionally extracting contents of local files that look like
 // non-empty archives.
@@ -363,20 +386,32 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 		for _, glob := range localSourceStat.Globbed {
 			rel, err := filepath.Rel(contextDir, glob)
 			if err != nil {
-				return errors.Wrapf(err, "error computing path of %q", glob)
+				return errors.Wrapf(err, "error computing path of %q relative to %q", glob, contextDir)
 			}
 			if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
 				return errors.Errorf("possible escaping context directory error: %q is outside of %q", glob, contextDir)
 			}
 			// Check for dockerignore-style exclusion of this item.
 			if rel != "." {
-				matches, err := pm.Matches(filepath.ToSlash(rel)) // nolint:staticcheck
+				excluded, err := pm.Matches(filepath.ToSlash(rel)) // nolint:staticcheck
 				if err != nil {
 					return errors.Wrapf(err, "error checking if %q(%q) is excluded", glob, rel)
 				}
-				if matches {
-					continue
+				if excluded {
+					// non-directories that are excluded are excluded, no question, but
+					// directories can only be skipped if we don't have to allow for the
+					// possibility of finding things to include under them
+					globInfo := localSourceStat.Results[glob]
+					if !globInfo.IsDir || !includeDirectoryAnyway(rel, pm) {
+						continue
+					}
 				}
+			} else {
+				// Make sure we don't trigger a "copied nothing" error for an empty context
+				// directory if we were told to copy the context directory itself.  We won't
+				// actually copy it, but we need to make sure that we don't produce an error
+				// due to potentially not having anything in the tarstream that we passed.
+				itemsCopied++
 			}
 			st := localSourceStat.Results[glob]
 			pipeReader, pipeWriter := io.Pipe()
@@ -391,6 +426,10 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 						return false, false, nil
 					})
 				}
+				writer = newTarFilterer(writer, func(hdr *tar.Header) (bool, bool, io.Reader) {
+					itemsCopied++
+					return false, false, nil
+				})
 				getOptions := copier.GetOptions{
 					UIDMap:         srcUIDMap,
 					GIDMap:         srcGIDMap,
@@ -462,10 +501,9 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 				}
 				return multiErr.Errors[0]
 			}
-			itemsCopied++
 		}
 		if itemsCopied == 0 {
-			return errors.Wrapf(syscall.ENOENT, "no items matching glob %q copied (%d filtered)", localSourceStat.Glob, len(localSourceStat.Globbed))
+			return errors.Wrapf(syscall.ENOENT, "no items matching glob %q copied (%d filtered out)", localSourceStat.Glob, len(localSourceStat.Globbed))
 		}
 	}
 	return nil

copier/copier.go

@@ -976,20 +976,7 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa
 			return errorResponse("copier: get: glob %q: %v", glob, err)
 		}
 		globMatchedCount += len(globMatched)
-		filtered := make([]string, 0, len(globMatched))
-		for _, globbed := range globMatched {
-			rel, excluded, err := pathIsExcluded(req.Root, globbed, pm)
-			if err != nil {
-				return errorResponse("copier: get: checking if %q is excluded: %v", globbed, err)
-			}
-			if rel == "." || !excluded {
-				filtered = append(filtered, globbed)
-			}
-		}
-		if len(filtered) == 0 {
-			return errorResponse("copier: get: glob %q matched nothing (%d filtered out of %v): %v", glob, len(globMatched), globMatched, syscall.ENOENT)
-		}
-		queue = append(queue, filtered...)
+		queue = append(queue, globMatched...)
 	}
 	// no matches -> error
 	if len(queue) == 0 {
@@ -1042,16 +1029,16 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa
 				options := req.GetOptions
 				options.ExpandArchives = false
 				walkfn := func(path string, info os.FileInfo, err error) error {
+					if err != nil {
+						return errors.Wrapf(err, "copier: get: error reading %q", path)
+					}
 					// compute the path of this item
 					// relative to the top-level directory,
 					// for the tar header
 					rel, relErr := convertToRelSubdirectory(item, path)
 					if relErr != nil {
 						return errors.Wrapf(relErr, "copier: get: error computing path of %q relative to top directory %q", path, item)
 					}
-					if err != nil {
-						return errors.Wrapf(err, "copier: get: error reading %q", path)
-					}
 					// prefix the original item's name if we're keeping it
 					if relNamePrefix != "" {
 						rel = filepath.Join(relNamePrefix, rel)
@@ -1108,7 +1095,7 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa
 			}
 		}
 		if itemsCopied == 0 {
-			return errors.New("copier: get: copied no items")
+			return errors.Wrapf(syscall.ENOENT, "copier: get: copied no items")
 		}
 		return nil
 	}

copier/copier_test.go

@@ -741,6 +741,7 @@ func testGetMultiple(t *testing.T) {
 					items: []string{
 						"file-0",
 						"file-b",
+						"file-c",
 						"file-n",
 						"file-o",
 						"file-p",
@@ -815,6 +816,7 @@ func testGetMultiple(t *testing.T) {
 					pattern: "*",
 					exclude: []string{"*", "!**/*-c"},
 					items: []string{
+						"file-c",
 						"file-p",
 						"file-q",
 					},
@@ -913,7 +915,7 @@ func testGetMultiple(t *testing.T) {
 						"something-a",
 						"archive-a",
 						"non-archive-a",
-						// no "file-c" from subdir-a
+						"file-c",
 					},
 				},
 				{
@@ -926,7 +928,7 @@ func testGetMultiple(t *testing.T) {
 						"something-a",
 						"archive-a",
 						"non-archive-a",
-						// no "file-c" from subdir-a
+						"file-c",
 					},
 				},
 				{

tests/conformance/conformance_test.go

@@ -2649,4 +2649,94 @@ var internalTestCases = []testCase{
 		dockerfile: "Dockerfile.7",
 		fsSkip:     []string{"(dir):sub:mtime"},
 	},
+
+	{
+		name:         "dockerignore-allowlist-subdir-nofile-dir",
+		contextDir:   "dockerignore/allowlist/subdir-nofile",
+		shouldFailAt: 2,
+		failureRegex: "no such file or directory",
+	},
+
+	{
+		name:         "dockerignore-allowlist-subdir-nofile-file",
+		contextDir:   "dockerignore/allowlist/subdir-nofile",
+		shouldFailAt: 2,
+		failureRegex: "no such file or directory",
+	},
+
+	{
+		name:       "dockerignore-allowlist-subdir-file-dir",
+		contextDir: "dockerignore/allowlist/subdir-file",
+		fsSkip:     []string{"(dir):f1:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-subdir-file-file",
+		contextDir: "dockerignore/allowlist/subdir-file",
+		fsSkip:     []string{"(dir):f1:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-nothing-dot",
+		contextDir: "dockerignore/allowlist/nothing-dot",
+		fsSkip:     []string{"file:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-nothing-slash",
+		contextDir: "dockerignore/allowlist/nothing-slash",
+		fsSkip:     []string{"file:mtime"},
+	},
+
+	{
+		// the directories are excluded, so entries for them don't get
+		// included in the build context archive, so they only get
+		// created implicitly when extracted, so there's no point in us
+		// trying to preserve any of that, either
+		name:       "dockerignore-allowlist-subsubdir-file",
+		contextDir: "dockerignore/allowlist/subsubdir-file",
+		fsSkip:     []string{"(dir):folder:mtime", "(dir):folder:(dir):subfolder:mtime", "file:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-subsubdir-nofile",
+		contextDir: "dockerignore/allowlist/subsubdir-nofile",
+		fsSkip:     []string{"file:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-subsubdir-nosubdir",
+		contextDir: "dockerignore/allowlist/subsubdir-nosubdir",
+		fsSkip:     []string{"file:mtime"},
+	},
+
+	{
+		name:       "dockerignore-allowlist-alternating",
+		contextDir: "dockerignore/allowlist/alternating",
+		fsSkip: []string{
+			"(dir):subdir1:mtime",
+			"(dir):subdir1:(dir):subdir2:(dir):subdir3:mtime",
+			"(dir):subdir1:(dir):subdir2:(dir):subdir3:(dir):subdir4:(dir):subdir5:mtime",
+			"(dir):subdir2:(dir):subdir3:mtime",
+			"(dir):subdir2:(dir):subdir3:(dir):subdir4:(dir):subdir5:mtime",
+			"(dir):subdir3:mtime",
+			"(dir):subdir3:(dir):subdir4:(dir):subdir5:mtime",
+			"(dir):subdir4:(dir):subdir5:mtime",
+			"(dir):subdir5:mtime",
+		},
+	},
+
+	{
+		name:         "dockerignore-allowlist-alternating-nothing",
+		contextDir:   "dockerignore/allowlist/alternating-nothing",
+		shouldFailAt: 7,
+		failureRegex: "no such file or directory",
+	},
+
+	{
+		name:         "dockerignore-allowlist-alternating-other",
+		contextDir:   "dockerignore/allowlist/alternating-other",
+		shouldFailAt: 7,
+		failureRegex: "no such file or directory",
+	},
 }

tests/conformance/testdata/dockerignore/allowlist/alternating-nothing/.dockerignore

@@ -0,0 +1,7 @@
+**
+!subdir
+subdir/subdir1
+!subdir/subdir1/subdir2
+subdir/subdir1/subdir2/subdir3
+!subdir/subdir1/subdir2/subdir3/subdir4
+subdir/subdir1/subdir2/subdir3/subdir4/subdir5

tests/conformance/testdata/dockerignore/allowlist/alternating-nothing/Dockerfile

@@ -0,0 +1,8 @@
+FROM scratch
+ADD subdir /
+ADD subdir/subdir1 /
+ADD subdir/subdir1/subdir2 /
+ADD subdir/subdir1/subdir2/subdir3 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file /

tests/conformance/testdata/dockerignore/allowlist/alternating-nothing/subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file

@@ -0,0 +1 @@
+Hi, I'm a file.

tests/conformance/testdata/dockerignore/allowlist/alternating-other/.dockerignore

@@ -0,0 +1,8 @@
+**
+!subdir
+subdir/subdir1
+!subdir/subdir1/subdir2
+subdir/subdir1/subdir2/subdir3
+!subdir/subdir1/subdir2/subdir3/subdir4
+subdir/subdir1/subdir2/subdir3/subdir4/subdir5
+!subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file

tests/conformance/testdata/dockerignore/allowlist/alternating-other/Dockerfile

@@ -0,0 +1,8 @@
+FROM scratch
+ADD subdir /
+ADD subdir/subdir1 /
+ADD subdir/subdir1/subdir2 /
+ADD subdir/subdir1/subdir2/subdir3 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file /

tests/conformance/testdata/dockerignore/allowlist/alternating-other/subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file2

@@ -0,0 +1 @@
+Hi, I'm a file.

tests/conformance/testdata/dockerignore/allowlist/alternating/.dockerignore

@@ -0,0 +1,8 @@
+**
+!subdir
+subdir/subdir1
+!subdir/subdir1/subdir2
+subdir/subdir1/subdir2/subdir3
+!subdir/subdir1/subdir2/subdir3/subdir4
+subdir/subdir1/subdir2/subdir3/subdir4/subdir5
+!subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file

tests/conformance/testdata/dockerignore/allowlist/alternating/Dockerfile

@@ -0,0 +1,8 @@
+FROM scratch
+ADD subdir /
+ADD subdir/subdir1 /
+ADD subdir/subdir1/subdir2 /
+ADD subdir/subdir1/subdir2/subdir3 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5 /
+ADD subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file /

tests/conformance/testdata/dockerignore/allowlist/alternating/subdir/subdir1/subdir2/subdir3/subdir4/subdir5/file

@@ -0,0 +1 @@
+Hi, I'm a file.

tests/conformance/testdata/dockerignore/allowlist/nothing-dot/.dockerignore

@@ -0,0 +1 @@
+**

tests/conformance/testdata/dockerignore/allowlist/nothing-dot/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN touch /file
+FROM scratch
+COPY --from=0 /file /
+ADD . .

tests/conformance/testdata/dockerignore/allowlist/nothing-slash/.dockerignore

@@ -0,0 +1 @@
+**

tests/conformance/testdata/dockerignore/allowlist/nothing-slash/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN touch /file
+FROM scratch
+COPY --from=0 /file /
+ADD / /

tests/conformance/testdata/dockerignore/allowlist/subdir-file/.dockerignore

@@ -0,0 +1,2 @@
+**
+!folder1/file1

tests/conformance/testdata/dockerignore/allowlist/subdir-file/Dockerfile

@@ -0,0 +1,2 @@
+FROM scratch
+ADD folder1 /f1

tests/conformance/testdata/dockerignore/allowlist/subdir-file/folder1/file1

@@ -0,0 +1 @@
+Hi, I'm file 1.

tests/conformance/testdata/dockerignore/allowlist/subdir-file/folder1/file2

@@ -0,0 +1 @@
+Hi, I'm file 2.

tests/conformance/testdata/dockerignore/allowlist/subdir-nofile/.dockerignore

@@ -0,0 +1,2 @@
+**
+!folder1/file

tests/conformance/testdata/dockerignore/allowlist/subdir-nofile/Dockerfile

@@ -0,0 +1,2 @@
+FROM scratch
+ADD folder1 /f1

tests/conformance/testdata/dockerignore/allowlist/subdir-nofile/folder1/file1

@@ -0,0 +1 @@
+Hi, I'm file 1.

tests/conformance/testdata/dockerignore/allowlist/subdir-nofile/folder1/file2

@@ -0,0 +1 @@
+Hi, I'm file 2.

tests/conformance/testdata/dockerignore/allowlist/subsubdir-file/.dockerignore

@@ -0,0 +1,2 @@
+**
+!folder/subfolder/file

tests/conformance/testdata/dockerignore/allowlist/subsubdir-file/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN touch /file
+FROM scratch
+COPY --from=0 /file /
+ADD . .

tests/conformance/testdata/dockerignore/allowlist/subsubdir-file/folder/subfolder/file

@@ -0,0 +1 @@
+I'm a file.

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nofile/.dockerignore

@@ -0,0 +1,2 @@
+**
+!folder/subfolder/file

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nofile/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN touch /file
+FROM scratch
+COPY --from=0 /file /
+ADD . .

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nofile/folder/file

@@ -0,0 +1 @@
+I'm a file.

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nosubdir/.dockerignore

@@ -0,0 +1,2 @@
+**
+!folder/subfolder/file

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nosubdir/Dockerfile

@@ -0,0 +1,5 @@
+FROM busybox
+RUN touch /file
+FROM scratch
+COPY --from=0 /file /
+ADD . .

tests/conformance/testdata/dockerignore/allowlist/subsubdir-nosubdir/folder/file

@@ -0,0 +1 @@
+Hi, I'm a file