From 90f8d1b6756825093f92547f6141bd196c5a3a80 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 26 Jul 2021 11:28:10 -0700 Subject: [PATCH 1/4] fileutils: Fix incorrect handling of "**/foo" pattern (*PatternMatcher).Matches includes a special case for when the pattern matches a parent dir, even though it doesn't match the current path. However, it assumes that the parent dir which would match the pattern must have the same number of separators as the pattern itself. This doesn't hold true with a patern like "**/foo". A file foo/bar would have len(parentPathDirs) == 1, which is less than the number of path len(pattern.dirs) == 2... therefore this check would be skipped. Given that "**/foo" matches "foo", I think it's a bug that the "parent subdir matches" check is being skipped in this case. It seems safer to loop over the parent subdirs and check each against the pattern. It's possible there is a safe optimization to check only a certain subset, but the existing logic seems unsafe. Signed-off-by: Aaron Lehmann --- pkg/fileutils/fileutils.go | 7 +++++-- pkg/fileutils/fileutils_test.go | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/fileutils/fileutils.go b/pkg/fileutils/fileutils.go index fd4fb7f08f..31c5f78ec6 100644 --- a/pkg/fileutils/fileutils.go +++ b/pkg/fileutils/fileutils.go @@ -77,8 +77,11 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) { if !match && parentPath != "." { // Check to see if the pattern matches one of our parent dirs. - if len(pattern.dirs) <= len(parentPathDirs) { - match, _ = pattern.match(strings.Join(parentPathDirs[:len(pattern.dirs)], string(os.PathSeparator))) + for i := range parentPathDirs { + match, _ = pattern.match(strings.Join(parentPathDirs[:i+1], string(os.PathSeparator))) + if match { + break + } } } diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go index 36064b6f5f..e11711b653 100644 --- a/pkg/fileutils/fileutils_test.go +++ b/pkg/fileutils/fileutils_test.go @@ -328,6 +328,8 @@ func TestMatches(t *testing.T) { {"dir/**", "dir/file/", true}, {"dir/**", "dir/dir2/file", true}, {"dir/**", "dir/dir2/file/", true}, + {"**/dir", "dir", true}, + {"**/dir", "dir/file", true}, {"**/dir2/*", "dir/dir2/file", true}, {"**/dir2/*", "dir/dir2/file/", true}, {"**/dir2/**", "dir/dir2/dir3/file", true}, From 9bae4f2f246154507aab9b0c5b779133723888a6 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 12 Aug 2021 13:57:50 -0700 Subject: [PATCH 2/4] Add more optimal MatchesUsingParentResult method, use it in pkg/archive Signed-off-by: Aaron Lehmann --- pkg/archive/archive.go | 25 +++++++++++++++- pkg/fileutils/fileutils.go | 52 ++++++++++++++++++++++++++++----- pkg/fileutils/fileutils_test.go | 38 +++++++++++++++++++----- 3 files changed, 99 insertions(+), 16 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 38cba551a8..25ca3959cb 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -817,6 +817,11 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) for _, include := range options.IncludeFiles { rebaseName := options.RebaseNames[include] + var ( + parentMatched []bool + parentDirs []string + ) + walkRoot := getWalkRoot(srcPath, include) filepath.Walk(walkRoot, func(filePath string, f os.FileInfo, err error) error { if err != nil { @@ -843,11 +848,29 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) // is asking for that file no matter what - which is true // for some files, like .dockerignore and Dockerfile (sometimes) if include != relFilePath { - skip, err = pm.Matches(relFilePath) + for len(parentDirs) != 0 { + lastParentDir := parentDirs[len(parentDirs)-1] + if strings.HasPrefix(relFilePath, lastParentDir+string(os.PathSeparator)) { + break + } + parentDirs = parentDirs[:len(parentDirs)-1] + parentMatched = parentMatched[:len(parentMatched)-1] + } + + if len(parentMatched) != 0 { + skip, err = pm.MatchesUsingParentResult(relFilePath, parentMatched[len(parentMatched)-1]) + } else { + skip, err = pm.Matches(relFilePath) + } if err != nil { logrus.Errorf("Error matching %s: %v", relFilePath, err) return err } + + if f.IsDir() { + parentDirs = append(parentDirs, relFilePath) + parentMatched = append(parentMatched, skip) + } } if skip { diff --git a/pkg/fileutils/fileutils.go b/pkg/fileutils/fileutils.go index 31c5f78ec6..b90a5b48ed 100644 --- a/pkg/fileutils/fileutils.go +++ b/pkg/fileutils/fileutils.go @@ -55,8 +55,12 @@ func NewPatternMatcher(patterns []string) (*PatternMatcher, error) { return pm, nil } -// Matches matches path against all the patterns. Matches is not safe to be -// called concurrently +// Matches returns true if "file" matches any of the patterns +// and isn't excluded by any of the subsequent patterns. +// +// The "file" argument should be a slash-delimited path. +// +// Matches is not safe to call concurrently. func (pm *PatternMatcher) Matches(file string) (bool, error) { matched := false file = filepath.FromSlash(file) @@ -64,10 +68,11 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) { parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) for _, pattern := range pm.patterns { - negative := false - - if pattern.exclusion { - negative = true + // Skip evaluation if this is an inclusion and the filename + // already matched the pattern, or it's an exclusion and it has + // not matched the pattern yet. + if pattern.exclusion != matched { + continue } match, err := pattern.match(file) @@ -86,13 +91,45 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) { } if match { - matched = !negative + matched = !pattern.exclusion } } return matched, nil } +// MatchesUsingParentResult returns true if "file" matches any of the patterns +// and isn't excluded by any of the subsequent patterns. The functionality is +// the same as Matches, but as an optimization, the caller keeps track of +// whether the parent directory matched. +// +// The "file" argument should be a slash-delimited path. +// +// MatchesUsingParentResult is not safe to call concurrently. +func (pm *PatternMatcher) MatchesUsingParentResult(file string, parentMatched bool) (bool, error) { + matched := parentMatched + file = filepath.FromSlash(file) + + for _, pattern := range pm.patterns { + // Skip evaluation if this is an inclusion and the filename + // already matched the pattern, or it's an exclusion and it has + // not matched the pattern yet. + if pattern.exclusion != matched { + continue + } + + match, err := pattern.match(file) + if err != nil { + return false, err + } + + if match { + matched = !pattern.exclusion + } + } + return matched, nil +} + // Exclusions returns true if any of the patterns define exclusions func (pm *PatternMatcher) Exclusions() bool { return pm.exclusions @@ -121,7 +158,6 @@ func (p *Pattern) Exclusion() bool { } func (p *Pattern) match(path string) (bool, error) { - if p.regexp == nil { if err := p.compile(); err != nil { return false, filepath.ErrBadPattern diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go index e11711b653..5147c23597 100644 --- a/pkg/fileutils/fileutils_test.go +++ b/pkg/fileutils/fileutils_test.go @@ -382,13 +382,37 @@ func TestMatches(t *testing.T) { }...) } - for _, test := range tests { - desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text) - pm, err := NewPatternMatcher([]string{test.pattern}) - assert.NilError(t, err, desc) - res, _ := pm.Matches(test.text) - assert.Check(t, is.Equal(test.pass, res), desc) - } + t.Run("Matches", func(t *testing.T) { + for _, test := range tests { + desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text) + pm, err := NewPatternMatcher([]string{test.pattern}) + assert.NilError(t, err, desc) + res, _ := pm.Matches(test.text) + assert.Check(t, is.Equal(test.pass, res), desc) + } + }) + + t.Run("MatchesUsingParentResult", func(t *testing.T) { + for _, test := range tests { + desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text) + pm, err := NewPatternMatcher([]string{test.pattern}) + assert.NilError(t, err, desc) + + parentPath := path.Dir(test.text) + parentPathDirs := strings.Split(parentPath, "/") + + parentMatched := false + if parentPath != "." { + for i := range parentPathDirs { + parentMatched, _ = pm.MatchesUsingParentResult(strings.Join(parentPathDirs[:i+1], "/"), parentMatched) + } + } + + res, _ := pm.MatchesUsingParentResult(test.text, parentMatched) + assert.Check(t, is.Equal(test.pass, res), desc) + } + }) + } func TestCleanPatterns(t *testing.T) { From 97ede9df264c08bcf752c70569d6c87fe5c9e98d Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 12 Aug 2021 18:09:12 -0700 Subject: [PATCH 3/4] Rename Matches to MatchesOrParentMatches Signed-off-by: Aaron Lehmann --- builder/remotecontext/detect.go | 2 +- pkg/archive/archive.go | 2 +- pkg/fileutils/fileutils.go | 64 +++++++++++++++++++++++++++++++++ pkg/fileutils/fileutils_test.go | 4 +-- 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/builder/remotecontext/detect.go b/builder/remotecontext/detect.go index 9b126ef775..09f4d28434 100644 --- a/builder/remotecontext/detect.go +++ b/builder/remotecontext/detect.go @@ -130,7 +130,7 @@ func removeDockerfile(c modifiableContext, filesToRemove ...string) error { f.Close() filesToRemove = append([]string{".dockerignore"}, filesToRemove...) for _, fileToRemove := range filesToRemove { - if rm, _ := fileutils.Matches(fileToRemove, excludes); rm { + if rm, _ := fileutils.MatchesOrParentMatches(fileToRemove, excludes); rm { if err := c.Remove(fileToRemove); err != nil { logrus.Errorf("failed to remove %s: %v", fileToRemove, err) } diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 25ca3959cb..82d8d0eafa 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -860,7 +860,7 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) if len(parentMatched) != 0 { skip, err = pm.MatchesUsingParentResult(relFilePath, parentMatched[len(parentMatched)-1]) } else { - skip, err = pm.Matches(relFilePath) + skip, err = pm.MatchesOrParentMatches(relFilePath) } if err != nil { logrus.Errorf("Error matching %s: %v", relFilePath, err) diff --git a/pkg/fileutils/fileutils.go b/pkg/fileutils/fileutils.go index b90a5b48ed..da4ffd13e2 100644 --- a/pkg/fileutils/fileutils.go +++ b/pkg/fileutils/fileutils.go @@ -61,12 +61,56 @@ func NewPatternMatcher(patterns []string) (*PatternMatcher, error) { // The "file" argument should be a slash-delimited path. // // Matches is not safe to call concurrently. +// +// This implementation is buggy (it only checks a single parent dir against the +// pattern) and will be removed soon. Use either MatchesOrParentMatches or +// MatchesUsingParentResult instead. func (pm *PatternMatcher) Matches(file string) (bool, error) { matched := false file = filepath.FromSlash(file) parentPath := filepath.Dir(file) parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) + for _, pattern := range pm.patterns { + // Skip evaluation if this is an inclusion and the filename + // already matched the pattern, or it's an exclusion and it has + // not matched the pattern yet. + if pattern.exclusion != matched { + continue + } + + match, err := pattern.match(file) + if err != nil { + return false, err + } + + if !match && parentPath != "." { + // Check to see if the pattern matches one of our parent dirs. + if len(pattern.dirs) <= len(parentPathDirs) { + match, _ = pattern.match(strings.Join(parentPathDirs[:len(pattern.dirs)], string(os.PathSeparator))) + } + } + + if match { + matched = !pattern.exclusion + } + } + + return matched, nil +} + +// MatchesOrParentMatches returns true if "file" matches any of the patterns +// and isn't excluded by any of the subsequent patterns. +// +// The "file" argument should be a slash-delimited path. +// +// Matches is not safe to call concurrently. +func (pm *PatternMatcher) MatchesOrParentMatches(file string) (bool, error) { + matched := false + file = filepath.FromSlash(file) + parentPath := filepath.Dir(file) + parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) + for _, pattern := range pm.patterns { // Skip evaluation if this is an inclusion and the filename // already matched the pattern, or it's an exclusion and it has @@ -249,6 +293,9 @@ func (p *Pattern) compile() error { // Matches returns true if file matches any of the patterns // and isn't excluded by any of the subsequent patterns. +// +// This implementation is buggy (it only checks a single parent dir against the +// pattern) and will be removed soon. Use MatchesOrParentMatches instead. func Matches(file string, patterns []string) (bool, error) { pm, err := NewPatternMatcher(patterns) if err != nil { @@ -264,6 +311,23 @@ func Matches(file string, patterns []string) (bool, error) { return pm.Matches(file) } +// MatchesOrParentMatches returns true if file matches any of the patterns +// and isn't excluded by any of the subsequent patterns. +func MatchesOrParentMatches(file string, patterns []string) (bool, error) { + pm, err := NewPatternMatcher(patterns) + if err != nil { + return false, err + } + file = filepath.Clean(file) + + if file == "." { + // Don't let them exclude everything, kind of silly. + return false, nil + } + + return pm.MatchesOrParentMatches(file) +} + // CopyFile copies from src to dst until either EOF is reached // on src or an error occurs. It verifies src exists and removes // the dst if it exists. diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go index 5147c23597..7ac45a2c70 100644 --- a/pkg/fileutils/fileutils_test.go +++ b/pkg/fileutils/fileutils_test.go @@ -382,12 +382,12 @@ func TestMatches(t *testing.T) { }...) } - t.Run("Matches", func(t *testing.T) { + t.Run("MatchesOrParentMatches", func(t *testing.T) { for _, test := range tests { desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text) pm, err := NewPatternMatcher([]string{test.pattern}) assert.NilError(t, err, desc) - res, _ := pm.Matches(test.text) + res, _ := pm.MatchesOrParentMatches(test.text) assert.Check(t, is.Equal(test.pass, res), desc) } }) From c44b90f3bf5b9e1dc97087662482ad3a16f3d14f Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Thu, 12 Aug 2021 20:02:16 -0700 Subject: [PATCH 4/4] Test fix for Windows compatibility Signed-off-by: Aaron Lehmann --- pkg/fileutils/fileutils_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go index 7ac45a2c70..1e93b3e29d 100644 --- a/pkg/fileutils/fileutils_test.go +++ b/pkg/fileutils/fileutils_test.go @@ -398,8 +398,8 @@ func TestMatches(t *testing.T) { pm, err := NewPatternMatcher([]string{test.pattern}) assert.NilError(t, err, desc) - parentPath := path.Dir(test.text) - parentPathDirs := strings.Split(parentPath, "/") + parentPath := filepath.Dir(filepath.FromSlash(test.text)) + parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) parentMatched := false if parentPath != "." {