fix(appset): git files generator in matrix generator produces no para…

…ms (argoproj#12881)

* fix(appset): git files generator in matrix generator produces no params

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* upgrade notes

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* fix lint

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

applicationset/generators/generator_spec_processor_test.go

@@ -10,6 +10,7 @@ import (
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	testutils "github.com/argoproj/argo-cd/v2/applicationset/utils/test"
 	argov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 
 	"github.com/stretchr/testify/mock"
@@ -160,8 +161,8 @@ func getMockClusterGenerator() Generator {
 }
 
 func getMockGitGenerator() Generator {
-	argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
-	argoCDServiceMock.mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return([]string{"app1", "app2", "app_3", "p1/app4"}, nil)
+	argoCDServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
+	argoCDServiceMock.Mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return([]string{"app1", "app2", "app_3", "p1/app4"}, nil)
 	var gitGenerator = NewGitGenerator(argoCDServiceMock)
 	return gitGenerator
 }

applicationset/generators/git.go

@@ -58,9 +58,9 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
 
 	var err error
 	var res []map[string]interface{}
-	if appSetGenerator.Git.Directories != nil {
+	if len(appSetGenerator.Git.Directories) != 0 {
 		res, err = g.generateParamsForGitDirectories(appSetGenerator, appSet.Spec.GoTemplate)
-	} else if appSetGenerator.Git.Files != nil {
+	} else if len(appSetGenerator.Git.Files) != 0 {
 		res, err = g.generateParamsForGitFiles(appSetGenerator, appSet.Spec.GoTemplate)
 	} else {
 		return nil, EmptyAppSetGeneratorError

applicationset/generators/git_test.go

@@ -1,14 +1,14 @@
 package generators
 
 import (
-	"context"
 	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	testutils "github.com/argoproj/argo-cd/v2/applicationset/utils/test"
 	argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 )
 
@@ -20,33 +20,6 @@ import (
 // 	return io.NewCloser(func() error { return nil }), c.RepoServerServiceClient, nil
 // }
 
-type argoCDServiceMock struct {
-	mock *mock.Mock
-}
-
-func (a argoCDServiceMock) GetApps(ctx context.Context, repoURL string, revision string) ([]string, error) {
-	args := a.mock.Called(ctx, repoURL, revision)
-
-	return args.Get(0).([]string), args.Error(1)
-}
-
-func (a argoCDServiceMock) GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error) {
-	args := a.mock.Called(ctx, repoURL, revision, pattern)
-
-	return args.Get(0).(map[string][]byte), args.Error(1)
-}
-
-func (a argoCDServiceMock) GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error) {
-	args := a.mock.Called(ctx, repoURL, revision, path)
-
-	return args.Get(0).([]byte), args.Error(1)
-}
-
-func (a argoCDServiceMock) GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error) {
-	args := a.mock.Called(ctx, repoURL, revision)
-	return args.Get(0).([]string), args.Error(1)
-}
-
 func Test_generateParamsFromGitFile(t *testing.T) {
 	params, err := (*GitGenerator)(nil).generateParamsFromGitFile("path/dir/file_name.yaml", []byte(`
 foo:
@@ -271,9 +244,9 @@ func TestGitGenerateParamsFromDirectories(t *testing.T) {
 		t.Run(testCaseCopy.name, func(t *testing.T) {
 			t.Parallel()
 
-			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
+			argoCDServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
 
-			argoCDServiceMock.mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
+			argoCDServiceMock.Mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
 			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
@@ -301,7 +274,7 @@ func TestGitGenerateParamsFromDirectories(t *testing.T) {
 				assert.Equal(t, testCaseCopy.expected, got)
 			}
 
-			argoCDServiceMock.mock.AssertExpectations(t)
+			argoCDServiceMock.Mock.AssertExpectations(t)
 		})
 	}
 }
@@ -566,9 +539,9 @@ func TestGitGenerateParamsFromDirectoriesGoTemplate(t *testing.T) {
 		t.Run(testCaseCopy.name, func(t *testing.T) {
 			t.Parallel()
 
-			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
+			argoCDServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
 
-			argoCDServiceMock.mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
+			argoCDServiceMock.Mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
 			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
@@ -597,7 +570,7 @@ func TestGitGenerateParamsFromDirectoriesGoTemplate(t *testing.T) {
 				assert.Equal(t, testCaseCopy.expected, got)
 			}
 
-			argoCDServiceMock.mock.AssertExpectations(t)
+			argoCDServiceMock.Mock.AssertExpectations(t)
 		})
 	}
 
@@ -857,8 +830,8 @@ cluster:
 		t.Run(testCaseCopy.name, func(t *testing.T) {
 			t.Parallel()
 
-			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
-			argoCDServiceMock.mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+			argoCDServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
+			argoCDServiceMock.Mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 				Return(testCaseCopy.repoFileContents, testCaseCopy.repoPathsError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
@@ -887,7 +860,7 @@ cluster:
 				assert.ElementsMatch(t, testCaseCopy.expected, got)
 			}
 
-			argoCDServiceMock.mock.AssertExpectations(t)
+			argoCDServiceMock.Mock.AssertExpectations(t)
 		})
 	}
 }
@@ -1206,8 +1179,8 @@ cluster:
 		t.Run(testCaseCopy.name, func(t *testing.T) {
 			t.Parallel()
 
-			argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
-			argoCDServiceMock.mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+			argoCDServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
+			argoCDServiceMock.Mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 				Return(testCaseCopy.repoFileContents, testCaseCopy.repoPathsError)
 
 			var gitGenerator = NewGitGenerator(argoCDServiceMock)
@@ -1237,7 +1210,7 @@ cluster:
 				assert.ElementsMatch(t, testCaseCopy.expected, got)
 			}
 
-			argoCDServiceMock.mock.AssertExpectations(t)
+			argoCDServiceMock.Mock.AssertExpectations(t)
 		})
 	}
 }

applicationset/generators/matrix_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -16,6 +17,7 @@ import (
 	"github.com/stretchr/testify/mock"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
+	testutils "github.com/argoproj/argo-cd/v2/applicationset/utils/test"
 	argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 )
 
@@ -857,3 +859,72 @@ func (g *generatorMock) GetRequeueAfter(appSetGenerator *argoprojiov1alpha1.Appl
 	return args.Get(0).(time.Duration)
 
 }
+
+func TestGitGenerator_GenerateParams_list_x_git_matrix_generator(t *testing.T) {
+	// Given a matrix generator over a list generator and a git files generator, the nested git files generator should
+	// be treated as a files generator, and it should produce parameters.
+
+	// This tests for a specific bug where a nested git files generator was being treated as a directory generator. This
+	// happened because, when the matrix generator was being processed, the nested git files generator was being
+	// interpolated by the deeplyReplace function. That function cannot differentiate between a nil slice and an empty
+	// slice. So it was replacing the `Directories` field with an empty slice, which the ApplicationSet controller
+	// interpreted as meaning this was a directory generator, not a files generator.
+
+	// Now instead of checking for nil, we check whether the field is a non-empty slice. This test prevents a regression
+	// of that bug.
+
+	listGeneratorMock := &generatorMock{}
+	listGeneratorMock.On("GenerateParams", mock.AnythingOfType("*v1alpha1.ApplicationSetGenerator"), mock.AnythingOfType("*v1alpha1.ApplicationSet")).Return([]map[string]interface{}{
+		{"some": "value"},
+	}, nil)
+	listGeneratorMock.On("GetTemplate", mock.AnythingOfType("*v1alpha1.ApplicationSetGenerator")).Return(&argoprojiov1alpha1.ApplicationSetTemplate{})
+
+	gitGeneratorSpec := &argoprojiov1alpha1.GitGenerator{
+		RepoURL: "https://git.example.com",
+		Files: []argoprojiov1alpha1.GitFileGeneratorItem{
+			{Path: "some/path.json"},
+		},
+	}
+
+	repoServiceMock := testutils.ArgoCDServiceMock{Mock: &mock.Mock{}}
+	repoServiceMock.Mock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string][]byte{
+		"some/path.json": []byte("test: content"),
+	}, nil)
+	gitGenerator := NewGitGenerator(repoServiceMock)
+
+	matrixGenerator := NewMatrixGenerator(map[string]Generator{
+		"List": listGeneratorMock,
+		"Git":  gitGenerator,
+	})
+
+	matrixGeneratorSpec := &argoprojiov1alpha1.MatrixGenerator{
+		Generators: []argoprojiov1alpha1.ApplicationSetNestedGenerator{
+			{
+				List: &argoprojiov1alpha1.ListGenerator{
+					Elements: []apiextensionsv1.JSON{
+						{
+							Raw: []byte(`{"some": "value"}`),
+						},
+					},
+				},
+			},
+			{
+				Git: gitGeneratorSpec,
+			},
+		},
+	}
+	params, err := matrixGenerator.GenerateParams(&argoprojiov1alpha1.ApplicationSetGenerator{
+		Matrix: matrixGeneratorSpec,
+	}, &argoprojiov1alpha1.ApplicationSet{})
+	require.NoError(t, err)
+	assert.Equal(t, []map[string]interface{}{{
+		"path":                    "some",
+		"path.basename":           "some",
+		"path.basenameNormalized": "some",
+		"path.filename":           "path.json",
+		"path.filenameNormalized": "path.json",
+		"path[0]":                 "some",
+		"some":                    "value",
+		"test":                    "content",
+	}}, params)
+}

applicationset/utils/test/testutils.go

@@ -0,0 +1,34 @@
+package test
+
+import (
+	"context"
+
+	"github.com/stretchr/testify/mock"
+)
+
+type ArgoCDServiceMock struct {
+	Mock *mock.Mock
+}
+
+func (a ArgoCDServiceMock) GetApps(ctx context.Context, repoURL string, revision string) ([]string, error) {
+	args := a.Mock.Called(ctx, repoURL, revision)
+
+	return args.Get(0).([]string), args.Error(1)
+}
+
+func (a ArgoCDServiceMock) GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error) {
+	args := a.Mock.Called(ctx, repoURL, revision, pattern)
+
+	return args.Get(0).(map[string][]byte), args.Error(1)
+}
+
+func (a ArgoCDServiceMock) GetFileContent(ctx context.Context, repoURL string, revision string, path string) ([]byte, error) {
+	args := a.Mock.Called(ctx, repoURL, revision, path)
+
+	return args.Get(0).([]byte), args.Error(1)
+}
+
+func (a ArgoCDServiceMock) GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error) {
+	args := a.Mock.Called(ctx, repoURL, revision)
+	return args.Get(0).([]string), args.Error(1)
+}

docs/operator-manual/upgrading/2.4-2.5.md

@@ -21,6 +21,37 @@ CLI clients older that v2.4.0 rely on client-side filtering and are not impacted
 Upgrade to Argo CD >=2.4.24, >=2.5.12, or >=2.6.3. This version of Argo CD will accept both `project` and `projects` as
 valid filters.
 
+### Broken matrix-nested git files generator in 2.5.14
+
+Argo CD 2.5.14 introduced a bug in the matrix-nested git files generator. The bug only applies when the git files
+generator is the second generator nested under a matrix. For example:
+
+```yaml
+apiVersion: argoproj.io/v1alpha1
+kind: ApplicationSet
+metadata:
+  name: guestbook
+spec:
+  generators:
+  - matrix:
+      generators:
+        - clusters: {}
+        - git:
+            repoURL: https://git.example.com/org/repo.git
+            revision: HEAD
+            files:
+              - path: "defaults/*.yaml"
+  template:
+    # ...
+```
+
+The nested git files generator will produce no parameters, causing the matrix generator to also produce no parameters.
+This will cause the ApplicationSet to produce no Applications. If the ApplicationSet controller is
+[configured with the ability to delete applications](https://argo-cd.readthedocs.io/en/latest/operator-manual/applicationset/Controlling-Resource-Modification/),
+it will delete all Applications which were previously created by the ApplicationSet.
+
+To avoid this issue, upgrade directly to >=2.5.15 or >= 2.6.6.
+
 ## Configure RBAC to account for new `applicationsets` resource
 
 2.5 introduces a new `applicationsets` [RBAC resource](https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#rbac-resources-and-actions).

docs/operator-manual/upgrading/2.5-2.6.md

@@ -21,6 +21,37 @@ CLI clients older that v2.4.0 rely on client-side filtering and are not impacted
 Upgrade to Argo CD >=2.4.24, >=2.5.12, or >=2.6.3. This version of Argo CD will accept both `project` and `projects` as
 valid filters.
 
+### Broken matrix-nested git files generator in 2.6.5
+
+Argo CD 2.6.5 introduced a bug in the matrix-nested git files generator. The bug only applies when the git files 
+generator is the second generator nested under a matrix. For example:
+
+```yaml
+apiVersion: argoproj.io/v1alpha1
+kind: ApplicationSet
+metadata:
+  name: guestbook
+spec:
+  generators:
+  - matrix:
+      generators:
+        - clusters: {}
+        - git:
+            repoURL: https://git.example.com/org/repo.git
+            revision: HEAD
+            files:
+              - path: "defaults/*.yaml"
+  template:
+    # ...
+```
+
+The nested git files generator will produce no parameters, causing the matrix generator to also produce no parameters.
+This will cause the ApplicationSet to produce no Applications. If the ApplicationSet controller is 
+[configured with the ability to delete applications](https://argo-cd.readthedocs.io/en/latest/operator-manual/applicationset/Controlling-Resource-Modification/), 
+it will delete all Applications which were previously created by the ApplicationSet.
+
+To avoid this issue, upgrade directly to >=2.5.15 or >= 2.6.6.
+
 ## ApplicationSets: `^` behavior change in Sprig's semver functions
 Argo CD 2.5 introduced [Go templating in ApplicationSets](https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/GoTemplate/). Go templates have access to the Sprig function library.