feat(appset): ignoreApplicationDifferences (argoproj#9101) (argoproj#…

…14743) * feat(appset): ignoreDifferences (argoproj#9101) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * better error messages Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * do better Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * docs Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * more tests, update docs Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * e2e test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * expect auto-added fields Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * correct label Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * better Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * remove line that was reverted Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * Update docs/operator-manual/applicationset.yaml Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * remove line that mysteriously causes applicationset/utils unit tests to fail Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * login to fix test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * maybe this will work, who knows Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * burn it all down Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * works on my machine Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
ymktmk · Oct 29, 2023 · 9cc33cb · 9cc33cb
1 parent 8176ebf
commit 9cc33cb
Show file tree

Hide file tree

Showing 19 changed files with 1,855 additions and 1,037 deletions.
diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go
@@ -16,6 +16,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"time"
@@ -24,6 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierr "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -44,6 +46,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/applicationset/generators"
 	"github.com/argoproj/argo-cd/v2/applicationset/utils"
 	"github.com/argoproj/argo-cd/v2/common"
+	argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff"
 	"github.com/argoproj/argo-cd/v2/util/db"
 	"github.com/argoproj/argo-cd/v2/util/glob"
 
@@ -683,6 +686,14 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
 
 			found.ObjectMeta.Finalizers = generatedApp.Finalizers
 			found.ObjectMeta.Labels = generatedApp.Labels
+
+			if found != nil && len(found.Spec.IgnoreDifferences) > 0 {
+				err := applyIgnoreDifferences(applicationSet.Spec.IgnoreApplicationDifferences, found, generatedApp)
+				if err != nil {
+					return fmt.Errorf("failed to apply ignore differences: %w", err)
+				}
+			}
+
 			return controllerutil.SetControllerReference(&applicationSet, found, r.Scheme)
 		})
 
@@ -700,6 +711,54 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context,
 	return firstError
 }
 
+// applyIgnoreDifferences applies the ignore differences rules to the found application. It modifies the found application in place.
+func applyIgnoreDifferences(applicationSetIgnoreDifferences argov1alpha1.ApplicationSetIgnoreDifferences, found *argov1alpha1.Application, generatedApp argov1alpha1.Application) error {
+	diffConfig, err := argodiff.NewDiffConfigBuilder().
+		WithDiffSettings(applicationSetIgnoreDifferences.ToApplicationIgnoreDifferences(), nil, false).
+		WithNoCache().
+		Build()
+	if err != nil {
+		return fmt.Errorf("failed to build diff config: %w", err)
+	}
+	unstructuredFound, err := appToUnstructured(found)
+	if err != nil {
+		return fmt.Errorf("failed to convert found application to unstructured: %w", err)
+	}
+	unstructuredGenerated, err := appToUnstructured(&generatedApp)
+	if err != nil {
+		return fmt.Errorf("failed to convert found application to unstructured: %w", err)
+	}
+	result, err := argodiff.Normalize([]*unstructured.Unstructured{unstructuredFound}, []*unstructured.Unstructured{unstructuredGenerated}, diffConfig)
+	if err != nil {
+		return fmt.Errorf("failed to normalize application spec: %w", err)
+	}
+	if len(result.Targets) != 1 {
+		return fmt.Errorf("expected 1 normalized application, got %d", len(result.Targets))
+	}
+	jsonNormalized, err := json.Marshal(result.Targets[0].Object)
+	if err != nil {
+		return fmt.Errorf("failed to marshal normalized app to json: %w", err)
+	}
+	err = json.Unmarshal(jsonNormalized, &found)
+	if err != nil {
+		return fmt.Errorf("failed to unmarshal normalized app json to structured app: %w", err)
+	}
+	// Prohibit jq queries from mutating silly things.
+	found.TypeMeta = generatedApp.TypeMeta
+	found.Name = generatedApp.Name
+	found.Namespace = generatedApp.Namespace
+	found.Operation = generatedApp.Operation
+	return nil
+}
+
+func appToUnstructured(app *argov1alpha1.Application) (*unstructured.Unstructured, error) {
+	u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(app)
+	if err != nil {
+		return nil, fmt.Errorf("failed to convert app object to unstructured: %w", err)
+	}
+	return &unstructured.Unstructured{Object: u}, nil
+}
+
 // createInCluster will filter from the desiredApplications only the application that needs to be created
 // Then it will call createOrUpdateInCluster to do the actual create
 func (r *ApplicationSetReconciler) createInCluster(ctx context.Context, applicationSet argov1alpha1.ApplicationSet, desiredApplications []argov1alpha1.Application) error {

diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go
@@ -12,6 +12,8 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v2"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -5726,3 +5728,173 @@ func TestOwnsHandler(t *testing.T) {
 		})
 	}
 }
