From e182389447dda2702f2f6aee1f7a036e59ab21d2 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Fri, 18 Nov 2022 12:56:32 +0100 Subject: [PATCH 1/8] feat: display managed namespaces in ui (#11196) This commit re-enables the resource tracking for managed namespaces, which is required in order to ensure that multiple applications do not attempt to manage the same (managed) namespace. If multiple applications set `managedNamespaceMetadata` for the same namespace, `syncNamespace` will return an error. This leads to the fact that we also need to modify `state.go`, since otherwise managed namespaces will always be `OutOfSync`, due to the fact that the managed namespace is not present in the owning Application's git repository. Fixes #11196. Signed-off-by: Blake Pettersson --- controller/state.go | 32 +++++++- controller/state_test.go | 37 +++++++++ controller/sync.go | 2 +- controller/sync_namespace.go | 28 +++++-- controller/sync_namespace_test.go | 116 +++++++++++++++++------------ docs/user-guide/sync-options.md | 4 + test/e2e/app_management_ns_test.go | 29 ++++++++ 7 files changed, 190 insertions(+), 58 deletions(-) diff --git a/controller/state.go b/controller/state.go index 62b55bea9ce75..9d7ae57ab6384 100644 --- a/controller/state.go +++ b/controller/state.go @@ -577,6 +577,17 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap isSelfReferencedObj := m.isSelfReferencedObj(liveObj, targetObj, app.GetName(), appLabelKey, trackingMethod) + isNotPresentInGit := targetObj == nil && liveObj != nil && isSelfReferencedObj + isManagedNamespace := isNotPresentInGit && gvk.Kind == kubeutil.NamespaceKind && obj.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil + + var requiresPruning bool + // A managed namespace should never be pruned. Any other resource that is not present in git, should be pruned. + if isManagedNamespace { + requiresPruning = false + } else { + requiresPruning = isNotPresentInGit + } + resState := v1alpha1.ResourceStatus{ Namespace: obj.GetNamespace(), Name: obj.GetName(), @@ -584,28 +595,43 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap Version: gvk.Version, Group: gvk.Group, Hook: hookutil.IsHook(obj), - RequiresPruning: targetObj == nil && liveObj != nil && isSelfReferencedObj, + RequiresPruning: requiresPruning, } if targetObj != nil { resState.SyncWave = int64(syncwaves.Wave(targetObj)) } var diffResult diff.DiffResult - if i < len(diffResults.Diffs) { + if i < len(diffResults.Diffs) && !isManagedNamespace { diffResult = diffResults.Diffs[i] + } else if isManagedNamespace { + // A managed namespace has no target object (i.e. it is not present in git), which means we will compute the + // diff by saying that liveObj == targetObj. + bytes, err := liveObj.MarshalJSON() + if err != nil { + conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now}) + failedToLoadObjs = true + continue + } + // TODO: Verify that live ns metadata contains the managed namespace metadata? or can we reliably say that + // this will be handled in the pre-sync? otherwise we'll need to do some involved diffing with the live ns + // vs managedNamespaceMetadata + diffResult = diff.DiffResult{Modified: false, NormalizedLive: bytes, PredictedLive: bytes} } else { diffResult = diff.DiffResult{Modified: false, NormalizedLive: []byte("{}"), PredictedLive: []byte("{}")} } + if resState.Hook || ignore.Ignore(obj) || (targetObj != nil && hookutil.Skip(targetObj)) || !isSelfReferencedObj { // For resource hooks, skipped resources or objects that may have // been created by another controller with annotations copied from // the source object, don't store sync status, and do not affect // overall sync status - } else if diffResult.Modified || targetObj == nil || liveObj == nil { + } else if !isManagedNamespace && (diffResult.Modified || targetObj == nil || liveObj == nil) { // Set resource state to OutOfSync since one of the following is true: // * target and live resource are different // * target resource not defined and live resource is extra // * target resource present but live resource is missing + // * live resource is NOT a managed namespace resState.Status = v1alpha1.SyncStatusCodeOutOfSync // we ignore the status if the obj needs pruning AND we have the annotation needsPruning := targetObj == nil && liveObj != nil diff --git a/controller/state_test.go b/controller/state_test.go index 623e04213e300..48ebc08abcf5c 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -365,6 +365,43 @@ func TestCompareAppStateDuplicatedNamespacedResources(t *testing.T) { assert.Equal(t, 4, len(compRes.resources)) } +func TestCompareAppStateManagedNamespaceMetadata(t *testing.T) { + app := newFakeApp() + app.Spec.SyncPolicy = &argoappv1.SyncPolicy{ + ManagedNamespaceMetadata: &argoappv1.ManagedNamespaceMetadata{ + Labels: nil, + Annotations: nil, + }, + } + + ns := NewNamespace() + ns.SetName(test.FakeDestNamespace) + ns.SetNamespace(test.FakeDestNamespace) + _ = argo.NewResourceTracking().SetAppInstance(ns, common.LabelKeyAppInstance, app.Name, "", argo.TrackingMethodLabel) + + 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) + compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, "", app.Spec.Source, false, false, nil) + + assert.NotNil(t, compRes) + assert.Equal(t, 0, len(app.Status.Conditions)) + assert.NotNil(t, compRes) + assert.NotNil(t, compRes.syncStatus) + assert.Len(t, compRes.resources, 1) + assert.Len(t, compRes.managedResources, 1) + assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status) +} + var defaultProj = argoappv1.AppProject{ ObjectMeta: metav1.ObjectMeta{ Name: "default", diff --git a/controller/sync.go b/controller/sync.go index 783183c17fc7c..5c7bc551ac1be 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -290,7 +290,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha } if syncOp.SyncOptions.HasOption("CreateNamespace=true") { - opts = append(opts, sync.WithNamespaceModifier(syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app.Name, app.Spec.SyncPolicy))) + opts = append(opts, sync.WithNamespaceModifier(syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app))) } syncCtx, cleanup, err := sync.NewSyncContext( diff --git a/controller/sync_namespace.go b/controller/sync_namespace.go index 9203e27f502e7..11639cdfe88b8 100644 --- a/controller/sync_namespace.go +++ b/controller/sync_namespace.go @@ -1,21 +1,24 @@ package controller import ( + "fmt" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/util/argo" gitopscommon "github.com/argoproj/gitops-engine/pkg/sync/common" + log "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -// syncNamespace determine if Argo CD should create and/or manage the namespace +// syncNamespace determines whether Argo CD should create and/or manage the namespace // where the application will be deployed. -func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, trackingMethod v1alpha1.TrackingMethod, appName string, syncPolicy *v1alpha1.SyncPolicy) func(m, l *unstructured.Unstructured) (bool, error) { +func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, trackingMethod v1alpha1.TrackingMethod, app *v1alpha1.Application) func(m *unstructured.Unstructured, l *unstructured.Unstructured) (bool, error) { // This function must return true for the managed namespace to be synced. return func(managedNs, liveNs *unstructured.Unstructured) (bool, error) { if managedNs == nil { return false, nil } + syncPolicy := app.Spec.SyncPolicy isNewNamespace := liveNs == nil isManagedNamespace := syncPolicy != nil && syncPolicy.ManagedNamespaceMetadata != nil @@ -26,18 +29,27 @@ func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, t } if isManagedNamespace { + if liveNs != nil { + // first check if another application owns the live namespace + liveAppName := resourceTracking.GetAppName(liveNs, appLabelKey, trackingMethod) + if liveAppName != "" && liveAppName != app.Name { + log.Errorf("expected namespace %s to be managed by application %s, but it's managed by application %s", liveNs.GetName(), app.Name, liveAppName) + return false, fmt.Errorf("namespace %s is managed by another application than %s", liveNs.GetName(), app.Name) + } + } + managedNamespaceMetadata := syncPolicy.ManagedNamespaceMetadata managedNs.SetLabels(managedNamespaceMetadata.Labels) // managedNamespaceMetadata relies on SSA in order to avoid overriding // existing labels and annotations in namespaces managedNs.SetAnnotations(appendSSAAnnotation(managedNamespaceMetadata.Annotations)) - } - // TODO: https://github.com/argoproj/argo-cd/issues/11196 - // err := resourceTracking.SetAppInstance(managedNs, appLabelKey, appName, "", trackingMethod) - // if err != nil { - // return false, fmt.Errorf("failed to set app instance tracking on the namespace %s: %s", managedNs.GetName(), err) - // } + // set ownership of the namespace to the current application + err := resourceTracking.SetAppInstance(managedNs, appLabelKey, app.Name, "", trackingMethod) + if err != nil { + return false, fmt.Errorf("failed to set app instance tracking on the namespace %s: %s", managedNs.GetName(), err) + } + } return true, nil } diff --git a/controller/sync_namespace_test.go b/controller/sync_namespace_test.go index e18f52800bf03..5fcdb026c3dc3 100644 --- a/controller/sync_namespace_test.go +++ b/controller/sync_namespace_test.go @@ -1,19 +1,18 @@ package controller import ( + "errors" "github.com/argoproj/argo-cd/v2/common" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/util/argo" "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/types" "testing" ) -func createFakeNamespace(uid string, resourceVersion string, labels map[string]string, annotations map[string]string) *unstructured.Unstructured { +func createFakeNamespace(labels map[string]string, annotations map[string]string) *unstructured.Unstructured { un := unstructured.Unstructured{} - un.SetUID(types.UID(uid)) - un.SetResourceVersion(resourceVersion) un.SetLabels(labels) un.SetAnnotations(annotations) un.SetKind("Namespace") @@ -28,6 +27,7 @@ func Test_shouldNamespaceSync(t *testing.T) { managedNs *unstructured.Unstructured liveNs *unstructured.Unstructured expected bool + expectedErr error expectedLabels map[string]string expectedAnnotations map[string]string }{ @@ -66,7 +66,7 @@ func Test_shouldNamespaceSync(t *testing.T) { expected: true, expectedLabels: map[string]string{}, expectedAnnotations: map[string]string{}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: nil, @@ -75,8 +75,8 @@ func Test_shouldNamespaceSync(t *testing.T) { { name: "namespace does not yet exist and managedNamespaceMetadata not nil", expected: true, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{}, @@ -86,8 +86,8 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace does not yet exist and managedNamespaceMetadata has empty labels map", expected: true, expectedLabels: map[string]string{}, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -98,8 +98,8 @@ func Test_shouldNamespaceSync(t *testing.T) { { name: "namespace does not yet exist and managedNamespaceMetadata has empty annotations map", expected: true, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -111,8 +111,8 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace does not yet exist and managedNamespaceMetadata has empty annotations and labels map", expected: true, expectedLabels: map[string]string{}, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -125,8 +125,8 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace does not yet exist and managedNamespaceMetadata has labels", expected: true, expectedLabels: map[string]string{"my-cool-label": "some-value"}, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -138,8 +138,8 @@ func Test_shouldNamespaceSync(t *testing.T) { { name: "namespace does not yet exist and managedNamespaceMetadata has annotations", expected: true, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -152,8 +152,8 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace does not yet exist and managedNamespaceMetadata has annotations and labels", expected: true, expectedLabels: map[string]string{"my-cool-label": "some-value"}, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), liveNs: nil, syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ @@ -166,9 +166,8 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace exists with no labels or annotations and managedNamespaceMetadata has labels", expected: true, expectedLabels: map[string]string{"my-cool-label": "some-value"}, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{"my-cool-label": "some-value"}, @@ -178,9 +177,9 @@ func Test_shouldNamespaceSync(t *testing.T) { { name: "namespace exists with no labels or annotations and managedNamespaceMetadata has annotations", expected: true, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{}, map[string]string{}), + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Annotations: map[string]string{"my-cool-annotation": "some-value"}, @@ -191,10 +190,9 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace exists with no labels or annotations and managedNamespaceMetadata has annotations and labels", expected: true, expectedLabels: map[string]string{"my-cool-label": "some-value"}, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{}, map[string]string{}), - syncPolicy: &v1alpha1.SyncPolicy{ + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{"my-cool-label": "some-value"}, Annotations: map[string]string{"my-cool-annotation": "some-value"}, @@ -204,11 +202,10 @@ func Test_shouldNamespaceSync(t *testing.T) { { name: "namespace exists with labels and managedNamespaceMetadata has mismatching labels", expected: true, - expectedAnnotations: map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, expectedLabels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{"my-cool-label": "some-value"}, map[string]string{}), - syncPolicy: &v1alpha1.SyncPolicy{ + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, Annotations: map[string]string{}, @@ -219,10 +216,9 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace exists with annotations and managedNamespaceMetadata has mismatching annotations", expected: true, expectedLabels: map[string]string{}, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{}, map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}), - syncPolicy: &v1alpha1.SyncPolicy{ + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{}, Annotations: map[string]string{"my-cool-annotation": "some-value"}, @@ -233,29 +229,57 @@ func Test_shouldNamespaceSync(t *testing.T) { name: "namespace exists with annotations and labels managedNamespaceMetadata has mismatching annotations and labels", expected: true, expectedLabels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, - expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, - managedNs: createFakeNamespace("", "", map[string]string{}, map[string]string{}), - liveNs: createFakeNamespace("something", "1", map[string]string{"my-cool-label": "some-value"}, map[string]string{"my-cool-annotation": "some-value"}), - syncPolicy: &v1alpha1.SyncPolicy{ + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value", "argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{"my-cool-annotation": "some-value"}), syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, + Annotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}, + }, + }, + }, + { + name: "managed namespace exists with liveNs ownership set to another application", + expected: false, + expectedErr: errors.New("namespace some-namespace is managed by another application than some-app"), + managedNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{"argocd.argoproj.io/tracking-id": "some-app:/Namespace:/some-namespace", "my-cool-annotation": "some-value"}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{"argocd.argoproj.io/tracking-id": "some-other-app:/Namespace:/some-other-namespace", "my-cool-annotation": "some-value"}), syncPolicy: &v1alpha1.SyncPolicy{ ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ Labels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, Annotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}, }, }, }, + { + name: "managed namespace does not exist with liveNs ownership set to another application", + expected: false, + expectedErr: nil, + expectedLabels: map[string]string{}, + expectedAnnotations: map[string]string{}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{"argocd.argoproj.io/tracking-id": "some-other-app:/Namespace:/some-other-namespace", "my-cool-annotation": "some-value"}), syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: nil, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - actual, err := syncNamespace(argo.NewResourceTracking(), common.LabelKeyAppInstance, argo.TrackingMethodAnnotation, "some-app", tt.syncPolicy)(tt.managedNs, tt.liveNs) - assert.NoError(t, err) + actual, err := syncNamespace(argo.NewResourceTracking(), common.LabelKeyAppInstance, argo.TrackingMethodAnnotation, &v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-app", + }, + Spec: v1alpha1.ApplicationSpec{ + SyncPolicy: tt.syncPolicy, + }, + })(tt.managedNs, tt.liveNs) + assert.Equalf(t, tt.expected, actual, "syncNamespace(%v)", tt.syncPolicy) + assert.Equalf(t, tt.expectedErr, err, "error mismatch: syncNamespace(%v)", tt.syncPolicy) - if tt.managedNs != nil { + if tt.managedNs != nil && tt.expectedErr == nil { assert.Equal(t, tt.expectedLabels, tt.managedNs.GetLabels()) assert.Equal(t, tt.expectedAnnotations, tt.managedNs.GetAnnotations()) } - - assert.Equalf(t, tt.expected, actual, "syncNamespace(%v)", tt.syncPolicy) }) } } diff --git a/docs/user-guide/sync-options.md b/docs/user-guide/sync-options.md index 688e1800bf406..5f2bf17e27a18 100644 --- a/docs/user-guide/sync-options.md +++ b/docs/user-guide/sync-options.md @@ -339,6 +339,10 @@ spec: - CreateNamespace=true ``` +Keep in mind that having *multiple ArgoCD applications managing namespace metadata will raise an error*. It is fine for +other applications to have `CreateNamespace=true` set, as along as `managedNamespaceMetadata` is not present in said +namespaces (the metadata will remain untouched in those cases). + In the case where ArgoCD is "adopting" an existing namespace which already has metadata set on it, we rely on using Server Side Apply in order not to lose metadata which has already been set. The main implication here is that it takes a few extra steps to get rid of an already preexisting field. diff --git a/test/e2e/app_management_ns_test.go b/test/e2e/app_management_ns_test.go index a6065aa8c195a..a500fe6c1ad49 100644 --- a/test/e2e/app_management_ns_test.go +++ b/test/e2e/app_management_ns_test.go @@ -1909,11 +1909,15 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { assert.Empty(t, app.Status.Conditions) + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + delete(ns.Labels, "kubernetes.io/metadata.name") delete(ns.Labels, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) assert.Equal(t, map[string]string{"foo": "bar"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) @@ -1931,12 +1935,15 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { Then(). Expect(Success("")). Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] delete(ns.Labels, "kubernetes.io/metadata.name") delete(ns.Labels, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) assert.Equal(t, map[string]string{"new": "label"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) @@ -1951,11 +1958,15 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { Then(). Expect(Success("")). Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + delete(ns.Labels, "kubernetes.io/metadata.name") delete(ns.Labels, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) assert.Equal(t, map[string]string{"new": "custom-annotation", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) assert.Equal(t, map[string]string{"new": "label"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) @@ -2001,12 +2012,20 @@ func TestNamespacedNamespaceAutoCreationWithMetadataAndNsManifest(t *testing.T) Then(). Expect(Success("")). Expect(Namespace(namespace, func(app *Application, ns *v1.Namespace) { + assert.NotEmpty(t, app.Status.Conditions) + + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + delete(ns.Labels, "kubernetes.io/metadata.name") delete(ns.Labels, "argocd.argoproj.io/tracking-id") delete(ns.Labels, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + // TODO: The tracking id is different for namespace manifests vs namespaces generated with CreateNamespace=true + // Should that be the case or should we change the format to be the same for both types? + assert.Equal(t, fmt.Sprintf("%s:/Namespace:%s/%s", app.Name, AppNamespace(), namespace), trackingId) + // The application namespace manifest takes precedence over what is in managedNamespaceMetadata assert.Equal(t, map[string]string{"test": "true"}, ns.Labels) assert.Equal(t, map[string]string{"foo": "bar", "something": "else"}, ns.Annotations) @@ -2082,6 +2101,8 @@ metadata: Then(). Expect(Success("")). Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + assert.Empty(t, app.Status.Conditions) delete(ns.Labels, "kubernetes.io/metadata.name") @@ -2089,6 +2110,8 @@ metadata: delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "whatevs", "bar": "bat"}, ns.Annotations) })). @@ -2101,6 +2124,7 @@ metadata: Then(). Expect(Success("")). Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] assert.Empty(t, app.Status.Conditions) @@ -2109,6 +2133,8 @@ metadata: delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "hmm", "bar": "bat"}, ns.Annotations) assert.Equal(t, map[string]string{"something": "hmm", "bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) @@ -2122,6 +2148,7 @@ metadata: Then(). Expect(Success("")). Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] assert.Empty(t, app.Status.Conditions) @@ -2130,6 +2157,8 @@ metadata: delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "bar": "bat"}, ns.Annotations) assert.Equal(t, map[string]string{"bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) From 3506437cf44199e8806274378de0cdc9b4ce215f Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Thu, 24 Nov 2022 20:05:15 +0100 Subject: [PATCH 2/8] fix: account for app namespaces Previously we did not account for ArgoCD applications which can be in other namespaces other than the default one. Signed-off-by: Blake Pettersson --- controller/sync.go | 2 +- controller/sync_namespace.go | 7 +- controller/sync_namespace_test.go | 189 +++++++++++++++++- test/e2e/app_management_ns_test.go | 17 +- test/e2e/app_management_test.go | 297 +++++++++++++++++++++++++++++ 5 files changed, 498 insertions(+), 14 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 5c7bc551ac1be..2aa24f0113f04 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -290,7 +290,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha } if syncOp.SyncOptions.HasOption("CreateNamespace=true") { - opts = append(opts, sync.WithNamespaceModifier(syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app))) + opts = append(opts, sync.WithNamespaceModifier(syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app, m.namespace))) } syncCtx, cleanup, err := sync.NewSyncContext( diff --git a/controller/sync_namespace.go b/controller/sync_namespace.go index 11639cdfe88b8..0a7fc95781573 100644 --- a/controller/sync_namespace.go +++ b/controller/sync_namespace.go @@ -11,7 +11,7 @@ import ( // syncNamespace determines whether Argo CD should create and/or manage the namespace // where the application will be deployed. -func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, trackingMethod v1alpha1.TrackingMethod, app *v1alpha1.Application) func(m *unstructured.Unstructured, l *unstructured.Unstructured) (bool, error) { +func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, trackingMethod v1alpha1.TrackingMethod, app *v1alpha1.Application, appNamespace string) func(m *unstructured.Unstructured, l *unstructured.Unstructured) (bool, error) { // This function must return true for the managed namespace to be synced. return func(managedNs, liveNs *unstructured.Unstructured) (bool, error) { if managedNs == nil { @@ -29,10 +29,11 @@ func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, t } if isManagedNamespace { + appInstanceName := app.InstanceName(appNamespace) if liveNs != nil { // first check if another application owns the live namespace liveAppName := resourceTracking.GetAppName(liveNs, appLabelKey, trackingMethod) - if liveAppName != "" && liveAppName != app.Name { + if liveAppName != "" && liveAppName != appInstanceName { log.Errorf("expected namespace %s to be managed by application %s, but it's managed by application %s", liveNs.GetName(), app.Name, liveAppName) return false, fmt.Errorf("namespace %s is managed by another application than %s", liveNs.GetName(), app.Name) } @@ -45,7 +46,7 @@ func syncNamespace(resourceTracking argo.ResourceTracking, appLabelKey string, t managedNs.SetAnnotations(appendSSAAnnotation(managedNamespaceMetadata.Annotations)) // set ownership of the namespace to the current application - err := resourceTracking.SetAppInstance(managedNs, appLabelKey, app.Name, "", trackingMethod) + err := resourceTracking.SetAppInstance(managedNs, appLabelKey, appInstanceName, "", trackingMethod) if err != nil { return false, fmt.Errorf("failed to set app instance tracking on the namespace %s: %s", managedNs.GetName(), err) } diff --git a/controller/sync_namespace_test.go b/controller/sync_namespace_test.go index 5fcdb026c3dc3..5755e36f189d3 100644 --- a/controller/sync_namespace_test.go +++ b/controller/sync_namespace_test.go @@ -30,6 +30,7 @@ func Test_shouldNamespaceSync(t *testing.T) { expectedErr error expectedLabels map[string]string expectedAnnotations map[string]string + appNamespace string }{ { name: "liveNs is nil and syncPolicy is nil", @@ -261,18 +262,202 @@ func Test_shouldNamespaceSync(t *testing.T) { ManagedNamespaceMetadata: nil, }, }, + { + name: "namespace does not yet exist and managedNamespaceMetadata not nil (namespaced)", + expected: true, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{}, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has empty labels map (namespaced)", + expected: true, + expectedLabels: map[string]string{}, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{}, + }, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has empty annotations map (namespaced)", + expected: true, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Annotations: map[string]string{}, + }, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has empty annotations and labels map (namespaced)", + expected: true, + expectedLabels: map[string]string{}, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{}, + Annotations: map[string]string{}, + }, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has labels (namespaced)", + expected: true, + expectedLabels: map[string]string{"my-cool-label": "some-value"}, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value"}, + Annotations: nil, + }, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has annotations (namespaced)", + expected: true, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: nil, + Annotations: map[string]string{"my-cool-annotation": "some-value"}, + }, + }, + }, + { + name: "namespace does not yet exist and managedNamespaceMetadata has annotations and labels (namespaced)", + expected: true, + expectedLabels: map[string]string{"my-cool-label": "some-value"}, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: nil, + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value"}, + Annotations: map[string]string{"my-cool-annotation": "some-value"}, + }, + }, + }, + { + name: "namespace exists with no labels or annotations and managedNamespaceMetadata has labels (namespaced)", + expected: true, + expectedLabels: map[string]string{"my-cool-label": "some-value"}, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value"}, + }, + }, + }, + { + name: "namespace exists with no labels or annotations and managedNamespaceMetadata has annotations (namespaced)", + expected: true, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Annotations: map[string]string{"my-cool-annotation": "some-value"}, + }, + }, + }, + { + name: "namespace exists with no labels or annotations and managedNamespaceMetadata has annotations and labels (namespaced)", + expected: true, + expectedLabels: map[string]string{"my-cool-label": "some-value"}, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value"}, + Annotations: map[string]string{"my-cool-annotation": "some-value"}, + }, + }, + }, + { + name: "namespace exists with labels and managedNamespaceMetadata has mismatching labels (namespaced)", + expected: true, + expectedAnnotations: map[string]string{"argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + expectedLabels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, + Annotations: map[string]string{}, + }, + }, + }, + { + name: "namespace exists with annotations and managedNamespaceMetadata has mismatching annotations (namespaced)", + expected: true, + expectedLabels: map[string]string{}, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{}, map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{}, + Annotations: map[string]string{"my-cool-annotation": "some-value"}, + }, + }, + }, + { + name: "namespace exists with annotations and labels managedNamespaceMetadata has mismatching annotations and labels (namespaced)", + expected: true, + expectedLabels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, + expectedAnnotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value", "argocd.argoproj.io/tracking-id": "argocd-namespace_some-app:/Namespace:/some-namespace", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, + managedNs: createFakeNamespace(map[string]string{}, map[string]string{}), + liveNs: createFakeNamespace(map[string]string{"my-cool-label": "some-value"}, map[string]string{"my-cool-annotation": "some-value"}), + appNamespace: "argocd-namespace", + syncPolicy: &v1alpha1.SyncPolicy{ + ManagedNamespaceMetadata: &v1alpha1.ManagedNamespaceMetadata{ + Labels: map[string]string{"my-cool-label": "some-value", "my-other-label": "some-other-value"}, + Annotations: map[string]string{"my-cool-annotation": "some-value", "my-other-annotation": "some-other-value"}, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { actual, err := syncNamespace(argo.NewResourceTracking(), common.LabelKeyAppInstance, argo.TrackingMethodAnnotation, &v1alpha1.Application{ ObjectMeta: metav1.ObjectMeta{ - Name: "some-app", + Name: "some-app", + Namespace: tt.appNamespace, }, Spec: v1alpha1.ApplicationSpec{ SyncPolicy: tt.syncPolicy, }, - })(tt.managedNs, tt.liveNs) + }, "")(tt.managedNs, tt.liveNs) assert.Equalf(t, tt.expected, actual, "syncNamespace(%v)", tt.syncPolicy) assert.Equalf(t, tt.expectedErr, err, "error mismatch: syncNamespace(%v)", tt.syncPolicy) diff --git a/test/e2e/app_management_ns_test.go b/test/e2e/app_management_ns_test.go index a500fe6c1ad49..43b2ecb2c2e0c 100644 --- a/test/e2e/app_management_ns_test.go +++ b/test/e2e/app_management_ns_test.go @@ -1916,7 +1916,7 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) @@ -1942,7 +1942,7 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) @@ -1965,7 +1965,7 @@ func TestNamespacedNamespaceAutoCreationWithMetadata(t *testing.T) { delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) assert.Equal(t, map[string]string{"new": "custom-annotation", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) @@ -2012,7 +2012,7 @@ func TestNamespacedNamespaceAutoCreationWithMetadataAndNsManifest(t *testing.T) Then(). Expect(Success("")). Expect(Namespace(namespace, func(app *Application, ns *v1.Namespace) { - assert.NotEmpty(t, app.Status.Conditions) + assert.Empty(t, app.Status.Conditions) trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] @@ -2024,7 +2024,8 @@ func TestNamespacedNamespaceAutoCreationWithMetadataAndNsManifest(t *testing.T) // TODO: The tracking id is different for namespace manifests vs namespaces generated with CreateNamespace=true // Should that be the case or should we change the format to be the same for both types? - assert.Equal(t, fmt.Sprintf("%s:/Namespace:%s/%s", app.Name, AppNamespace(), namespace), trackingId) + //assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, namespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:%s/%s", AppNamespace(), app.Name, namespace, namespace), trackingId) // The application namespace manifest takes precedence over what is in managedNamespaceMetadata assert.Equal(t, map[string]string{"test": "true"}, ns.Labels) @@ -2110,7 +2111,7 @@ metadata: delete(ns.Annotations, "argocd.argoproj.io/tracking-id") delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "whatevs", "bar": "bat"}, ns.Annotations) @@ -2133,7 +2134,7 @@ metadata: delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "hmm", "bar": "bat"}, ns.Annotations) @@ -2157,7 +2158,7 @@ metadata: delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") delete(ns.Annotations, "argocd.argoproj.io/tracking-id") - assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + assert.Equal(t, fmt.Sprintf("%s_%s:/Namespace:/%s", AppNamespace(), app.Name, updatedNamespace), trackingId) assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "bar": "bat"}, ns.Annotations) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index d33e3166735d7..4c801fc0fffba 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -3,6 +3,8 @@ package e2e import ( "context" "fmt" + "math/rand" + "os" "reflect" "regexp" "testing" @@ -1908,6 +1910,301 @@ func TestNamespaceAutoCreation(t *testing.T) { }) } +// Given application is set with --sync-option CreateNamespace=true +// +// application --dest-namespace does not exist +// +// Verify application --dest-namespace is created with managedNamespaceMetadata +func TestNamespaceAutoCreationWithMetadata(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + updatedNamespace := getNewNamespace(t) + defer func() { + if !t.Skipped() { + _, err := Run("", "kubectl", "delete", "namespace", updatedNamespace) + assert.NoError(t, err) + } + }() + ctx := Given(t) + ctx. + //SetTrackingMethod("annotation"). + Timeout(30). + Path("guestbook"). + When(). + CreateFromFile(func(app *Application) { + app.Spec.SyncPolicy = &SyncPolicy{ + SyncOptions: SyncOptions{"CreateNamespace=true"}, + ManagedNamespaceMetadata: &ManagedNamespaceMetadata{ + Labels: map[string]string{"foo": "bar"}, + Annotations: map[string]string{"bar": "bat"}, + }} + }). + Then(). + Expect(NoNamespace(updatedNamespace)). + When(). + AppSet("--dest-namespace", updatedNamespace). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + assert.Empty(t, app.Status.Conditions) + + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"foo": "bar"}, ns.Labels) + assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) + assert.Equal(t, map[string]string{"foo": "bar"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) + assert.Equal(t, map[string]string{"bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) + })). + Expect(OperationPhaseIs(OperationSucceeded)).Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, health.HealthStatusHealthy)). + Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, health.HealthStatusHealthy)). + Expect(ResourceSyncStatusWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, SyncStatusCodeSynced)). + When(). + And(func() { + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "replace", "path": "/spec/syncPolicy/managedNamespaceMetadata/labels", "value": {"new":"label"} }]`), metav1.PatchOptions{})) + }). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) + assert.Equal(t, map[string]string{"bar": "bat", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) + assert.Equal(t, map[string]string{"new": "label"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) + assert.Equal(t, map[string]string{"bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) + })). + When(). + And(func() { + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "replace", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations", "value": {"new":"custom-annotation"} }]`), metav1.PatchOptions{})) + }). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"new": "label"}, ns.Labels) + assert.Equal(t, map[string]string{"new": "custom-annotation", "argocd.argoproj.io/sync-options": "ServerSideApply=true"}, ns.Annotations) + assert.Equal(t, map[string]string{"new": "label"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Labels) + assert.Equal(t, map[string]string{"new": "custom-annotation"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) + })) +} + +// Given application is set with --sync-option CreateNamespace=true +// +// application --dest-namespace does not exist +// +// Verify application namespace manifest takes precedence over managedNamespaceMetadata +func TestNamespaceAutoCreationWithMetadataAndNsManifest(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + namespace := "guestbook-ui-with-namespace-manifest" + defer func() { + if !t.Skipped() { + _, err := Run("", "kubectl", "delete", "namespace", namespace) + assert.NoError(t, err) + } + }() + + ctx := Given(t) + ctx. + SetTrackingMethod("annotation"). + Timeout(30). + Path("guestbook-with-namespace-manifest"). + When(). + CreateFromFile(func(app *Application) { + app.Spec.SyncPolicy = &SyncPolicy{ + SyncOptions: SyncOptions{"CreateNamespace=true"}, + ManagedNamespaceMetadata: &ManagedNamespaceMetadata{ + Labels: map[string]string{"foo": "bar", "abc": "123"}, + Annotations: map[string]string{"bar": "bat"}, + }} + }). + Then(). + Expect(NoNamespace(namespace)). + When(). + AppSet("--dest-namespace", namespace). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(namespace, func(app *Application, ns *v1.Namespace) { + assert.NotEmpty(t, app.Status.Conditions) + + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Labels, "kubectl.kubernetes.io/last-applied-configuration") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + + // TODO: The tracking id is different for namespace manifests vs namespaces generated with CreateNamespace=true + // Should that be the case or should we change the format to be the same for both types? + assert.Equal(t, fmt.Sprintf("%s:/Namespace:%s/%s", app.Name, namespace, namespace), trackingId) + + // The application namespace manifest takes precedence over what is in managedNamespaceMetadata + assert.Equal(t, map[string]string{"test": "true"}, ns.Labels) + assert.Equal(t, map[string]string{"foo": "bar", "something": "else"}, ns.Annotations) + })). + Expect(OperationPhaseIs(OperationSucceeded)).Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", namespace, health.HealthStatusHealthy)). + Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", namespace, health.HealthStatusHealthy)). + Expect(ResourceSyncStatusWithNamespaceIs("Deployment", "guestbook-ui", namespace, SyncStatusCodeSynced)) +} + +// Given application is set with --sync-option CreateNamespace=true +// +// application --dest-namespace exists +// +// Verify application --dest-namespace is updated with managedNamespaceMetadata labels and annotations +func TestNamespaceAutoCreationWithPreexistingNs(t *testing.T) { + SkipOnEnv(t, "OPENSHIFT") + updatedNamespace := getNewNamespace(t) + defer func() { + if !t.Skipped() { + _, err := Run("", "kubectl", "delete", "namespace", updatedNamespace) + assert.NoError(t, err) + } + }() + + existingNs := ` +apiVersion: v1 +kind: Namespace +metadata: + name: %s + labels: + test: "true" + annotations: + something: "whatevs" +` + s := fmt.Sprintf(existingNs, updatedNamespace) + + tmpFile, err := os.CreateTemp("", "") + errors.CheckError(err) + _, err = tmpFile.Write([]byte(s)) + errors.CheckError(err) + + _, err = Run("", "kubectl", "apply", "-f", tmpFile.Name()) + assert.NoError(t, err) + + ctx := Given(t) + ctx. + SetTrackingMethod("annotation"). + Timeout(30). + Path("guestbook"). + When(). + CreateFromFile(func(app *Application) { + app.Spec.SyncPolicy = &SyncPolicy{ + SyncOptions: SyncOptions{"CreateNamespace=true"}, + ManagedNamespaceMetadata: &ManagedNamespaceMetadata{ + Labels: map[string]string{"foo": "bar"}, + Annotations: map[string]string{"bar": "bat"}, + }} + }). + Then(). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + assert.Empty(t, app.Status.Conditions) + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + + assert.Equal(t, map[string]string{"test": "true"}, ns.Labels) + assert.Equal(t, map[string]string{"something": "whatevs"}, ns.Annotations) + })). + When(). + AppSet("--dest-namespace", updatedNamespace). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + assert.Empty(t, app.Status.Conditions) + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) + assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "whatevs", "bar": "bat"}, ns.Annotations) + })). + When(). + And(func() { + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "add", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations/something", "value": "hmm" }]`), metav1.PatchOptions{})) + }). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + assert.Empty(t, app.Status.Conditions) + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) + assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "something": "hmm", "bar": "bat"}, ns.Annotations) + assert.Equal(t, map[string]string{"something": "hmm", "bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) + })). + When(). + And(func() { + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "remove", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations/something" }]`), metav1.PatchOptions{})) + }). + Sync(). + Then(). + Expect(Success("")). + Expect(Namespace(updatedNamespace, func(app *Application, ns *v1.Namespace) { + trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] + + assert.Empty(t, app.Status.Conditions) + + delete(ns.Labels, "kubernetes.io/metadata.name") + delete(ns.Labels, "argocd.argoproj.io/tracking-id") + delete(ns.Annotations, "kubectl.kubernetes.io/last-applied-configuration") + delete(ns.Annotations, "argocd.argoproj.io/tracking-id") + + assert.Equal(t, fmt.Sprintf("%s:/Namespace:/%s", app.Name, updatedNamespace), trackingId) + + assert.Equal(t, map[string]string{"test": "true", "foo": "bar"}, ns.Labels) + assert.Equal(t, map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true", "bar": "bat"}, ns.Annotations) + assert.Equal(t, map[string]string{"bar": "bat"}, app.Spec.SyncPolicy.ManagedNamespaceMetadata.Annotations) + })). + Expect(OperationPhaseIs(OperationSucceeded)).Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, health.HealthStatusHealthy)). + Expect(ResourceHealthWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, health.HealthStatusHealthy)). + Expect(ResourceSyncStatusWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, SyncStatusCodeSynced)) +} + func TestFailedSyncWithRetry(t *testing.T) { Given(t). Path("hook"). From b7bb909540c66e95da06224a9f332e1f2ecde860 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Tue, 29 Nov 2022 22:53:20 +0100 Subject: [PATCH 3/8] fix: simplify and correct managedNamespace diffs Instead of doing what we did previously, it's simpler to just generate a managed namespace from the live namespace, which is only used for comparisons. Signed-off-by: Blake Pettersson --- controller/state.go | 59 ++++++++++++++++++++-------------------- controller/state_test.go | 4 ++- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/controller/state.go b/controller/state.go index 9d7ae57ab6384..734a01fd5dd7c 100644 --- a/controller/state.go +++ b/controller/state.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + v1 "k8s.io/api/core/v1" "reflect" "strings" "time" @@ -503,6 +504,32 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap LastTransitionTime: &now, }) } + + // Live namespaces which are managed namespaces (i.e. application namespaces which are managed with + // CreateNamespace=true and has non-nil managedNamespaceMetadata) will (usually) not have a corresponding + // entry in source control. In order for the namespace not to risk being pruned, we'll need to generate a + // namespace which we can compare the live namespace with. For that, we'll do the same as is done in + // gitops-engine and create a managed namespace which is only used for comparison. + if liveObj.GetKind() == kubeutil.NamespaceKind && liveObj.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil { + nsSpec := &v1.Namespace{TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: kubeutil.NamespaceKind}, ObjectMeta: metav1.ObjectMeta{Name: liveObj.GetName()}} + managedNs, err := kubeutil.ToUnstructured(nsSpec) + + // TODO: Check the error handling to confirm that this is indeed the way to do it + if err != nil { + conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now}) + failedToLoadObjs = true + continue + } + + _, err = syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app, m.namespace)(managedNs, liveObj) + if err != nil { + conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now}) + failedToLoadObjs = true + continue + } else { + targetObjs = append(targetObjs, managedNs) + } + } } } @@ -577,17 +604,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap isSelfReferencedObj := m.isSelfReferencedObj(liveObj, targetObj, app.GetName(), appLabelKey, trackingMethod) - isNotPresentInGit := targetObj == nil && liveObj != nil && isSelfReferencedObj - isManagedNamespace := isNotPresentInGit && gvk.Kind == kubeutil.NamespaceKind && obj.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil - - var requiresPruning bool - // A managed namespace should never be pruned. Any other resource that is not present in git, should be pruned. - if isManagedNamespace { - requiresPruning = false - } else { - requiresPruning = isNotPresentInGit - } - resState := v1alpha1.ResourceStatus{ Namespace: obj.GetNamespace(), Name: obj.GetName(), @@ -595,43 +611,28 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap Version: gvk.Version, Group: gvk.Group, Hook: hookutil.IsHook(obj), - RequiresPruning: requiresPruning, + RequiresPruning: targetObj == nil && liveObj != nil && isSelfReferencedObj, } if targetObj != nil { resState.SyncWave = int64(syncwaves.Wave(targetObj)) } var diffResult diff.DiffResult - if i < len(diffResults.Diffs) && !isManagedNamespace { + if i < len(diffResults.Diffs) { diffResult = diffResults.Diffs[i] - } else if isManagedNamespace { - // A managed namespace has no target object (i.e. it is not present in git), which means we will compute the - // diff by saying that liveObj == targetObj. - bytes, err := liveObj.MarshalJSON() - if err != nil { - conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now}) - failedToLoadObjs = true - continue - } - // TODO: Verify that live ns metadata contains the managed namespace metadata? or can we reliably say that - // this will be handled in the pre-sync? otherwise we'll need to do some involved diffing with the live ns - // vs managedNamespaceMetadata - diffResult = diff.DiffResult{Modified: false, NormalizedLive: bytes, PredictedLive: bytes} } else { diffResult = diff.DiffResult{Modified: false, NormalizedLive: []byte("{}"), PredictedLive: []byte("{}")} } - if resState.Hook || ignore.Ignore(obj) || (targetObj != nil && hookutil.Skip(targetObj)) || !isSelfReferencedObj { // For resource hooks, skipped resources or objects that may have // been created by another controller with annotations copied from // the source object, don't store sync status, and do not affect // overall sync status - } else if !isManagedNamespace && (diffResult.Modified || targetObj == nil || liveObj == nil) { + } else if diffResult.Modified || targetObj == nil || liveObj == nil { // Set resource state to OutOfSync since one of the following is true: // * target and live resource are different // * target resource not defined and live resource is extra // * target resource present but live resource is missing - // * live resource is NOT a managed namespace resState.Status = v1alpha1.SyncStatusCodeOutOfSync // we ignore the status if the obj needs pruning AND we have the annotation needsPruning := targetObj == nil && liveObj != nil diff --git a/controller/state_test.go b/controller/state_test.go index 48ebc08abcf5c..4bc85671575c2 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -365,7 +365,7 @@ func TestCompareAppStateDuplicatedNamespacedResources(t *testing.T) { assert.Equal(t, 4, len(compRes.resources)) } -func TestCompareAppStateManagedNamespaceMetadata(t *testing.T) { +func TestCompareAppStateManagedNamespaceMetadataWithLiveNs(t *testing.T) { app := newFakeApp() app.Spec.SyncPolicy = &argoappv1.SyncPolicy{ ManagedNamespaceMetadata: &argoappv1.ManagedNamespaceMetadata{ @@ -377,6 +377,8 @@ func TestCompareAppStateManagedNamespaceMetadata(t *testing.T) { ns := NewNamespace() ns.SetName(test.FakeDestNamespace) ns.SetNamespace(test.FakeDestNamespace) + ns.SetAnnotations(map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"}) + _ = argo.NewResourceTracking().SetAppInstance(ns, common.LabelKeyAppInstance, app.Name, "", argo.TrackingMethodLabel) data := fakeData{ From 8e31f0b5eb111c2ee50813fed502ace41e02c412 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Tue, 29 Nov 2022 23:46:32 +0100 Subject: [PATCH 4/8] fix: set annotation tracking Signed-off-by: Blake Pettersson --- test/e2e/app_management_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 4c801fc0fffba..cdd8b5deaf69a 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -1878,6 +1878,7 @@ func TestNamespaceAutoCreation(t *testing.T) { } }() Given(t). + SetTrackingMethod("annotation"). Timeout(30). Path("guestbook"). When(). @@ -1926,7 +1927,7 @@ func TestNamespaceAutoCreationWithMetadata(t *testing.T) { }() ctx := Given(t) ctx. - //SetTrackingMethod("annotation"). + SetTrackingMethod("annotation"). Timeout(30). Path("guestbook"). When(). From 6f1686517770a1d8b0be701fafbc655d873b6366 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Wed, 30 Nov 2022 23:55:47 +0100 Subject: [PATCH 5/8] test: set correct argo cd namespace in tests Signed-off-by: Blake Pettersson --- test/e2e/app_management_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index cdd8b5deaf69a..420923907f017 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -1968,7 +1968,7 @@ func TestNamespaceAutoCreationWithMetadata(t *testing.T) { Expect(ResourceSyncStatusWithNamespaceIs("Deployment", "guestbook-ui", updatedNamespace, SyncStatusCodeSynced)). When(). And(func() { - FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(ArgoCDNamespace).Patch(context.Background(), ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "replace", "path": "/spec/syncPolicy/managedNamespaceMetadata/labels", "value": {"new":"label"} }]`), metav1.PatchOptions{})) }). Sync(). @@ -1991,7 +1991,7 @@ func TestNamespaceAutoCreationWithMetadata(t *testing.T) { })). When(). And(func() { - FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(ArgoCDNamespace).Patch(context.Background(), ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "replace", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations", "value": {"new":"custom-annotation"} }]`), metav1.PatchOptions{})) }). Sync(). @@ -2051,7 +2051,7 @@ func TestNamespaceAutoCreationWithMetadataAndNsManifest(t *testing.T) { Then(). Expect(Success("")). Expect(Namespace(namespace, func(app *Application, ns *v1.Namespace) { - assert.NotEmpty(t, app.Status.Conditions) + assert.Empty(t, app.Status.Conditions) trackingId := ns.Annotations["argocd.argoproj.io/tracking-id"] @@ -2097,7 +2097,7 @@ metadata: labels: test: "true" annotations: - something: "whatevs" + something: "whatevs" ` s := fmt.Sprintf(existingNs, updatedNamespace) @@ -2155,7 +2155,7 @@ metadata: })). When(). And(func() { - FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(ArgoCDNamespace).Patch(context.Background(), ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "add", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations/something", "value": "hmm" }]`), metav1.PatchOptions{})) }). Sync(). @@ -2179,7 +2179,7 @@ metadata: })). When(). And(func() { - FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(AppNamespace()).Patch(context.Background(), + FailOnErr(AppClientset.ArgoprojV1alpha1().Applications(ArgoCDNamespace).Patch(context.Background(), ctx.GetName(), types.JSONPatchType, []byte(`[{ "op": "remove", "path": "/spec/syncPolicy/managedNamespaceMetadata/annotations/something" }]`), metav1.PatchOptions{})) }). Sync(). From cef3920c300ed7fa75df04778c112a194c2e16be Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Thu, 1 Dec 2022 09:05:01 +0100 Subject: [PATCH 6/8] test: keep test as it originally was Signed-off-by: Blake Pettersson --- test/e2e/app_management_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 420923907f017..feb1eff53996d 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -1878,7 +1878,6 @@ func TestNamespaceAutoCreation(t *testing.T) { } }() Given(t). - SetTrackingMethod("annotation"). Timeout(30). Path("guestbook"). When(). From dd9bb9a9b191286a7be2627a99ee381646c4bdce Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Thu, 1 Dec 2022 09:05:21 +0100 Subject: [PATCH 7/8] fix: add nil-check Signed-off-by: Blake Pettersson --- controller/state.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controller/state.go b/controller/state.go index 734a01fd5dd7c..af08c700f78e6 100644 --- a/controller/state.go +++ b/controller/state.go @@ -509,8 +509,8 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap // CreateNamespace=true and has non-nil managedNamespaceMetadata) will (usually) not have a corresponding // entry in source control. In order for the namespace not to risk being pruned, we'll need to generate a // namespace which we can compare the live namespace with. For that, we'll do the same as is done in - // gitops-engine and create a managed namespace which is only used for comparison. - if liveObj.GetKind() == kubeutil.NamespaceKind && liveObj.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil { + // gitops-engine, the difference here being that we create a managed namespace which is only used for comparison. + if liveObj.GetKind() == kubeutil.NamespaceKind && liveObj.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy != nil && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil { nsSpec := &v1.Namespace{TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: kubeutil.NamespaceKind}, ObjectMeta: metav1.ObjectMeta{Name: liveObj.GetName()}} managedNs, err := kubeutil.ToUnstructured(nsSpec) @@ -521,11 +521,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap continue } + // No need to care about the return value here, we just want the modified managedNs _, err = syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app, m.namespace)(managedNs, liveObj) if err != nil { conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now}) failedToLoadObjs = true - continue } else { targetObjs = append(targetObjs, managedNs) } From c925d6e1dbb2d4b534879c432bc0fc252a0b5c03 Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Fri, 10 Feb 2023 20:09:26 +0100 Subject: [PATCH 8/8] test: rebase fixes Signed-off-by: Blake Pettersson --- controller/state_test.go | 2 +- test/e2e/app_management_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/controller/state_test.go b/controller/state_test.go index 4bc85671575c2..2d162acb306f9 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -393,7 +393,7 @@ func TestCompareAppStateManagedNamespaceMetadataWithLiveNs(t *testing.T) { }, } ctrl := newFakeController(&data) - compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, "", app.Spec.Source, false, false, nil) + compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, []string{}, app.Spec.Sources, false, false, nil, false) assert.NotNil(t, compRes) assert.Equal(t, 0, len(app.Status.Conditions)) diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index feb1eff53996d..2f2efeec072f8 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -3,7 +3,6 @@ package e2e import ( "context" "fmt" - "math/rand" "os" "reflect" "regexp"