fix: support 'project' filter field for backwards-compatibility (#12594)

* fix: support 'project' filter field for backwards-compatibility Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix codegen Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * add upgrade notes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix upgrade notes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
argoproj · Mar 14, 2023 · ccd7f76 · ccd7f76
1 parent c2cb669
commit ccd7f76
Show file tree

Hide file tree

Showing 11 changed files with 362 additions and 170 deletions.
diff --git a/assets/swagger.json b/assets/swagger.json
@@ -271,6 +271,16 @@
             "description": "the application's namespace.",
             "name": "appNamespace",
             "in": "query"
+          },
+          {
+            "type": "array",
+            "items": {
+              "type": "string"
+            },
+            "collectionFormat": "multi",
+            "description": "the project names to restrict returned list applications (legacy name for backwards-compatibility).",
+            "name": "project",
+            "in": "query"
           }
         ],
         "responses": {
@@ -585,6 +595,16 @@
             "description": "the application's namespace.",
             "name": "appNamespace",
             "in": "query"
+          },
+          {
+            "type": "array",
+            "items": {
+              "type": "string"
+            },
+            "collectionFormat": "multi",
+            "description": "the project names to restrict returned list applications (legacy name for backwards-compatibility).",
+            "name": "project",
+            "in": "query"
           }
         ],
         "responses": {
@@ -3507,6 +3527,16 @@
             "description": "the application's namespace.",
             "name": "appNamespace",
             "in": "query"
+          },
+          {
+            "type": "array",
+            "items": {
+              "type": "string"
+            },
+            "collectionFormat": "multi",
+            "description": "the project names to restrict returned list applications (legacy name for backwards-compatibility).",
+            "name": "project",
+            "in": "query"
           }
         ],
         "responses": {

diff --git a/cmpserver/apiclient/plugin.pb.go b/cmpserver/apiclient/plugin.pb.go
diff --git a/docs/operator-manual/upgrading/2.3-2.4.md b/docs/operator-manual/upgrading/2.3-2.4.md
@@ -1,5 +1,26 @@
 # v2.3 to 2.4
 
+## Known Issues
+
+### Broken `project` filter before 2.4.24
+
+Argo CD 2.4.0 introduced a breaking API change, renaming the `project` filter to `projects`.
+
+#### Impact to API clients
+
+A similar issue applies to other API clients which communicate with the Argo CD API server via its REST API. If the
+client uses the `project` field to filter projects, the filter will not be applied. **The failing project filter could 
+have detrimental consequences if, for example, you rely on it to list Applications to be deleted.**
+
+#### Impact to CLI clients
+
+CLI clients older that v2.4.0 rely on client-side filtering and are not impacted by this bug.
+
+#### How to fix the problem
+
+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.
+
 ## KSonnet support is removed
 
 Ksonnet was deprecated in [2019](https://github.com/ksonnet/ksonnet/pull/914/files) and is no longer maintained.

diff --git a/docs/operator-manual/upgrading/2.4-2.5.md b/docs/operator-manual/upgrading/2.4-2.5.md
@@ -1,5 +1,26 @@
 # v2.4 to 2.5
 
+## Known Issues
+
+### Broken `project` filter before 2.5.12
+
+Argo CD 2.4.0 introduced a breaking API change, renaming the `project` filter to `projects`.
+
+#### Impact to API clients
+
+A similar issue applies to other API clients which communicate with the Argo CD API server via its REST API. If the
+client uses the `project` field to filter projects, the filter will not be applied. **The failing project filter could
+have detrimental consequences if, for example, you rely on it to list Applications to be deleted.**
+
+#### Impact to CLI clients
+
+CLI clients older that v2.4.0 rely on client-side filtering and are not impacted by this bug.
+
+#### How to fix the problem
+
+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.
+
 ## 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/pkg/apiclient/application/application.pb.go b/pkg/apiclient/application/application.pb.go
diff --git a/pkg/apis/application/v1alpha1/applicationset_types.go b/pkg/apis/application/v1alpha1/applicationset_types.go
@@ -541,7 +541,7 @@ const (
 // prefix "Info" means informational condition
 type ApplicationSetConditionType string
 
-//ErrorOccurred / ParametersGenerated / TemplateRendered / ResourcesUpToDate
+// ErrorOccurred / ParametersGenerated / TemplateRendered / ResourcesUpToDate
 const (
 	ApplicationSetConditionErrorOccurred       ApplicationSetConditionType = "ErrorOccurred"
 	ApplicationSetConditionParametersGenerated ApplicationSetConditionType = "ParametersGenerated"

diff --git a/pkg/client/clientset/versioned/fake/register.go b/pkg/client/clientset/versioned/fake/register.go
diff --git a/pkg/client/clientset/versioned/scheme/register.go b/pkg/client/clientset/versioned/scheme/register.go
diff --git a/server/application/application.go b/server/application/application.go
@@ -148,7 +148,7 @@ func (s *Server) List(ctx context.Context, q *application.ApplicationQuery) (*ap
 	}
 	newItems := make([]appv1.Application, 0)
 	for _, a := range apps {
-		// Skip any application that is neither in the conrol plane's namespace
+		// Skip any application that is neither in the control plane's namespace
 		// nor in the list of enabled namespaces.
 		if a.Namespace != s.ns && !glob.MatchStringInList(s.enabledNamespaces, a.Namespace, false) {
 			continue
@@ -165,8 +165,8 @@ func (s *Server) List(ctx context.Context, q *application.ApplicationQuery) (*ap
 		}
 	}
 
-	// Filter applications by name
-	newItems = argoutil.FilterByProjects(newItems, q.Projects)
+	// Filter applications by projects
+	newItems = argoutil.FilterByProjects(newItems, getProjectsFromApplicationQuery(*q))
 
 	// Filter applications by source repo URL
 	newItems = argoutil.FilterByRepo(newItems, q.GetRepo())
@@ -1008,8 +1008,8 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati
 		logCtx = logCtx.WithField("application", *q.Name)
 	}
 	projects := map[string]bool{}
-	for i := range q.Projects {
-		projects[q.Projects[i]] = true
+	for _, project := range getProjectsFromApplicationQuery(*q) {
+		projects[project] = true
 	}
 	claims := ws.Context().Value("claims")
 	selector, err := labels.Parse(q.GetSelector())
@@ -2292,3 +2292,12 @@ func (s *Server) appNamespaceOrDefault(appNs string) string {
 func (s *Server) isNamespaceEnabled(namespace string) bool {
 	return security.IsNamespaceEnabled(namespace, s.ns, s.enabledNamespaces)
 }
+
+// getProjectFromApplicationQuery gets the project names from a query. If the legacy "project" field was specified, use
+// that. Otherwise, use the newer "projects" field.
+func getProjectsFromApplicationQuery(q application.ApplicationQuery) []string {
+	if q.Project != nil {
+		return q.Project
+	}
+	return q.Projects
+}
diff --git a/server/application/application.proto b/server/application/application.proto
@@ -29,6 +29,8 @@ message ApplicationQuery {
 	optional string repo = 6;
 	// the application's namespace
 	optional string appNamespace = 7;
+	// the project names to restrict returned list applications (legacy name for backwards-compatibility)
+	repeated string project = 8;
 }
 
 message NodeQuery {

diff --git a/server/application/application_test.go b/server/application/application_test.go
@@ -401,6 +401,57 @@ func testListAppsWithLabels(t *testing.T, appQuery application.ApplicationQuery,
 	}
 }
 
+func TestListAppWithProjects(t *testing.T) {
+	appServer := newTestAppServer(newTestApp(func(app *appsv1.Application) {
+		app.Name = "App1"
+		app.Spec.Project = "test-project1"
+	}), newTestApp(func(app *appsv1.Application) {
+		app.Name = "App2"
+		app.Spec.Project = "test-project2"
+	}), newTestApp(func(app *appsv1.Application) {
+		app.Name = "App3"
+		app.Spec.Project = "test-project3"
+	}))
+
+	t.Run("List all apps", func(t *testing.T) {
+		appQuery := application.ApplicationQuery{}
+		appList, err := appServer.List(context.Background(), &appQuery)
+		assert.NoError(t, err)
+		assert.Len(t, appList.Items, 3)
+	})
+
+	t.Run("List apps with projects filter set", func(t *testing.T) {
+		appQuery := application.ApplicationQuery{Projects: []string{"test-project1"}}
+		appList, err := appServer.List(context.Background(), &appQuery)
+		assert.NoError(t, err)
+		assert.Len(t, appList.Items, 1)
+		for _, app := range appList.Items {
+			assert.Equal(t, "test-project1", app.Spec.Project)
+		}
+	})
+
+	t.Run("List apps with project filter set (legacy field)", func(t *testing.T) {
+		appQuery := application.ApplicationQuery{Project: []string{"test-project1"}}
+		appList, err := appServer.List(context.Background(), &appQuery)
+		assert.NoError(t, err)
+		assert.Len(t, appList.Items, 1)
+		for _, app := range appList.Items {
+			assert.Equal(t, "test-project1", app.Spec.Project)
+		}
+	})
+
+	t.Run("List apps with both projects and project filter set", func(t *testing.T) {
+		// If the older field is present, we should use it instead of the newer field.
+		appQuery := application.ApplicationQuery{Project: []string{"test-project1"}, Projects: []string{"test-project2"}}
+		appList, err := appServer.List(context.Background(), &appQuery)
+		assert.NoError(t, err)
+		assert.Len(t, appList.Items, 1)
+		for _, app := range appList.Items {
+			assert.Equal(t, "test-project1", app.Spec.Project)
+		}
+	})
+}
+
 func TestListApps(t *testing.T) {
 	appServer := newTestAppServer(newTestApp(func(app *appsv1.Application) {
 		app.Name = "bcd"