+
+func Test_applyIgnoreDifferences(t *testing.T) {
+	appMeta := metav1.TypeMeta{
+		APIVersion: v1alpha1.ApplicationSchemaGroupVersionKind.GroupVersion().String(),
+		Kind:       v1alpha1.ApplicationSchemaGroupVersionKind.Kind,
+	}
+	testCases := []struct {
+		name              string
+		ignoreDifferences v1alpha1.ApplicationSetIgnoreDifferences
+		foundApp          string
+		generatedApp      string
+		expectedApp       string
+	}{
+		{
+			name: "empty ignoreDifferences",
+			foundApp: `
+spec: {}`,
+			generatedApp: `
+spec: {}`,
+			expectedApp: `
+spec: {}`,
+		},
+		{
+			// For this use case: https://github.com/argoproj/argo-cd/issues/9101#issuecomment-1191138278
+			name: "ignore target revision with jq",
+			ignoreDifferences: v1alpha1.ApplicationSetIgnoreDifferences{
+				{JQPathExpressions: []string{".spec.source.targetRevision"}},
+			},
+			foundApp: `
+spec:
+  source:
+    targetRevision: foo`,
+			generatedApp: `
+spec:
+  source:
+    targetRevision: bar`,
+			expectedApp: `
+spec:
+  source:
+    targetRevision: foo`,
+		},
+		{
+			// For this use case: https://github.com/argoproj/argo-cd/issues/9101#issuecomment-1103593714
+			name: "ignore helm parameter with jq",
+			ignoreDifferences: v1alpha1.ApplicationSetIgnoreDifferences{
+				{JQPathExpressions: []string{`.spec.source.helm.parameters | select(.name == "image.tag")`}},
+			},
+			foundApp: `
+spec:
+  source:
+    helm:
+      parameters:
+      - name: image.tag
+        value: test
+      - name: another
+        value: value`,
+			generatedApp: `
+spec:
+  source:
+    helm:
+      parameters:
+      - name: image.tag
+        value: v1.0.0
+      - name: another
+        value: value`,
+			expectedApp: `
+spec:
+  source:
+    helm:
+      parameters:
+      - name: image.tag
+        value: test
+      - name: another
+        value: value`,
+		},
+		{
+			// For this use case: https://github.com/argoproj/argo-cd/issues/9101#issuecomment-1191138278
+			name: "ignore auto-sync with jq",
+			ignoreDifferences: v1alpha1.ApplicationSetIgnoreDifferences{
+				{JQPathExpressions: []string{".spec.syncPolicy.automated"}},
+			},
+			foundApp: `
+spec:
+  syncPolicy:
+    retry:
+      limit: 5`,
+			generatedApp: `
+spec:
+  syncPolicy:
+    automated:
+      selfHeal: true
+    retry:
+      limit: 5`,
+			expectedApp: `
+spec:
+  syncPolicy:
+    retry:
+      limit: 5`,
+		},
+		{
+			// For this use case: https://github.com/argoproj/argo-cd/issues/9101#issuecomment-1420656537
+			name: "ignore a one-off annotation with jq",
+			ignoreDifferences: v1alpha1.ApplicationSetIgnoreDifferences{
+				{JQPathExpressions: []string{`.metadata.annotations | select(.["foo.bar"] == "baz")`}},
+			},
+			foundApp: `
+metadata:
+  annotations:
+    foo.bar: baz
+    some.other: annotation`,
+			generatedApp: `
+metadata:
+  annotations:
+    some.other: annotation`,
+			expectedApp: `
+metadata:
+  annotations:
+    foo.bar: baz
+    some.other: annotation`,
+		},
+		{
+			// For this use case: https://github.com/argoproj/argo-cd/issues/9101#issuecomment-1515672638
+			name: "ignore the source.plugin field with a json pointer",
+			ignoreDifferences: v1alpha1.ApplicationSetIgnoreDifferences{
+				{JSONPointers: []string{"/spec/source/plugin"}},
+			},
+			foundApp: `
+spec:
+  source:
+    plugin:
+      parameters:
+      - name: url
+        string: https://example.com`,
+			generatedApp: `
+spec:
+  source:
+    plugin:
+      parameters:
+      - name: url
+        string: https://example.com/wrong`,
+			expectedApp: `
+spec:
+  source:
+    plugin:
+      parameters:
+      - name: url
+        string: https://example.com`,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			foundApp := v1alpha1.Application{TypeMeta: appMeta}
+			err := yaml.Unmarshal([]byte(tc.foundApp), &foundApp)
+			require.NoError(t, err, tc.foundApp)
+			generatedApp := v1alpha1.Application{TypeMeta: appMeta}
+			err = yaml.Unmarshal([]byte(tc.generatedApp), &generatedApp)
+			require.NoError(t, err, tc.generatedApp)
+			err = applyIgnoreDifferences(tc.ignoreDifferences, &foundApp, generatedApp)
+			require.NoError(t, err)
+			jsonFound, err := json.Marshal(tc.foundApp)
+			require.NoError(t, err)
+			jsonExpected, err := json.Marshal(tc.expectedApp)
+			require.NoError(t, err)
+			assert.Equal(t, string(jsonExpected), string(jsonFound))
+		})
+	}
+}
diff --git a/applicationset/utils/clusterUtils_test.go b/applicationset/utils/clusterUtils_test.go
@@ -13,7 +13,6 @@ import (
 	kubetesting "k8s.io/client-go/testing"
 
 	argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-	"github.com/argoproj/argo-cd/v2/test/e2e/fixture/applicationsets/utils"
 )
 
 const (
@@ -69,7 +68,7 @@ func createClusterSecret(secretName string, clusterName string, clusterServer st
 	secret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      secretName,
-			Namespace: utils.ArgoCDNamespace,
+			Namespace: fakeNamespace,
 			Labels: map[string]string{
 				ArgoCDSecretTypeLabel: ArgoCDSecretTypeCluster,
 			},
@@ -111,7 +110,7 @@ func TestValidateDestination(t *testing.T) {
 		objects = append(objects, secret)
 		kubeclientset := fake.NewSimpleClientset(objects...)
 
-		appCond := ValidateDestination(context.Background(), &dest, kubeclientset, utils.ArgoCDNamespace)
+		appCond := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
 		assert.Nil(t, appCond)
 		assert.Equal(t, "https://127.0.0.1:6443", dest.Server)
 		assert.True(t, dest.IsServerInferred())
@@ -124,7 +123,7 @@ func TestValidateDestination(t *testing.T) {
 			Namespace: "default",
 		}
 
-		err := ValidateDestination(context.Background(), &dest, nil, utils.ArgoCDNamespace)
+		err := ValidateDestination(context.Background(), &dest, nil, fakeNamespace)
 		assert.Equal(t, "application destination can't have both name and server defined: minikube https://127.0.0.1:6443", err.Error())
 		assert.False(t, dest.IsServerInferred())
 	})
@@ -139,7 +138,7 @@ func TestValidateDestination(t *testing.T) {
 			return true, nil, fmt.Errorf("an error occurred")
 		})
 
