From 4c4c624c2c1b8415ded731a413b07aaea17e028b Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Wed, 15 Mar 2023 19:50:37 -0400 Subject: [PATCH] fix(appset): git files generator in matrix generator produces no params (#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> --- .../generator_spec_processor_test.go | 5 +- applicationset/generators/git.go | 4 +- applicationset/generators/git_test.go | 53 ++++---------- applicationset/generators/matrix_test.go | 71 +++++++++++++++++++ applicationset/utils/test/testutils.go | 34 +++++++++ docs/operator-manual/upgrading/2.4-2.5.md | 31 ++++++++ docs/operator-manual/upgrading/2.5-2.6.md | 31 ++++++++ 7 files changed, 185 insertions(+), 44 deletions(-) create mode 100644 applicationset/utils/test/testutils.go diff --git a/applicationset/generators/generator_spec_processor_test.go b/applicationset/generators/generator_spec_processor_test.go index 7822a87aebde9..f9d8fb415292a 100644 --- a/applicationset/generators/generator_spec_processor_test.go +++ b/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 } diff --git a/applicationset/generators/git.go b/applicationset/generators/git.go index 750ce2f057e9f..911dc49391f18 100644 --- a/applicationset/generators/git.go +++ b/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 diff --git a/applicationset/generators/git_test.go b/applicationset/generators/git_test.go index 96cfe08404d81..e428cfd3141e3 100644 --- a/applicationset/generators/git_test.go +++ b/applicationset/generators/git_test.go @@ -1,7 +1,6 @@ package generators import ( - "context" "fmt" "testing" @@ -9,6 +8,7 @@ import ( "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) }) } } diff --git a/applicationset/generators/matrix_test.go b/applicationset/generators/matrix_test.go index 1d79c452f6dce..d1b2d272f6d79 100644 --- a/applicationset/generators/matrix_test.go +++ b/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) +} diff --git a/applicationset/utils/test/testutils.go b/applicationset/utils/test/testutils.go new file mode 100644 index 0000000000000..896ac75e274d3 --- /dev/null +++ b/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) +} diff --git a/docs/operator-manual/upgrading/2.4-2.5.md b/docs/operator-manual/upgrading/2.4-2.5.md index 7455a6cec4152..03d7e49fd2caf 100644 --- a/docs/operator-manual/upgrading/2.4-2.5.md +++ b/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). diff --git a/docs/operator-manual/upgrading/2.5-2.6.md b/docs/operator-manual/upgrading/2.5-2.6.md index a85be67e31fb9..49bfc52f99057 100644 --- a/docs/operator-manual/upgrading/2.5-2.6.md +++ b/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.