fix: add optional safer Git file generator globbing (argoproj#13313) (a…

…rgoproj#13314)

Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

applicationset/services/repo_service.go

@@ -18,10 +18,11 @@ type RepositoryDB interface {
 }
 
 type argoCDService struct {
-	repositoriesDB      RepositoryDB
-	storecreds          git.CredsStore
-	submoduleEnabled    bool
-	repoServerClientSet apiclient.Clientset
+	repositoriesDB         RepositoryDB
+	storecreds             git.CredsStore
+	submoduleEnabled       bool
+	repoServerClientSet    apiclient.Clientset
+	newFileGlobbingEnabled bool
 }
 
 type Repos interface {
@@ -33,11 +34,12 @@ type Repos interface {
 	GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error)
 }
 
-func NewArgoCDService(db db.ArgoDB, submoduleEnabled bool, repoClientset apiclient.Clientset) (Repos, error) {
+func NewArgoCDService(db db.ArgoDB, submoduleEnabled bool, repoClientset apiclient.Clientset, newFileGlobbingEnabled bool) (Repos, error) {
 	return &argoCDService{
-		repositoriesDB:      db.(RepositoryDB),
-		submoduleEnabled:    submoduleEnabled,
-		repoServerClientSet: repoClientset,
+		repositoriesDB:         db.(RepositoryDB),
+		submoduleEnabled:       submoduleEnabled,
+		repoServerClientSet:    repoClientset,
+		newFileGlobbingEnabled: newFileGlobbingEnabled,
 	}, nil
 }
 
@@ -48,10 +50,11 @@ func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision s
 	}
 
 	fileRequest := &apiclient.GitFilesRequest{
-		Repo:             repo,
-		SubmoduleEnabled: a.submoduleEnabled,
-		Revision:         revision,
-		Path:             pattern,
+		Repo:                      repo,
+		SubmoduleEnabled:          a.submoduleEnabled,
+		Revision:                  revision,
+		Path:                      pattern,
+		NewGitFileGlobbingEnabled: a.newFileGlobbingEnabled,
 	}
 	closer, client, err := a.repoServerClientSet.NewRepoServerClient()
 	if err != nil {

applicationset/services/repo_service_test.go

@@ -185,7 +185,7 @@ func TestGetFiles(t *testing.T) {
 }
 
 func TestNewArgoCDService(t *testing.T) {
-	service, err := NewArgoCDService(&db_mocks.ArgoDB{}, false, &repo_mocks.Clientset{})
+	service, err := NewArgoCDService(&db_mocks.ArgoDB{}, false, &repo_mocks.Clientset{}, false)
 	assert.NoError(t, err, err)
 	assert.NotNil(t, service)
 }

cmd/argocd-applicationset-controller/commands/applicationset_controller.go

@@ -58,6 +58,7 @@ func NewCommand() *cobra.Command {
 		debugLog                     bool
 		dryRun                       bool
 		enableProgressiveSyncs       bool
+		enableNewGitFileGlobbing     bool
 		repoServerPlaintext          bool
 		repoServerStrictTLS          bool
 		repoServerTimeoutSeconds     int
@@ -141,7 +142,7 @@ func NewCommand() *cobra.Command {
 			}
 
 			repoClientset := apiclient.NewRepoServerClientset(argocdRepoServer, repoServerTimeoutSeconds, tlsConfig)
-			argoCDService, err := services.NewArgoCDService(argoCDDB, getSubmoduleEnabled(), repoClientset)
+			argoCDService, err := services.NewArgoCDService(argoCDDB, getSubmoduleEnabled(), repoClientset, enableNewGitFileGlobbing)
 			errors.CheckError(err)
 
 			terminalGenerators := map[string]generators.Generator{
@@ -224,6 +225,7 @@ func NewCommand() *cobra.Command {
 	command.Flags().StringVar(&cmdutil.LogLevel, "loglevel", env.StringFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_LOGLEVEL", "info"), "Set the logging level. One of: debug|info|warn|error")
 	command.Flags().BoolVar(&dryRun, "dry-run", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_DRY_RUN", false), "Enable dry run mode")
 	command.Flags().BoolVar(&enableProgressiveSyncs, "enable-progressive-syncs", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_PROGRESSIVE_SYNCS", false), "Enable use of the experimental progressive syncs feature.")
+	command.Flags().BoolVar(&enableNewGitFileGlobbing, "enable-new-git-file-globbing", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING", false), "Enable new globbing in Git files generator.")
 	command.Flags().BoolVar(&repoServerPlaintext, "repo-server-plaintext", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT", false), "Disable TLS on connections to repo server")
 	command.Flags().BoolVar(&repoServerStrictTLS, "repo-server-strict-tls", env.ParseBoolFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_STRICT_TLS", false), "Whether to use strict validation of the TLS cert presented by the repo server")
 	command.Flags().IntVar(&repoServerTimeoutSeconds, "repo-server-timeout-seconds", env.ParseNumFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS", 60, 0, math.MaxInt64), "Repo server RPC call timeout seconds.")

docs/operator-manual/applicationset/Generators-Git-File-Globbing.md

@@ -0,0 +1,85 @@
+# Git File Generator Globbing
+
+## Problem Statement
+
+The original and default implementation of the Git file generator does very greedy globbing. This can trigger errors or catch users off-guard. For example, consider the following repository layout:
+
+```
+└── cluster-charts/
+    ├── cluster1
+    │   ├── mychart/
+    │   │   ├── charts/
+    │   │   │    └── mysubchart/
+    │   │   │        ├── values.yaml
+    │   │   │        └── etc…
+    │   │   ├── values.yaml
+    │   │   └── etc…
+    │   └── myotherchart/
+    │       ├── values.yaml
+    │       └── etc…
+    └── cluster2
+        └── etc…
+```
+
+In `cluster1` we have two charts, one of them with a subchart.
+
+Assuming we need the ApplicationSet to template values in the `values.yaml`, then we need to use a Git file generator instead of a directory generator. The value of the `path` key of the Git file generator should be set to:
+
+```
+path: cluster-charts/*/*/values.yaml
+```
+
+However, the default implementation will interpret the above pattern as:
+
+```
+path: cluster-charts/**/values.yaml
+```
+
+Meaning, for `mychart` in `cluster1`, that it will pick up both the chart's `values.yaml` but also the one from its subchart. This will most likely fail, and even if it didn't it would be wrong.
+
+There are multiple other ways this undesirable globbing can fail. For example:
+
+```
+path: some-path/*.yaml
+```
+
+This will return all YAML files in any directory at any level under `some-path`, instead of only those directly under it.
+
+## Enabling the New Globbing
+
+Since some users may rely on the old behavior it was decided to make the fix optional and not enabled by default.
+
+It can be enabled in any of these ways:
+
+1. Pass `--enable-new-git-file-globbing` to the ApplicationSet controller args.
+1. Set `ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING=true` in the ApplicationSet controller environment variables.
+1. Set `applicationsetcontroller.enable.new.git.file.globbing: true` in the Argo CD ConfigMap.
+
+Note that the default may change in the future.
+
+## Usage
+
+The new Git file generator globbing uses the `doublestar` package. You can find it [here](https://github.com/bmatcuk/doublestar).
+
+Below is a short excerpt from its documentation.
+
+doublestar patterns match files and directories recursively. For example, if
+you had the following directory structure:
+
+```bash
+grandparent
+`-- parent
+    |-- child1
+    `-- child2
+```
+
+You could find the children with patterns such as: `**/child*`,
+`grandparent/**/child?`, `**/parent/*`, or even just `**` by itself (which will
+return all files and directories recursively).
+
+Bash's globstar is doublestar's inspiration and, as such, works similarly.
+Note that the doublestar must appear as a path component by itself. A pattern
+such as `/path**` is invalid and will be treated the same as `/path*`, but
+`/path*/**` should achieve the desired result. Additionally, `/path/**` will
+match all directories and files under the path directory, but `/path/**/` will
+only match directories.

docs/operator-manual/applicationset/Generators-Git.md

@@ -288,6 +288,8 @@ The filename can always be accessed using `{{path.filename}}`.
 
 **Note**: If the `pathParamPrefix` option is specified, all `path`-related parameter names above will be prefixed with the specified value and a dot separator. E.g., if `pathParamPrefix` is `myRepo`, then the generated parameter name would be `myRepo.path` instead of `path`. Using this option is necessary in a Matrix generator where both child generators are Git generators (to avoid conflicts when merging the child generators’ items).
 
+**Note**: The default behavior of the Git file generator is very greedy. Please see [Git File Generator Globbing](./Generators-Git-File-Globbing.md) for more information.
+
 ## Webhook Configuration
 
 When using a Git generator, ApplicationSet polls Git repositories every three minutes to detect changes. To eliminate

go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/argoproj/notifications-engine v0.4.1-0.20230228182525-f754726f03da
 	github.com/argoproj/pkg v0.13.7-0.20221221191914-44694015343d
 	github.com/aws/aws-sdk-go v1.44.164
+	github.com/bmatcuk/doublestar/v4 v4.6.0
 	github.com/bombsimon/logrusr/v2 v2.0.1
 	github.com/bradleyfalzon/ghinstallation/v2 v2.1.0
 	github.com/casbin/casbin/v2 v2.60.0

go.sum

@@ -168,6 +168,8 @@ github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJm
 github.com/bketelsen/crypt v0.0.4/go.mod h1:aI6NrJ0pMGgvZKL1iVgXLnfIFJtfV+bKCoqOes/6LfM=
 github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
+github.com/bmatcuk/doublestar/v4 v4.6.0 h1:HTuxyug8GyFbRkrffIpzNCSK4luc0TY3wzXvzIZhEXc=
+github.com/bmatcuk/doublestar/v4 v4.6.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc=
 github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps=
 github.com/bombsimon/logrusr/v2 v2.0.1 h1:1VgxVNQMCvjirZIYaT9JYn6sAVGVEcNtRE0y4mvaOAM=
 github.com/bombsimon/logrusr/v2 v2.0.1/go.mod h1:ByVAX+vHdLGAfdroiMg6q0zgq2FODY2lc5YJvzmOJio=

manifests/base/applicationset-controller/argocd-applicationset-controller-deployment.yaml

@@ -91,6 +91,12 @@ spec:
                   key: applicationsetcontroller.enable.progressive.syncs
                   name: argocd-cmd-params-cm
                   optional: true
+            - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+              valueFrom:
+                configMapKeyRef:
+                  key: applicationsetcontroller.enable.new.git.file.globbing
+                  name: argocd-cmd-params-cm
+                  optional: true
             - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
               valueFrom:
                 configMapKeyRef:
@@ -162,4 +168,4 @@ spec:
               - key: tls.key
                 path: tls.key
               - key: ca.crt
-                path: ca.crt
+                path: ca.crt

manifests/core-install.yaml

@@ -16723,6 +16723,12 @@ spec:
               key: applicationsetcontroller.enable.progressive.syncs
               name: argocd-cmd-params-cm
               optional: true
+        - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+          valueFrom:
+            configMapKeyRef:
+              key: applicationsetcontroller.enable.new.git.file.globbing
+              name: argocd-cmd-params-cm
+              optional: true
         - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
           valueFrom:
             configMapKeyRef:

manifests/ha/install.yaml

@@ -17944,6 +17944,12 @@ spec:
               key: applicationsetcontroller.enable.progressive.syncs
               name: argocd-cmd-params-cm
               optional: true
+        - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+          valueFrom:
+            configMapKeyRef:
+              key: applicationsetcontroller.enable.new.git.file.globbing
+              name: argocd-cmd-params-cm
+              optional: true
         - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
           valueFrom:
             configMapKeyRef:

manifests/ha/namespace-install.yaml

@@ -1587,6 +1587,12 @@ spec:
               key: applicationsetcontroller.enable.progressive.syncs
               name: argocd-cmd-params-cm
               optional: true
+        - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+          valueFrom:
+            configMapKeyRef:
+              key: applicationsetcontroller.enable.new.git.file.globbing
+              name: argocd-cmd-params-cm
+              optional: true
         - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
           valueFrom:
             configMapKeyRef:

manifests/install.yaml

@@ -17061,6 +17061,12 @@ spec:
               key: applicationsetcontroller.enable.progressive.syncs
               name: argocd-cmd-params-cm
               optional: true
+        - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+          valueFrom:
+            configMapKeyRef:
+              key: applicationsetcontroller.enable.new.git.file.globbing
+              name: argocd-cmd-params-cm
+              optional: true
         - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
           valueFrom:
             configMapKeyRef:

manifests/namespace-install.yaml

@@ -704,6 +704,12 @@ spec:
               key: applicationsetcontroller.enable.progressive.syncs
               name: argocd-cmd-params-cm
               optional: true
+        - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_NEW_GIT_FILE_GLOBBING
+          valueFrom:
+            configMapKeyRef:
+              key: applicationsetcontroller.enable.new.git.file.globbing
+              name: argocd-cmd-params-cm
+              optional: true
         - name: ARGOCD_APPLICATIONSET_CONTROLLER_REPO_SERVER_PLAINTEXT
           valueFrom:
             configMapKeyRef:

mkdocs.yml

@@ -107,6 +107,7 @@ nav:
     - Controlling Resource Modification: operator-manual/applicationset/Controlling-Resource-Modification.md
     - Application Pruning & Resource Deletion: operator-manual/applicationset/Application-Deletion.md
     - Progressive Syncs: operator-manual/applicationset/Progressive-Syncs.md
+    - Git File Generator Globbing: operator-manual/applicationset/Generators-Git-File-Globbing.md
   - Server Configuration Parameters:
     - operator-manual/server-commands/argocd-server.md
     - operator-manual/server-commands/argocd-application-controller.md