From d4dc155a8969a19523bef95cc9741af3ea07ecb4 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Wed, 8 Nov 2023 04:04:07 +0100 Subject: [PATCH 1/2] fix: check for double definition (#15670) * fix: check for double definition As found in #13965 (and as a follow-up to #13999), we also need to define what happens if _both_ managedNamespaceMetadata _and_ an Application manifest are both defined for the same namespace. The idea here is that if that happens, we emit an `ApplicationConditionRepeatedResourceWarning`, and set the sync status to `Unknown`, since it's unclear what is supposed to happen. The user will then have the option of removing one of the two definitions. Signed-off-by: Blake Pettersson * fix: check for double definition A simpler fix - don't add a managed namespace to the targetObjs list if a namespace already exists in the application source. Signed-off-by: Blake Pettersson * test: add test cases This adds a test case showing that an ns manifest will override `managedNamespaceMetadata` without failing horribly (as it currently does on `HEAD`), as well as a "standard" mNMd diff vs live. Signed-off-by: Blake Pettersson --------- Signed-off-by: Blake Pettersson --- controller/state_test.go | 116 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/controller/state_test.go b/controller/state_test.go index 9d2df1f2391858..b08cb74f5b6e6f 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -117,6 +117,122 @@ func TestCompareAppStateNamespaceMetadataDiffers(t *testing.T) { assert.Len(t, app.Status.Conditions, 0) } +// TestCompareAppStateNamespaceMetadataDiffers tests comparison when managed namespace metadata differs to live and manifest ns +func TestCompareAppStateNamespaceMetadataDiffersToManifest(t *testing.T) { + ns := NewNamespace() + ns.SetName(test.FakeDestNamespace) + ns.SetNamespace(test.FakeDestNamespace) + ns.SetAnnotations(map[string]string{"bar": "bat"}) + + app := newFakeApp() + app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{ + Labels: map[string]string{ + "foo": "bar", + }, + Annotations: map[string]string{ + "foo": "bar", + }, + } + app.Status.OperationState = &argoappv1.OperationState{ + SyncResult: &argoappv1.SyncOperationResult{}, + } + + liveNs := ns.DeepCopy() + liveNs.SetAnnotations(nil) + + data := fakeData{ + manifestResponse: &apiclient.ManifestResponse{ + Manifests: []string{toJSON(t, liveNs)}, + Namespace: test.FakeDestNamespace, + Server: test.FakeClusterURL, + Revision: "abc123", + }, + managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ + kube.GetResourceKey(ns): ns, + }, + } + ctrl := newFakeController(&data) + sources := make([]argoappv1.ApplicationSource, 0) + sources = append(sources, app.Spec.GetSource()) + revisions := make([]string, 0) + revisions = append(revisions, "") + compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.NotNil(t, compRes) + assert.NotNil(t, compRes.syncStatus) + assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) + assert.Len(t, compRes.resources, 1) + assert.Len(t, compRes.managedResources, 1) + assert.NotNil(t, compRes.diffResultList) + assert.Len(t, compRes.diffResultList.Diffs, 1) + + result := NewNamespace() + assert.NoError(t, json.Unmarshal(compRes.diffResultList.Diffs[0].PredictedLive, result)) + + labels := result.GetLabels() + delete(labels, "kubernetes.io/metadata.name") + + assert.Equal(t, map[string]string{}, labels) + // Manifests override definitions in managedNamespaceMetadata + assert.Equal(t, map[string]string{"bar": "bat"}, result.GetAnnotations()) + assert.Len(t, app.Status.Conditions, 0) +} + +// TestCompareAppStateNamespaceMetadata tests comparison when managed namespace metadata differs to live +func TestCompareAppStateNamespaceMetadata(t *testing.T) { + ns := NewNamespace() + ns.SetName(test.FakeDestNamespace) + ns.SetNamespace(test.FakeDestNamespace) + ns.SetAnnotations(map[string]string{"bar": "bat"}) + + app := newFakeApp() + app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{ + Labels: map[string]string{ + "foo": "bar", + }, + Annotations: map[string]string{ + "foo": "bar", + }, + } + app.Status.OperationState = &argoappv1.OperationState{ + SyncResult: &argoappv1.SyncOperationResult{}, + } + + data := fakeData{ + manifestResponse: &apiclient.ManifestResponse{ + Manifests: []string{}, + Namespace: test.FakeDestNamespace, + Server: test.FakeClusterURL, + Revision: "abc123", + }, + managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{ + kube.GetResourceKey(ns): ns, + }, + } + ctrl := newFakeController(&data) + sources := make([]argoappv1.ApplicationSource, 0) + sources = append(sources, app.Spec.GetSource()) + revisions := make([]string, 0) + revisions = append(revisions, "") + compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.NotNil(t, compRes) + assert.NotNil(t, compRes.syncStatus) + assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) + assert.Len(t, compRes.resources, 1) + assert.Len(t, compRes.managedResources, 1) + assert.NotNil(t, compRes.diffResultList) + assert.Len(t, compRes.diffResultList.Diffs, 1) + + result := NewNamespace() + assert.NoError(t, json.Unmarshal(compRes.diffResultList.Diffs[0].PredictedLive, result)) + + labels := result.GetLabels() + delete(labels, "kubernetes.io/metadata.name") + + assert.Equal(t, map[string]string{"foo": "bar"}, labels) + assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "bar": "bat", "foo": "bar"}, result.GetAnnotations()) + assert.Len(t, app.Status.Conditions, 0) +} + // TestCompareAppStateNamespaceMetadataIsTheSame tests comparison when managed namespace metadata is the same func TestCompareAppStateNamespaceMetadataIsTheSame(t *testing.T) { app := newFakeApp() From 72245e8557b27bf2141fa5079c155f34a6d8c5ab Mon Sep 17 00:00:00 2001 From: Takumi Sue <23391543+mikutas@users.noreply.github.com> Date: Wed, 8 Nov 2023 21:28:21 +0900 Subject: [PATCH 2/2] fix: failing lint and unit test (#16275) Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --- controller/state_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/controller/state_test.go b/controller/state_test.go index b08cb74f5b6e6f..6eb336cff2ffb3 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -151,12 +151,13 @@ func TestCompareAppStateNamespaceMetadataDiffersToManifest(t *testing.T) { kube.GetResourceKey(ns): ns, }, } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status) @@ -208,12 +209,13 @@ func TestCompareAppStateNamespaceMetadata(t *testing.T) { kube.GetResourceKey(ns): ns, }, } - ctrl := newFakeController(&data) + ctrl := newFakeController(&data, nil) sources := make([]argoappv1.ApplicationSource, 0) sources = append(sources, app.Spec.GetSource()) revisions := make([]string, 0) revisions = append(revisions, "") - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + compRes, err := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false) + assert.Nil(t, err) assert.NotNil(t, compRes) assert.NotNil(t, compRes.syncStatus) assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status)