fix(app): Fix patch creation for Applications with ValuesObject fields #15126

USERS.md

@@ -38,6 +38,7 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [Beez Innovation Labs](https://www.beezlabs.com/)
 1. [Bedag Informatik AG](https://www.bedag.ch/)
 1. [Beleza Na Web](https://www.belezanaweb.com.br/)
+1. [Believable Bots](https://believablebots.io/)
 1. [BigPanda](https://bigpanda.io)
 1. [BioBox Analytics](https://biobox.io)
 1. [BMW Group](https://www.bmwgroup.com/)

controller/appcontroller.go

@@ -27,6 +27,7 @@ import (
 	"golang.org/x/sync/semaphore"
 	v1 "k8s.io/api/core/v1"
 	apierr "k8s.io/apimachinery/pkg/api/errors"
+	kubeerrors "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/labels"
@@ -51,17 +52,14 @@ import (
 	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned"
 	"github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions/application/v1alpha1"
 	applisters "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
+	"github.com/argoproj/argo-cd/v2/pkg/ratelimiter"
 	"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	"github.com/argoproj/argo-cd/v2/util/argo"
 	argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff"
 	"github.com/argoproj/argo-cd/v2/util/argo/normalizers"
-	"github.com/argoproj/argo-cd/v2/util/env"
-
-	kubeerrors "k8s.io/apimachinery/pkg/api/errors"
-
-	"github.com/argoproj/argo-cd/v2/pkg/ratelimiter"
 	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
 	"github.com/argoproj/argo-cd/v2/util/db"
+	"github.com/argoproj/argo-cd/v2/util/env"
 	"github.com/argoproj/argo-cd/v2/util/errors"
 	"github.com/argoproj/argo-cd/v2/util/glob"
 	"github.com/argoproj/argo-cd/v2/util/helm"
@@ -1749,7 +1747,7 @@ func (ctrl *ApplicationController) normalizeApplication(orig, app *appv1.Applica
 	logCtx := log.WithFields(log.Fields{"application": app.QualifiedName()})
 	app.Spec = *argo.NormalizeApplicationSpec(&app.Spec)
 
-	patch, modified, err := diff.CreateTwoWayMergePatch(orig, app, appv1.Application{})
+	patch, modified, err := CreateAppMergePatch(orig, app)
 
 	if err != nil {
 		logCtx.Errorf("error constructing app spec patch: %v", err)
@@ -1782,9 +1780,11 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 		}
 		delete(newAnnotations, appv1.AnnotationKeyRefresh)
 	}
-	patch, modified, err := diff.CreateTwoWayMergePatch(
+
+	// Only create merge patch for annotations+status.
+	patch, modified, err := CreateAppMergePatch(
 		&appv1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: orig.GetAnnotations()}, Status: orig.Status},
-		&appv1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: newAnnotations}, Status: *newStatus}, appv1.Application{})
+		&appv1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: newAnnotations}, Status: *newStatus})
 	if err != nil {
 		logCtx.Errorf("Error constructing app status patch: %v", err)
 		return
@@ -1793,6 +1793,7 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 		logCtx.Infof("No status changes. Skipping patch")
 		return
 	}
+
 	// calculate time for path call
 	start := time.Now()
 	defer func() {
@@ -2265,3 +2266,121 @@ func (ctrl *ApplicationController) getAppList(options metav1.ListOptions) (*appv
 }
 
 type ClusterFilterFunction func(c *appv1.Cluster, distributionFunction sharding.DistributionFunction) bool
+
+// CreateAppMergePatch calculates the merge patch for two applications.
+// The main meat of this includes special logic required for handling RawUnstructured fields, which MUST be merged
+// using JSON merges, rather than strategic merges (as is default for other items).
+func CreateAppMergePatch(orig *appv1.Application, new *appv1.Application) ([]byte, bool, error) {
+	noRawOrig, rawOnlyOrig, hasRawOrig := splitAppRawFields(orig)
+	noRawNew, rawOnlyNew, hasRawNew := splitAppRawFields(new)
+
+	// Create merge patch on the Application, which we has no raw fields.
+	patch, modified, err := diff.CreateTwoWayMergePatch(noRawOrig, noRawNew, appv1.Application{})
+	if err != nil {
+		return nil, false, fmt.Errorf("error constructing app patch: %w", err)
+	}
+
+	// If no there are no RawExtension fields to manage, we can return now.
+	if !(hasRawOrig || hasRawNew) {
+		return patch, modified, nil
+	}
+
+	// Create raw objects merge patch, set with the correct paths to ensure merge compatibility.
+	rawObjectsPatch, modifiedRaw, err := jsonCreateMergePatch(rawOnlyOrig, rawOnlyNew)
+	if err != nil {
+		return nil, false, fmt.Errorf("error constructing patch for raw objects: %w", err)
+	}
+
+	// If we generated a merge patch for valueObjects, we now need to merge that patch with the rest of the app patch.
+	if modifiedRaw {
+		patch, err = jsonpatch.MergeMergePatches(patch, rawObjectsPatch)
+		if err != nil {
+			return nil, false, fmt.Errorf("error merging operation patch: %w", err)
+		}
+	}
+
+	return patch, modified || modifiedRaw, nil
+}
+
+// appSourceHasRawFields checks if an appv1.ApplicationSource has any RawExtension fields present.
+func appSourceHasRawFields(src *appv1.ApplicationSource) bool {
+	return src != nil && src.Helm != nil && src.Helm.ValuesObject != nil
+}
+
+// splitAppRawFields takes an appv1.Application and splits it into two application structs:
+// - The first application has all runtime.RawExtension fields set to nil
+// - The second application ONLY has runtime.RawExtension fields
+// The third return value is true if and only if there are non-nil runtime.RawExtension fields to be handled.
+func splitAppRawFields(app *appv1.Application) (*appv1.Application, *appv1.Application, bool) {
+	noRawFields := app.DeepCopy()
+	onlyRawFields := &appv1.Application{}
+	hasRawFields := false
+
+	if src := app.Spec.Source; appSourceHasRawFields(src) {
+		hasRawFields = true
+		onlyRawFields.Spec.Source = &appv1.ApplicationSource{Helm: &appv1.ApplicationSourceHelm{ValuesObject: src.Helm.ValuesObject}}
+		noRawFields.Spec.Source.Helm.ValuesObject = nil
+	}
+	if src := app.Status.Sync.ComparedTo.Source; appSourceHasRawFields(&src) {
+		hasRawFields = true
+		onlyRawFields.Status.Sync.ComparedTo.Source = appv1.ApplicationSource{Helm: &appv1.ApplicationSourceHelm{ValuesObject: src.Helm.ValuesObject}}
+		noRawFields.Status.Sync.ComparedTo.Source.Helm.ValuesObject = nil
+	}
+
+	if srcs := app.Spec.Sources; len(srcs) > 0 {
+		noRaw, rawOnly, changed := splitObjSrcRawFields(srcs)
+		if changed {
+			hasRawFields = true
+			onlyRawFields.Spec.Sources = rawOnly
+			noRawFields.Spec.Sources = noRaw
+		}
+	}
+
+	if srcs := app.Status.Sync.ComparedTo.Sources; len(srcs) > 0 {
+		noRaw, rawOnly, changed := splitObjSrcRawFields(srcs)
+		if changed {
+			hasRawFields = true
+			onlyRawFields.Status.Sync.ComparedTo.Sources = rawOnly
+			noRawFields.Status.Sync.ComparedTo.Sources = noRaw
+		}
+	}
+
+	return noRawFields, onlyRawFields, hasRawFields
+}
+
+// splitObjSrcRawFields takes an appv1.ApplicationSources, and returns two application sources:
+// - The first applicationSource has all runtime.RawExtension fields set to nil
+// - The second applicationSource ONLY has runtime.RawExtension fields
+// The third return value is true if and only if there are non-nil runtime.RawExtension fields to be handled.
+func splitObjSrcRawFields(in appv1.ApplicationSources) (appv1.ApplicationSources, appv1.ApplicationSources, bool) {
+	rawOnly := make(appv1.ApplicationSources, len(in))
+	noRaw := make(appv1.ApplicationSources, len(in))
+	hasHelmValues := false
+
+	// We copy the input to avoid mutating the caller data
+	for idx, s := range in.DeepCopy() {
+		if s.Helm != nil && s.Helm.ValuesObject != nil {
+			rawOnly[idx].Helm = &appv1.ApplicationSourceHelm{ValuesObject: s.Helm.ValuesObject}
+			s.Helm.ValuesObject = nil
+			hasHelmValues = true
+		}
+		noRaw[idx] = s
+	}
+
+	return noRaw, rawOnly, hasHelmValues
+}
+
+// jsonCreateMergePatch is a wrapper func to calculate the diff between two objects
+// instead of bytes.
+func jsonCreateMergePatch(orig, new any) ([]byte, bool, error) {
+	origBytes, err := json.Marshal(orig)
+	if err != nil {
+		return nil, false, err
+	}
+	newBytes, err := json.Marshal(new)
+	if err != nil {
+		return nil, false, err
+	}
+	patch, err := jsonpatch.CreateMergePatch(origBytes, newBytes)
+	return patch, string(patch) != "{}", err
+}

controller/appcontroller_test.go

@@ -4,47 +4,49 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
-	"github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest"
-	"github.com/sirupsen/logrus"
-	"github.com/stretchr/testify/require"
-	"k8s.io/apimachinery/pkg/api/resource"
-	"k8s.io/client-go/rest"
-
 	clustercache "github.com/argoproj/gitops-engine/pkg/cache"
-
-	"github.com/argoproj/argo-cd/v2/common"
-	statecache "github.com/argoproj/argo-cd/v2/controller/cache"
-	"github.com/argoproj/argo-cd/v2/controller/sharding"
-
-	dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks"
 	"github.com/argoproj/gitops-engine/pkg/cache/mocks"
+	"github.com/argoproj/gitops-engine/pkg/diff"
 	synccommon "github.com/argoproj/gitops-engine/pkg/sync/common"
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
+	"github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	apierr "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
 	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/runtime/schema"
 	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/rest"
 	kubetesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/yaml"
 
+	"github.com/argoproj/argo-cd/v2/common"
+	statecache "github.com/argoproj/argo-cd/v2/controller/cache"
 	mockstatecache "github.com/argoproj/argo-cd/v2/controller/cache/mocks"
+	"github.com/argoproj/argo-cd/v2/controller/sharding"
 	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	argoappsv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
 	"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	mockrepoclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient/mocks"
 	"github.com/argoproj/argo-cd/v2/test"
 	"github.com/argoproj/argo-cd/v2/util/argo/normalizers"
 	cacheutil "github.com/argoproj/argo-cd/v2/util/cache"
 	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
+	dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks"
 	"github.com/argoproj/argo-cd/v2/util/settings"
 )
 
@@ -1914,3 +1916,130 @@ func TestAddControllerNamespace(t *testing.T) {
 		assert.Equal(t, test.FakeArgoCDNamespace, updatedApp.Status.ControllerNamespace)
 	})
 }
+
+func newFakeAppHelmValueObject(input []byte) *argoappsv1.Application {
+	app := newFakeApp()
+	helmSource := &argoappsv1.ApplicationSourceHelm{
+		ValuesObject: &runtime.RawExtension{Raw: input},
+	}
+	app.Spec.Source.Helm = helmSource
+	app.Status.Sync.ComparedTo.Source.Helm = helmSource
+	return app
+}
+
+func newFakeAppSources(s argoappsv1.ApplicationSources) *argoappsv1.Application {
+	app := newFakeApp()
+	app.Spec.Sources = s
+	app.Status.Sync.ComparedTo.Sources = s
+	return app
+}
+
+func Test_SimpleHelmValuesObjectMerge(t *testing.T) {
+	origApp := newFakeAppHelmValueObject([]byte(`{"foo": {"bar": "2"}}`))
+	newApp := newFakeAppHelmValueObject([]byte(`{"foo": {"bar": 3}}`))
+
+	// "basic" patch throws error.
+	_, _, err := diff.CreateTwoWayMergePatch(
+		&appv1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: origApp.GetAnnotations()}, Status: origApp.Status},
+		&appv1.Application{ObjectMeta: metav1.ObjectMeta{Annotations: newApp.GetAnnotations()}, Status: newApp.Status}, appv1.Application{})
+	assert.Errorf(t, err, "Basic merge patch should fail")
+
+	// Fancy patch works fine.
+	_, modified, err := CreateAppMergePatch(origApp, newApp)
+	assert.NoError(t, err, "Two-way merge with should not have thrown an error")
+	assert.True(t, modified, "A merge patch should have been created")
+}
+
+func Test_createAppMergePatch(t *testing.T) {
+	tests := []struct {
+		name    string
+		origApp *argoappsv1.Application
+		newApp  *argoappsv1.Application
+	}{
+		{
+			name:    "NoValuesMerge",
+			origApp: newFakeApp(),
+			newApp:  newFakeApp(),
+		}, {
+			name:    "EmptyValuesMerge",
+			origApp: newFakeAppHelmValueObject([]byte(`{}`)),
+			newApp:  newFakeAppHelmValueObject([]byte(`{}`)),
+		}, {
+			name:    "SimpleValuesObjectMerge",
+			origApp: newFakeAppHelmValueObject([]byte(`{"foo": {"bar": "2"}}`)),
+			newApp:  newFakeAppHelmValueObject([]byte(`{"foo": {"bar": 3}}`)),
+		}, {
+			name:    "EmptySourcesMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{}),
+			newApp:  newFakeAppSources([]argoappsv1.ApplicationSource{}),
+		}, {
+			name: "SimpleSourcesMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": "2"}}`)},
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": 3}}`)},
+					},
+				},
+			}),
+		}, {
+			name: "MultipleSourcesMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": "2"}}`)},
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": 3}}`)},
+					},
+				},
+			}),
+		}, {
+			name: "DiffSourcesCountMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": "2"}}`)},
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: &runtime.RawExtension{Raw: []byte(`{"foo": {"bar": 3}}`)},
+					},
+				},
+			}),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			origTmp := tt.origApp.DeepCopy()
+			newTmp := tt.newApp.DeepCopy()
+
+			_, modified, err := CreateAppMergePatch(tt.origApp, tt.newApp)
+			assert.NoError(t, err, fmt.Sprintf("CreateAppMergePatch(%v, %v) should not have thrown an error", tt.origApp, tt.newApp))
+
+			// Ensure that the object haven't been modified during execution.
+			assert.True(t, reflect.DeepEqual(origTmp, tt.origApp), "CreateAppMergePatch(%v, %v) should not have modified original app", tt.origApp, tt.newApp)
+			assert.True(t, reflect.DeepEqual(newTmp, tt.newApp), "CreateAppMergePatch(%v, %v) should not have modified new app", tt.origApp, tt.newApp)
+
+			// If objects are different, then we should get a patch.
+			assert.Equal(t, !reflect.DeepEqual(tt.origApp, tt.newApp), modified, "CreateAppMergePatch(%v, %v) got an unexpected patch result", tt.origApp, tt.newApp)
+
+		})
+	}
+}