-		err := ValidateDestination(context.Background(), &dest, kubeclientset, utils.ArgoCDNamespace)
+		err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
 		assert.Equal(t, "unable to find destination server: an error occurred", err.Error())
 		assert.False(t, dest.IsServerInferred())
 	})
@@ -154,7 +153,7 @@ func TestValidateDestination(t *testing.T) {
 		objects = append(objects, secret)
 		kubeclientset := fake.NewSimpleClientset(objects...)
 
-		err := ValidateDestination(context.Background(), &dest, kubeclientset, utils.ArgoCDNamespace)
+		err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
 		assert.Equal(t, "unable to find destination server: there are no clusters with this name: minikube", err.Error())
 		assert.False(t, dest.IsServerInferred())
 	})
@@ -171,7 +170,7 @@ func TestValidateDestination(t *testing.T) {
 		objects = append(objects, secret, secret2)
 		kubeclientset := fake.NewSimpleClientset(objects...)
 
-		err := ValidateDestination(context.Background(), &dest, kubeclientset, utils.ArgoCDNamespace)
+		err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
 		assert.Equal(t, "unable to find destination server: there are 2 clusters with the same name: [https://127.0.0.1:2443 https://127.0.0.1:8443]", err.Error())
 		assert.False(t, dest.IsServerInferred())
 	})

diff --git a/assets/swagger.json b/assets/swagger.json
@@ -6055,6 +6055,30 @@
         }
       }
     },
+    "v1alpha1ApplicationSetResourceIgnoreDifferences": {
+      "description": "ApplicationSetResourceIgnoreDifferences configures how the ApplicationSet controller will ignore differences in live\napplications when applying changes from generated applications.",
+      "type": "object",
+      "properties": {
+        "jqPathExpressions": {
+          "description": "JQPathExpressions is a list of JQ path expressions to fields to ignore differences for.",
+          "type": "array",
+          "items": {
+            "type": "string"
+          }
+        },
+        "jsonPointers": {
+          "description": "JSONPointers is a list of JSON pointers to fields to ignore differences for.",
+          "type": "array",
+          "items": {
+            "type": "string"
+          }
+        },
+        "name": {
+          "description": "Name is the name of the application to ignore differences for. If not specified, the rule applies to all applications.",
+          "type": "string"
+        }
+      }
+    },
     "v1alpha1ApplicationSetRolloutStep": {
       "type": "object",
       "properties": {
@@ -6103,6 +6127,12 @@
             "type": "string"
           }
         },
+        "ignoreApplicationDifferences": {
+          "type": "array",
+          "items": {
+            "$ref": "#/definitions/v1alpha1ApplicationSetResourceIgnoreDifferences"
+          }
+        },
         "preservedFields": {
           "$ref": "#/definitions/v1alpha1ApplicationPreservedFields"
         },

diff --git a/docs/operator-manual/applicationset.yaml b/docs/operator-manual/applicationset.yaml
@@ -26,3 +26,13 @@ spec:
     preserveResourcesOnDeletion: false
   # Alpha feature to determine the order in which ApplicationSet applies changes.
   strategy:
+  # This field lets you define fields which should be ignored when applying Application resources. This is helpful if you
+  # want to use ApplicationSets to create apps, but also want to allow users to modify those apps without having their
+  # changes overwritten by the ApplicationSet.
+  ignoreApplicationDifferences:
+  - jsonPointers:
+    - /spec/source/targetRevision
+  - name: some-app
+    jqExpressions:
+    - .spec.source.helm.values
+