Skip to content

Commit

Permalink
fix: status.sync.comparedTo should use replace patch strategy (argopr…
Browse files Browse the repository at this point in the history 
…oj#18061)

* fix: status.sync.comparedTo should use replace patch strategy

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

* add e2e tests

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

---------

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
  • Loading branch information
@alexmt
alexmt committed May 4, 2024
1 parent a67037a commit 6ac4cda
Show file tree
Hide file tree
Showing 7 changed files with 222 additions and 3 deletions.
4 changes: 2 additions & 2 deletions controller/appcontroller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand All @@ -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
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/application/v1alpha1/generated.proto
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/apis/application/v1alpha1/openapi_generated.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion pkg/apis/application/v1alpha1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
85 changes: 85 additions & 0 deletions pkg/apis/application/v1alpha1/types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
42 changes: 42 additions & 0 deletions test/e2e/app_management_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
})

}
85 changes: 85 additions & 0 deletions test/e2e/applicationset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}))
}

0 comments on commit 6ac4cda

Please sign in to comment.