feat(controller): support filtering Git subscriptions using globs and…

… path prefixes (#1778)

Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>

api/v1alpha1/warehouse_types.go

@@ -126,18 +126,26 @@ type GitSubscription struct {
 	// should be ignored when connecting to the repository. This should be enabled
 	// only with great caution.
 	InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty" protobuf:"varint,7,opt,name=insecureSkipTLSVerify"`
-	// IncludePaths is a list of regular expressions that can optionally be used to
+	// IncludePaths is a list of filter strings that can optionally be used to
 	// limit file paths in repository, changes in which will result in creation of
 	// new freight. When not specified - changes in any path will produce new
-	// freight, it is equivalent to having a IncludePaths with an entry of ".*"
+	// freight, it is equivalent to having a IncludePaths with an entry of "regex:.*"
 	// When both IncludePaths and ExcludePaths are specified and match same path/paths,
 	// ExcludePaths will prevail over IncludePaths.
+	// Filter strings can be specified as prefix/exact match strings, globs or regular
+	// expressions, default being prefix/exact match, for example "helm/charts/", for
+	// regular expressions strings need to be prepended by either "regex:" or "regexp:"
+	// as in "regexp:^.*values\.yaml$", for globs with "glob:" as in "glob:helm/*/*/values.*ml"
 	//+kubebuilder:validation:Optional
 	IncludePaths []string `json:"includePaths,omitempty" protobuf:"bytes,8,rep,name=includePaths"`
-	// ExcludePaths is an optional list of regular expressions used to specify paths
+	// ExcludePaths is an optional list of filter strings used to specify paths
 	// in git repository, changes in which should never produce new freight. When used
 	// in conjuction with IncludePaths, both matching same path/paths, ExcludePaths takes
 	// precedence over IncludePaths.
+	// Filter strings can be specified as prefix/exact match strings, globs or regular
+	// expressions, default being prefix/exact match, for example "helm/charts/", for
+	// regular expressions strings need to be prepended by either "regex:" or "regexp:"
+	// as in "regexp:^.*values\.yaml$", for globs with "glob:" as in "glob:helm/*/*/values.*ml"
 	//+kubebuilder:validation:Optional
 	ExcludePaths []string `json:"excludePaths,omitempty" protobuf:"bytes,9,rep,name=excludePaths"`
 }

charts/kargo/resources/crds/kargo.akuity.io_warehouses.yaml

@@ -138,10 +138,14 @@ spec:
                           type: string
                         excludePaths:
                           description: |-
-                            ExcludePaths is an optional list of regular expressions used to specify paths
+                            ExcludePaths is an optional list of filter strings used to specify paths
                             in git repository, changes in which should never produce new freight. When used
                             in conjuction with IncludePaths, both matching same path/paths, ExcludePaths takes
                             precedence over IncludePaths.
+                            Filter strings can be specified as prefix/exact match strings, globs or regular
+                            expressions, default being prefix/exact match, for example "helm/charts/", for
+                            regular expressions strings need to be prepended by either "regex:" or "regexp:"
+                            as in "regexp:^.*values\.yaml$", for globs with "glob:" as in "glob:helm/*/*/values.*ml"
                           items:
                             type: string
                           type: array
@@ -157,12 +161,16 @@ spec:
                           type: array
                         includePaths:
                           description: |-
-                            IncludePaths is a list of regular expressions that can optionally be used to
+                            IncludePaths is a list of filter strings that can optionally be used to
                             limit file paths in repository, changes in which will result in creation of
                             new freight. When not specified - changes in any path will produce new
-                            freight, it is equivalent to having a IncludePaths with an entry of ".*"
+                            freight, it is equivalent to having a IncludePaths with an entry of "regex:.*"
                             When both IncludePaths and ExcludePaths are specified and match same path/paths,
                             ExcludePaths will prevail over IncludePaths.
+                            Filter strings can be specified as prefix/exact match strings, globs or regular
+                            expressions, default being prefix/exact match, for example "helm/charts/", for
+                            regular expressions strings need to be prepended by either "regex:" or "regexp:"
+                            as in "regexp:^.*values\.yaml$", for globs with "glob:" as in "glob:helm/*/*/values.*ml"
                           items:
                             type: string
                           type: array

internal/controller/warehouses/git.go

@@ -3,6 +3,7 @@ package warehouses
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
@@ -18,6 +19,7 @@ import (
 const (
 	regexpPrefix = "regexp:"
 	regexPrefix  = "regex:"
+	globPrefix   = "glob:"
 )
 
 type gitMeta struct {
@@ -27,6 +29,8 @@ type gitMeta struct {
 	Author  string
 }
 
+type pathSelector func(path string) (bool, error)
+
 func (r *reconciler) selectCommits(
 	ctx context.Context,
 	namespace string,
@@ -303,92 +307,91 @@ func ignores(tagName string, ignore []string) bool {
 	return false
 }
 
-// pathsFilterPositive returns true when IncludePaths and/or ExcludePaths
-// filters match one or more commit diffs and new Freight is
-// to be produced. It returns false otherwise.
+func getPathSelectors(selectorStrs []string) ([]pathSelector, error) {
+	selectors := make([]pathSelector, len(selectorStrs))
+	for i, selectorStr := range selectorStrs {
+		switch {
+		case strings.HasPrefix(selectorStr, regexpPrefix):
+			regex, err := regexp.Compile(strings.TrimPrefix(selectorStr, regexpPrefix))
+			if err != nil {
+				return nil, err
+			}
+			selectors[i] = func(path string) (bool, error) {
+				return regex.MatchString(path), nil
+			}
+		case strings.HasPrefix(selectorStr, regexPrefix):
+			regex, err := regexp.Compile(strings.TrimPrefix(selectorStr, regexPrefix))
+			if err != nil {
+				return nil, err
+			}
+			selectors[i] = func(path string) (bool, error) {
+				return regex.MatchString(path), nil
+			}
+		case strings.HasPrefix(selectorStr, globPrefix):
+			pattern := strings.TrimPrefix(selectorStr, globPrefix)
+			selectors[i] = func(path string) (bool, error) {
+				return filepath.Match(pattern, path)
+			}
+		default:
+			basePath := selectorStr
+			selectors[i] = func(path string) (bool, error) {
+				relPath, err := filepath.Rel(basePath, path)
+				if err != nil {
+					return false, err
+				}
+				return !strings.Contains(relPath, ".."), nil
+			}
+		}
+	}
+	return selectors, nil
+}
+
 func matchesPathsFilters(includePaths []string, excludePaths []string, diffs []string) (bool, error) {
-	includePathsRegexps, err := compileRegexps(includePaths)
+	includeSelectors, err := getPathSelectors(includePaths)
 	if err != nil {
-		return false, fmt.Errorf(
-			"error compiling includePaths regexps: %w",
-			err,
-		)
+		return false, err
 	}
-
-	excludePathsRegexps, err := compileRegexps(excludePaths)
+	excludeSelectors, err := getPathSelectors(excludePaths)
 	if err != nil {
-		return false, fmt.Errorf(
-			"error compiling excludePaths regexps: %w",
-			err,
-		)
+		return false, err
 	}
-
-	filteredDiffs := make([]string, 0, len(diffs))
-	for _, diffPath := range diffs {
-		// matchesIncludePaths case is a bit different from matchesExcludePaths
-		// in the way that if includePaths string array is empty - it matches
-		// ANY change so we need to have a check for that
-		matchesIncludePaths := len(includePaths) == 0 || matchesRegexpList(diffPath, includePathsRegexps)
-		matchesExcludePaths := matchesRegexpList(diffPath, excludePathsRegexps)
-		// combined filter decision, positive for matching includePaths and
-		// unmatching excludePaths
-		if matchesIncludePaths && !matchesExcludePaths {
-			filteredDiffs = append(filteredDiffs, diffPath)
+pathLoop:
+	for _, path := range diffs {
+		if len(includeSelectors) > 0 {
+			var selected bool
+			for _, selector := range includeSelectors {
+				if selected, err = selector(path); err != nil {
+					return false, err
+				}
+				if selected {
+					// Path was explicitly included, so we can move on to checking if
+					// it should be excluded
+					break
+				}
+			}
+			if !selected {
+				// Path was not explicitly included, so we can move on to the next path
+				continue pathLoop
+			}
 		}
-	}
-	if len(filteredDiffs) > 0 {
+		// If we reach this point, the path was either implicitly or explicitly
+		// included. Now check if it should be excluded.
+		for _, selector := range excludeSelectors {
+			selected, err := selector(path)
+			if err != nil {
+				return false, err
+			}
+			if selected {
+				// Path was explicitly excluded, so we can move on to the next path
+				continue pathLoop
+			}
+		}
+		// If we reach this point, the path was not explicitly excluded
 		return true, nil
 	}
 	return false, nil
 }
 
-// compileRegexps is a general purpose function taking a slice of strings
-// as argument and compiling them into a slice of *regexp.Regexp. It
-// returns the compiled regexps in case of success and if it fails to compile
-// a string - nil and error is returned
-func compileRegexps(regexpStrings []string) (regexps []*regexp.Regexp, err error) {
-	regexpsSlice := make([]*regexp.Regexp, 0, len(regexpStrings))
-	for _, regexpString := range regexpStrings {
-		switch {
-		case strings.HasPrefix(regexpString, regexpPrefix):
-			regexpString = strings.TrimPrefix(regexpString, regexpPrefix)
-		case strings.HasPrefix(regexpString, regexPrefix):
-			regexpString = strings.TrimPrefix(regexpString, regexPrefix)
-		default:
-			return nil, fmt.Errorf(
-				"error compiling %q into a regular expression: string must start with %q or %q",
-				regexpString, regexpPrefix, regexPrefix,
-			)
-		}
-
-		regex, err := regexp.Compile(regexpString)
-		if err != nil {
-			return nil, fmt.Errorf(
-				"error compiling string %q into a regular expression: %w",
-				regexpString,
-				err,
-			)
-		}
-		regexpsSlice = append(regexpsSlice, regex)
-	}
-	return regexpsSlice, nil
-}
-
-// matchesRegexpList is a general purpose function iterating given slice of
-// *regexp.Regexp (regexpList) to check if any of regexps match
-// stringToMatch string, if match is found it returns true and if match
-// is not found it returns false.
-func matchesRegexpList(stringToMatch string, regexpList []*regexp.Regexp) bool {
-	foundMatch := false
-	for _, regex := range regexpList {
-		if regex == nil || regex.MatchString(stringToMatch) {
-			foundMatch = true
-			break
-		}
-	}
-	return foundMatch
-}
-
 // selectLexicallyLastTag sorts the provided tag name in reverse lexicographic
 // order and returns the first tag name in the sorted list. If the list is
 // empty, it returns an empty string.

internal/controller/warehouses/git_test.go

@@ -269,12 +269,12 @@ func TestSelectCommitID(t *testing.T) {
 				require.Contains(
 					t,
 					err.Error(),
-					"error checking includePaths/excludePaths match for commit \"fake-commit\"",
+					"error checking includePaths/excludePaths match for commit",
 				)
 				require.Contains(
 					t,
 					err.Error(),
-					"error compiling includePaths regexps: error compiling string \"[\" into a regular expression",
+					"error parsing regexp: missing closing ]",
 				)
 				require.Contains(t, err.Error(), "error parsing regexp: missing closing ]")
 			},
@@ -681,7 +681,7 @@ func TestMatchesPathsFilters(t *testing.T) {
 			},
 		},
 		{
-			name:         "success with a matching filters configuration",
+			name:         "success with a matching regexp filters configuration",
 			includePaths: []string{regexpPrefix + "values\\.ya?ml$"},
 			excludePaths: []string{regexPrefix + "nonexistent"},
 			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
@@ -691,7 +691,7 @@ func TestMatchesPathsFilters(t *testing.T) {
 			},
 		},
 		{
-			name:         "success with unmatching filters configuration",
+			name:         "success with unmatching regexp filters configuration",
 			includePaths: []string{regexpPrefix + "values\\.ya?ml$"},
 			excludePaths: []string{regexPrefix + "nonexistent", regexpPrefix + ".*val.*"},
 			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
@@ -701,32 +701,62 @@ func TestMatchesPathsFilters(t *testing.T) {
 			},
 		},
 		{
-			name:         "error with invalid regexp in excludePaths configuration",
-			includePaths: []string{regexPrefix + "values\\.ya?ml$"},
-			excludePaths: []string{regexpPrefix + "nonexistent", regexpPrefix + ".*val.*", regexPrefix + "["},
+			name:         "success with matching glob filters configuration",
+			includePaths: []string{"glob:path2/*.tpl"},
+			excludePaths: []string{"nonexistent"},
 			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
-			assertions: func(t *testing.T, _ bool, err error) {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), "error compiling excludePaths regexps:")
-				require.Contains(t, err.Error(), "error compiling string \"[\" into a regular expression")
+			assertions: func(t *testing.T, matchFound bool, err error) {
+				require.NoError(t, err)
+				require.Equal(t, true, matchFound)
+			},
+		},
+		{
+			name:         "success with unmatching glob filters configuration",
+			includePaths: []string{"path2/*.tpl"},
+			excludePaths: []string{regexPrefix + "nonexistent", "*/?helpers.tpl"},
+			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
+			assertions: func(t *testing.T, matchFound bool, err error) {
+				require.NoError(t, err)
+				require.Equal(t, false, matchFound)
+			},
+		},
+		{
+			name:         "success with matching prefix filters configuration",
+			includePaths: []string{"path1/"},
+			excludePaths: []string{"nonexistent"},
+			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
+			assertions: func(t *testing.T, matchFound bool, err error) {
+				require.NoError(t, err)
+				require.Equal(t, true, matchFound)
+			},
+		},
+		{
+			name:         "success with unmatching prefix filters configuration",
+			includePaths: []string{"path3/"},
+			excludePaths: []string{regexPrefix + "nonexistent", "*/?helpers.tpl"},
+			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
+			assertions: func(t *testing.T, matchFound bool, err error) {
+				require.NoError(t, err)
+				require.Equal(t, false, matchFound)
 			},
 		},
 		{
-			name:         "error without prefix in includePaths configuration",
-			includePaths: []string{"values\\.ya?ml$"},
+			name:         "error with invalid regexp in excludePaths configuration",
+			includePaths: []string{regexPrefix + "values\\.ya?ml$"},
+			excludePaths: []string{regexpPrefix + "nonexistent", regexpPrefix + ".*val.*", regexPrefix + "["},
 			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
 			assertions: func(t *testing.T, _ bool, err error) {
 				require.Error(t, err)
-				require.ErrorContains(t, err, "string must start with")
+				require.Contains(t, err.Error(), "error parsing regexp: missing closing ]: `[`")
 			},
 		},
 		{
-			name:         "error without prefix in excludePaths configuration",
-			excludePaths: []string{"values\\.ya?ml$"},
+			name:         "error with invalid glob syntax",
+			includePaths: []string{"glob:path2/*.tpl["},
 			diffs:        []string{"path1/values.yaml", "path2/_helpers.tpl"},
 			assertions: func(t *testing.T, _ bool, err error) {
 				require.Error(t, err)
-				require.ErrorContains(t, err, "string must start with")
+				require.ErrorContains(t, err, "syntax error in pattern")
 			},
 		},
 	}