[DRAFT] Replace runtime.RawUnstructured with internal ArgoCD type to avoid #15126

USERS.md

@@ -36,6 +36,7 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [Beat](https://thebeat.co/en/)
 1. [Beez Innovation Labs](https://www.beezlabs.com/)
 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/)

applicationset/utils/utils.go

@@ -131,7 +131,12 @@ func (r *Render) deeplyReplace(copy, original reflect.Value, replaceMap map[stri
 			// specific case time
 			if currentType == "time.Time" {
 				copy.Field(i).Set(original.Field(i))
-			} else if currentType == "Raw.k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" || currentType == "Raw.k8s.io/apimachinery/pkg/runtime" {
+
+			} else if currentType == "Raw.k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ||
+				currentType == "Raw.k8s.io/apimachinery/pkg/runtime" ||
+				currentType == "ValuesObject.github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" {
+				// Note: We add any raw "[]byte" fields that need to have their contents replaced; eg for valuesObject.
+
 				var unmarshaled interface{}
 				originalBytes := original.Field(i).Bytes()
 				convertedToJson, err := ConvertYAMLToJSON(string(originalBytes))

applicationset/utils/utils_test.go

@@ -221,13 +221,11 @@ func TestRenderHelmValuesObjectJson(t *testing.T) {
 				TargetRevision: "",
 				Chart:          "",
 				Helm: &argoappsv1.ApplicationSourceHelm{
-					ValuesObject: &runtime.RawExtension{
-						Raw: []byte(`{
+					ValuesObject: []byte(`{
 								"some": {
 									"string": "{{.test}}"
 								}
 							  }`),
-					},
 				},
 			},
 			Destination: argoappsv1.ApplicationDestination{
@@ -247,10 +245,10 @@ func TestRenderHelmValuesObjectJson(t *testing.T) {
 	assert.NotNil(t, newApplication)
 
 	var unmarshaled interface{}
-	err = json.Unmarshal(newApplication.Spec.Source.Helm.ValuesObject.Raw, &unmarshaled)
+	err = json.Unmarshal(newApplication.Spec.Source.Helm.ValuesObject, &unmarshaled)
 
 	assert.NoError(t, err)
-	assert.Equal(t, unmarshaled.(map[string]interface{})["some"].(map[string]interface{})["string"], "Hello world")
+	assert.Equal(t, "Hello world", unmarshaled.(map[string]interface{})["some"].(map[string]interface{})["string"])
 
 }
 
@@ -276,10 +274,8 @@ func TestRenderHelmValuesObjectYaml(t *testing.T) {
 				TargetRevision: "",
 				Chart:          "",
 				Helm: &argoappsv1.ApplicationSourceHelm{
-					ValuesObject: &runtime.RawExtension{
-						Raw: []byte(`some:
+					ValuesObject: []byte(`some:
   string: "{{.test}}"`),
-					},
 				},
 			},
 			Destination: argoappsv1.ApplicationDestination{
@@ -299,10 +295,10 @@ func TestRenderHelmValuesObjectYaml(t *testing.T) {
 	assert.NotNil(t, newApplication)
 
 	var unmarshaled interface{}
-	err = json.Unmarshal(newApplication.Spec.Source.Helm.ValuesObject.Raw, &unmarshaled)
+	err = json.Unmarshal(newApplication.Spec.Source.Helm.ValuesObject, &unmarshaled)
 
 	assert.NoError(t, err)
-	assert.Equal(t, unmarshaled.(map[string]interface{})["some"].(map[string]interface{})["string"], "Hello world")
+	assert.Equal(t, "Hello world", unmarshaled.(map[string]interface{})["some"].(map[string]interface{})["string"])
 
 }
 

assets/swagger.json

@@ -5091,17 +5091,6 @@
         }
       }
     },
-    "runtimeRawExtension": {
-      "description": "RawExtension is used to hold extensions in external versions.\n\nTo use this, make a field which has RawExtension as its type in your external, versioned\nstruct, and Object in your internal struct. You also need to register your\nvarious plugin types.\n\n// Internal package:\n\n\ttype MyAPIObject struct {\n\t\truntime.TypeMeta `json:\",inline\"`\n\t\tMyPlugin runtime.Object `json:\"myPlugin\"`\n\t}\n\n\ttype PluginA struct {\n\t\tAOption string `json:\"aOption\"`\n\t}\n\n// External package:\n\n\ttype MyAPIObject struct {\n\t\truntime.TypeMeta `json:\",inline\"`\n\t\tMyPlugin runtime.RawExtension `json:\"myPlugin\"`\n\t}\n\n\ttype PluginA struct {\n\t\tAOption string `json:\"aOption\"`\n\t}\n\n// On the wire, the JSON will look something like this:\n\n\t{\n\t\t\"kind\":\"MyAPIObject\",\n\t\t\"apiVersion\":\"v1\",\n\t\t\"myPlugin\": {\n\t\t\t\"kind\":\"PluginA\",\n\t\t\t\"aOption\":\"foo\",\n\t\t},\n\t}\n\nSo what happens? Decode first uses json or yaml to unmarshal the serialized data into\nyour external MyAPIObject. That causes the raw JSON to be stored, but not unpacked.\nThe next step is to copy (using pkg/conversion) into the internal struct. The runtime\npackage's DefaultScheme has conversion functions installed which will unpack the\nJSON stored in RawExtension, turning it into the correct object type, and storing it\nin the Object. (TODO: In the case where the object is of an unknown type, a\nruntime.Unknown object will be created and stored.)\n\n+k8s:deepcopy-gen=true\n+protobuf=true\n+k8s:openapi-gen=true",
-      "type": "object",
-      "properties": {
-        "raw": {
-          "description": "Raw is the underlying serialization of this object.\n\nTODO: Determine how to detect ContentType and ContentEncoding of 'Raw' data.",
-          "type": "string",
-          "format": "byte"
-        }
-      }
-    },
     "runtimeStreamError": {
       "type": "object",
       "properties": {
@@ -6345,7 +6334,9 @@
           "title": "Values specifies Helm values to be passed to helm template, typically defined as a block. ValuesObject takes precedence over Values, so use one or the other.\n+patchStrategy=replace"
         },
         "valuesObject": {
-          "$ref": "#/definitions/runtimeRawExtension"
+          "type": "string",
+          "format": "byte",
+          "title": "ValuesObject specifies Helm values to be passed to helm template, defined as a map. This takes precedence over Values.\n+kubebuilder:pruning:PreserveUnknownFields"
         },
         "version": {
           "type": "string",

cmd/argocd/commands/app_test.go

@@ -35,7 +35,6 @@ import (
 	"golang.org/x/oauth2"
 	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/util/intstr"
 	k8swatch "k8s.io/apimachinery/pkg/watch"
 )
@@ -603,7 +602,7 @@ func TestPrintApplicationHistoryTableWithMultipleSources(t *testing.T) {
 				"1a",
 				"1b",
 			},
-			//added Source just for testing the fuction
+			// added Source just for testing the fuction
 			Source: v1alpha1.ApplicationSource{
 				TargetRevision: "-1",
 				RepoURL:        "ignore",
@@ -1068,7 +1067,7 @@ func Test_unset(t *testing.T) {
 				},
 			},
 			PassCredentials: true,
-			ValuesObject:    &runtime.RawExtension{Raw: []byte("some: yaml")},
+			ValuesObject:    []byte("some: yaml"),
 			ValueFiles: []string{
 				"values-1.yaml",
 				"values-2.yaml",

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,16 +52,13 @@ 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/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"
@@ -1778,9 +1776,12 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 		}
 		delete(newAnnotations, appv1.AnnotationKeyRefresh)
 	}
+
+	// Only create merge patch for annotations+status.
 	patch, modified, err := diff.CreateTwoWayMergePatch(
 		&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},
+		appv1.Application{})
 	if err != nil {
 		logCtx.Errorf("Error constructing app status patch: %v", err)
 		return
@@ -1789,6 +1790,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() {

controller/appcontroller_test.go

@@ -4,46 +4,48 @@ 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"
 	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"
 )
 
@@ -1910,3 +1912,113 @@ 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: 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_createUnstructuredAppMergePatch(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: []byte(`{"foo": {"bar": "2"}}`),
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: []byte(`{"foo": {"bar": 3}}`),
+					},
+				},
+			}),
+		}, {
+			name: "MultipleSourcesMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: []byte(`{"foo": {"bar": "2"}}`),
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: []byte(`{"foo": {"bar": 3}}`),
+					},
+				},
+			}),
+		}, {
+			name: "DiffSourcesCountMerge",
+			origApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: []byte(`{"foo": {"bar": "2"}}`),
+					},
+				},
+			}),
+			newApp: newFakeAppSources([]argoappsv1.ApplicationSource{
+				{Kustomize: &argoappsv1.ApplicationSourceKustomize{}},
+				{
+					Helm: &argoappsv1.ApplicationSourceHelm{
+						ValuesObject: []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 := diff.CreateTwoWayMergePatch(tt.origApp, tt.newApp, appv1.Application{})
+			assert.NoError(t, err, fmt.Sprintf("CreateTwoWayMergePatch(%v, %v, appv1.Application{}) 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), "CreateTwoWayMergePatch(%v, %v, appv1.Application{}) should not have modified original app", tt.origApp, tt.newApp)
+			assert.True(t, reflect.DeepEqual(newTmp, tt.newApp), "CreateTwoWayMergePatch(%v, %v, appv1.Application{}) 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, "CreateTwoWayMergePatch(%v, %v, appv1.Application{}) got an unexpected patch result", tt.origApp, tt.newApp)
+		})
+	}
+}