From 170ef860a6db8e3537447e1ffc2a50dd1d679516 Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Thu, 1 Aug 2024 11:55:55 -0700 Subject: [PATCH] fix: ArgoCD 2.11 - Loop of PATCH calls to Application objects (#19340) Signed-off-by: Alexander Matyushentsev --- controller/appcontroller.go | 20 ++++- controller/appcontroller_test.go | 65 +++++++++++++- pkg/apis/application/v1alpha1/generated.proto | 1 - .../application/v1alpha1/openapi_generated.go | 5 -- pkg/apis/application/v1alpha1/types.go | 3 +- pkg/apis/application/v1alpha1/types_test.go | 87 +------------------ 6 files changed, 84 insertions(+), 97 deletions(-) diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 49f0fbe2a76b6..ea788e955e726 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -1605,6 +1605,22 @@ func (ctrl *ApplicationController) normalizeApplication(orig, app *appv1.Applica } } +func createMergePatch(orig, new interface{}) ([]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) + if err != nil { + return nil, false, err + } + return patch, string(patch) != "{}", nil +} + // persistAppStatus persists updates to application status. If no changes were made, it is a no-op func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, newStatus *appv1.ApplicationStatus) { logCtx := log.WithFields(log.Fields{"application": orig.QualifiedName()}) @@ -1624,9 +1640,9 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new } delete(newAnnotations, appv1.AnnotationKeyRefresh) } - patch, modified, err := diff.CreateTwoWayMergePatch( + patch, modified, err := createMergePatch( &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 diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 934d748a8fa51..be2e0fa73498b 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -21,6 +21,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/utils/kube/kubetest" "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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -801,7 +802,7 @@ func TestNormalizeApplication(t *testing.T) { normalized := false fakeAppCs.AddReactor("patch", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { if patchAction, ok := action.(kubetesting.PatchAction); ok { - if string(patchAction.GetPatch()) == `{"spec":{"project":"default"},"status":{"sync":{"comparedTo":{"destination":{},"source":{"repoURL":""}}}}}` { + if string(patchAction.GetPatch()) == `{"spec":{"project":"default"}}` { normalized = true } } @@ -1728,3 +1729,65 @@ func TestAddControllerNamespace(t *testing.T) { assert.Equal(t, test.FakeArgoCDNamespace, updatedApp.Status.ControllerNamespace) }) } + +func TestHelmValuesObjectHasReplaceStrategy(t *testing.T) { + app := v1alpha1.Application{ + Status: v1alpha1.ApplicationStatus{Sync: v1alpha1.SyncStatus{ComparedTo: v1alpha1.ComparedTo{ + Source: v1alpha1.ApplicationSource{ + Helm: &v1alpha1.ApplicationSourceHelm{ + ValuesObject: &runtime.RawExtension{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{"key": []string{"value"}}}, + }, + }, + }, + }}}, + } + + appModified := v1alpha1.Application{ + Status: v1alpha1.ApplicationStatus{Sync: v1alpha1.SyncStatus{ComparedTo: v1alpha1.ComparedTo{ + Source: v1alpha1.ApplicationSource{ + Helm: &v1alpha1.ApplicationSourceHelm{ + ValuesObject: &runtime.RawExtension{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{"key": []string{"value-modified1"}}}, + }, + }, + }, + }}}, + } + + patch, _, err := createMergePatch( + app, + appModified) + require.NoError(t, err) + assert.Equal(t, `{"status":{"sync":{"comparedTo":{"source":{"helm":{"valuesObject":{"key":["value-modified1"]}}}}}}}`, string(patch)) +} + +func TestAppStatusIsReplaced(t *testing.T) { + original := &v1alpha1.ApplicationStatus{Sync: v1alpha1.SyncStatus{ + ComparedTo: v1alpha1.ComparedTo{ + Destination: v1alpha1.ApplicationDestination{ + Server: "https://mycluster", + }, + }, + }} + + updated := &v1alpha1.ApplicationStatus{Sync: v1alpha1.SyncStatus{ + ComparedTo: v1alpha1.ComparedTo{ + Destination: v1alpha1.ApplicationDestination{ + Name: "mycluster", + }, + }, + }} + + patchData, ok, err := createMergePatch(original, updated) + + require.NoError(t, err) + require.True(t, ok) + patchObj := map[string]interface{}{} + require.NoError(t, json.Unmarshal(patchData, &patchObj)) + + val, has, err := unstructured.NestedFieldNoCopy(patchObj, "sync", "comparedTo", "destination", "server") + require.NoError(t, err) + require.True(t, has) + require.Nil(t, val) +} diff --git a/pkg/apis/application/v1alpha1/generated.proto b/pkg/apis/application/v1alpha1/generated.proto index c9225e327ae22..9794adad75483 100644 --- a/pkg/apis/application/v1alpha1/generated.proto +++ b/pkg/apis/application/v1alpha1/generated.proto @@ -2153,7 +2153,6 @@ message SyncStatus { optional string status = 1; // ComparedTo contains information about what has been compared - // +patchStrategy=replace optional ComparedTo comparedTo = 2; // Revision contains information about the revision the comparison has been performed to diff --git a/pkg/apis/application/v1alpha1/openapi_generated.go b/pkg/apis/application/v1alpha1/openapi_generated.go index 1387b9538924b..3696b2935c9e4 100644 --- a/pkg/apis/application/v1alpha1/openapi_generated.go +++ b/pkg/apis/application/v1alpha1/openapi_generated.go @@ -7381,11 +7381,6 @@ func schema_pkg_apis_application_v1alpha1_SyncStatus(ref common.ReferenceCallbac }, }, "comparedTo": { - VendorExtensible: spec.VendorExtensible{ - Extensions: spec.Extensions{ - "x-kubernetes-patch-strategy": "replace", - }, - }, SchemaProps: spec.SchemaProps{ Description: "ComparedTo contains information about what has been compared", Default: map[string]interface{}{}, diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index b1814bfaa5406..2eb5166c7d1ea 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -1422,8 +1422,7 @@ type SyncStatus struct { // Status is the sync state of the comparison Status SyncStatusCode `json:"status" protobuf:"bytes,1,opt,name=status,casttype=SyncStatusCode"` // ComparedTo contains information about what has been compared - // +patchStrategy=replace - ComparedTo ComparedTo `json:"comparedTo,omitempty" protobuf:"bytes,2,opt,name=comparedTo" patchStrategy:"replace"` + ComparedTo ComparedTo `json:"comparedTo,omitempty" protobuf:"bytes,2,opt,name=comparedTo"` // Revision contains information about the revision the comparison has been performed to Revision string `json:"revision,omitempty" protobuf:"bytes,3,opt,name=revision"` // Revisions contains information about the revisions of multiple sources the comparison has been performed to diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 0e3cfd9560615..14ce4feb838b4 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -11,13 +11,10 @@ import ( "testing" "time" - "github.com/argoproj/gitops-engine/pkg/diff" "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/pointer" argocdcommon "github.com/argoproj/argo-cd/v2/common" + "k8s.io/utils/pointer" "github.com/argoproj/gitops-engine/pkg/sync/common" "github.com/stretchr/testify/assert" @@ -3613,85 +3610,3 @@ func TestOptionalMapEquality(t *testing.T) { }) } } -<<<<<<< HEAD -======= - -func TestApplicationSpec_GetSourcePtrByIndex(t *testing.T) { - testCases := []struct { - name string - application ApplicationSpec - sourceIndex int - expected *ApplicationSource - }{ - { - name: "HasMultipleSources_ReturnsFirstSource", - application: ApplicationSpec{ - Sources: []ApplicationSource{ - {RepoURL: "https://github.com/argoproj/test1.git"}, - {RepoURL: "https://github.com/argoproj/test2.git"}, - }, - }, - sourceIndex: 0, - expected: &ApplicationSource{RepoURL: "https://github.com/argoproj/test1.git"}, - }, - { - name: "HasMultipleSources_ReturnsSourceAtIndex", - application: ApplicationSpec{ - Sources: []ApplicationSource{ - {RepoURL: "https://github.com/argoproj/test1.git"}, - {RepoURL: "https://github.com/argoproj/test2.git"}, - }, - }, - sourceIndex: 1, - expected: &ApplicationSource{RepoURL: "https://github.com/argoproj/test2.git"}, - }, - { - name: "HasSingleSource_ReturnsSource", - application: ApplicationSpec{ - Source: &ApplicationSource{RepoURL: "https://github.com/argoproj/test.git"}, - }, - sourceIndex: 0, - expected: &ApplicationSource{RepoURL: "https://github.com/argoproj/test.git"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - actual := tc.application.GetSourcePtrByIndex(tc.sourceIndex) - assert.Equal(t, tc.expected, actual) - }) - } -} - -func TestHelmValuesObjectHasReplaceStrategy(t *testing.T) { - app := Application{ - Status: ApplicationStatus{Sync: SyncStatus{ComparedTo: ComparedTo{ - Source: ApplicationSource{ - Helm: &ApplicationSourceHelm{ - ValuesObject: &runtime.RawExtension{ - Object: &unstructured.Unstructured{Object: map[string]interface{}{"key": []string{"value"}}}, - }, - }, - }, - }}}, - } - - appModified := Application{ - Status: ApplicationStatus{Sync: SyncStatus{ComparedTo: ComparedTo{ - Source: ApplicationSource{ - Helm: &ApplicationSourceHelm{ - ValuesObject: &runtime.RawExtension{ - Object: &unstructured.Unstructured{Object: map[string]interface{}{"key": []string{"value-modified1"}}}, - }, - }, - }, - }}}, - } - - patch, _, err := diff.CreateTwoWayMergePatch( - app, - appModified, Application{}) - require.NoError(t, err) - assert.Equal(t, `{"status":{"sync":{"comparedTo":{"destination":{},"source":{"helm":{"valuesObject":{"key":["value-modified1"]}},"repoURL":""}}}}}`, string(patch)) -} ->>>>>>> ec09937fe (fix: status.sync.comparedTo should use replace patch strategy (#18061))