diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index ff223cfd4e125..934d748a8fa51 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -801,7 +801,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"}}` { + if string(patchAction.GetPatch()) == `{"spec":{"project":"default"},"status":{"sync":{"comparedTo":{"destination":{},"source":{"repoURL":""}}}}}` { normalized = true } } @@ -823,7 +823,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"}}` { + if string(patchAction.GetPatch()) == `{"spec":{"project":"default"},"status":{"sync":{"comparedTo":{"destination":{},"source":{"repoURL":""}}}}}` { normalized = true } } diff --git a/pkg/apis/application/v1alpha1/generated.proto b/pkg/apis/application/v1alpha1/generated.proto index 9794adad75483..c9225e327ae22 100644 --- a/pkg/apis/application/v1alpha1/generated.proto +++ b/pkg/apis/application/v1alpha1/generated.proto @@ -2153,6 +2153,7 @@ 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 3696b2935c9e4..1387b9538924b 100644 --- a/pkg/apis/application/v1alpha1/openapi_generated.go +++ b/pkg/apis/application/v1alpha1/openapi_generated.go @@ -7381,6 +7381,11 @@ 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 2eb5166c7d1ea..b1814bfaa5406 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -1422,7 +1422,8 @@ 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 - ComparedTo ComparedTo `json:"comparedTo,omitempty" protobuf:"bytes,2,opt,name=comparedTo"` + // +patchStrategy=replace + ComparedTo ComparedTo `json:"comparedTo,omitempty" protobuf:"bytes,2,opt,name=comparedTo" patchStrategy:"replace"` // 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 aa629529a25e9..0e3cfd9560615 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -11,7 +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" @@ -3610,3 +3613,85 @@ 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)) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index e25e9c2aab2ac..5bbac08dafb4d 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -21,6 +21,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" 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/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -2846,3 +2847,44 @@ func TestAnnotationTrackingExtraResources(t *testing.T) { Expect(HealthIs(health.HealthStatusHealthy)) } + +// Test designed to cover #15126. +// The issue occurs in the controller, when a valuesObject field that contains non-strings (eg, a nested map) gets +// merged/patched. +// Note: Failure is observed by the test timing out, because the controller cannot 'merge' the patch. +func TestPatchValuesObject(t *testing.T) { + + Given(t). + Timeout(30). + Path("helm"). + When(). + // app should be auto-synced once created + CreateFromFile(func(app *Application) { + app.Spec.Source.Helm = &ApplicationSourceHelm{ + ValuesObject: &runtime.RawExtension{ + // Setup by using nested YAML objects, which is what causes the patch error: + // "unable to find api field in struct RawExtension for the json field "some"" + Raw: []byte(`{"some": {"foo": "bar"}}`), + }, + } + }). + Then(). + When(). + PatchApp(`[{ + "op": "add", + "path": "/spec/source/helm/valuesObject", + "value": {"some":{"foo":"bar","new":"field"}} + }]`). + Refresh(RefreshTypeNormal). + Sync(). + Then(). + Expect(Success("")). + Expect(OperationPhaseIs(OperationSucceeded)). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + Expect(NoConditions()). + And(func(app *Application) { + // Check that the patch was a success. + assert.Equal(t, `{"some":{"foo":"bar","new":"field"}}`, string(app.Spec.Source.Helm.ValuesObject.Raw)) + }) + +} diff --git a/test/e2e/applicationset_test.go b/test/e2e/applicationset_test.go index 415de564566cc..6833835470b8a 100644 --- a/test/e2e/applicationset_test.go +++ b/test/e2e/applicationset_test.go @@ -2266,3 +2266,88 @@ func TestGitGeneratorPrivateRepoGoTemplate(t *testing.T) { When(). Delete().Then().Expect(ApplicationsDoNotExist(expectedAppsNewNamespace)) } + +func TestUpdateHelmValuesObject(t *testing.T) { + + expectedApp := argov1alpha1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: application.ApplicationKind, + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: fixture.TestNamespace(), + Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, + }, + Spec: argov1alpha1.ApplicationSpec{ + Project: "default", + Source: &argov1alpha1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "helm-guestbook", + Helm: &argov1alpha1.ApplicationSourceHelm{ + ValuesObject: &runtime.RawExtension{ + // This will always be converted as yaml + Raw: []byte(`{"some":{"foo":"bar"}}`), + }, + }, + }, + Destination: argov1alpha1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + }, + } + + Given(t). + // Create a ListGenerator-based ApplicationSet + When().Create(v1alpha1.ApplicationSet{ObjectMeta: metav1.ObjectMeta{ + Name: "test-values-object-patch", + }, + Spec: v1alpha1.ApplicationSetSpec{ + GoTemplate: true, + Template: v1alpha1.ApplicationSetTemplate{ + ApplicationSetTemplateMeta: v1alpha1.ApplicationSetTemplateMeta{Name: "{{.cluster}}-guestbook"}, + Spec: argov1alpha1.ApplicationSpec{ + Project: "default", + Source: &argov1alpha1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "helm-guestbook", + Helm: &argov1alpha1.ApplicationSourceHelm{ + ValuesObject: &runtime.RawExtension{ + Raw: []byte(`{"some":{"string":"{{.test}}"}}`), + }, + }, + }, + Destination: argov1alpha1.ApplicationDestination{ + Server: "{{.url}}", + Namespace: "guestbook", + }, + }, + }, + Generators: []v1alpha1.ApplicationSetGenerator{ + { + List: &v1alpha1.ListGenerator{ + Elements: []apiextensionsv1.JSON{{ + Raw: []byte(`{"cluster": "my-cluster","url": "https://kubernetes.default.svc", "test": "Hello world"}`), + }}, + }, + }, + }, + }, + }).Then(). + Expect(ApplicationSetHasConditions("test-values-object-patch", ExpectedConditions)). + When(). + // Update the app spec with some knew ValuesObject to force a merge + Update(func(as *argov1alpha1.ApplicationSet) { + as.Spec.Template.Spec.Source.Helm.ValuesObject = &runtime.RawExtension{ + Raw: []byte(`{"some":{"foo":"bar"}}`), + } + }). + Then(). + Expect(ApplicationsExist([]argov1alpha1.Application{expectedApp})). + When(). + // Delete the ApplicationSet, and verify it deletes the Applications + Delete().Then().Expect(ApplicationsDoNotExist([]argov1alpha1.Application{expectedApp})) +}