fix(controller): make managed namespaces more 'prune-proof' (argoproj…

…#13999)

* fix: make managed namespaces more 'prune-proof'

In the cases where someone would want to set resource tracking on a
managed namespace, or if someone would want to migrate from having a
namespace in Git to using `managedNamespaceMetadata`, we need to take
steps to ensure that the namespace does not get pruned.

In order to do that, we add the live namespace to the list of
`targetObjs` (so that it's considered a part of the source even though
it's not).

Finally, we need to also ensure that the managed namespace is not
considered `OutOfSync` (due to the same reason as above).

This is a subset of argoproj#11350, the main difference being that this commit
does not set resource tracking on its own, but can be opted into if
people choose to do so.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* refactor: extract managed namespace check

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

---------

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

controller/state.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	v1 "k8s.io/api/core/v1"
 	"reflect"
 	"strings"
 	"time"
@@ -337,6 +338,10 @@ func verifyGnuPGSignature(revision string, project *v1alpha1.AppProject, manifes
 	return conditions
 }
 
+func isManagedNamespace(ns *unstructured.Unstructured, app *v1alpha1.Application) bool {
+	return ns != nil && ns.GetKind() == kubeutil.NamespaceKind && ns.GetName() == app.Spec.Destination.Namespace && app.Spec.SyncPolicy != nil && app.Spec.SyncPolicy.ManagedNamespaceMetadata != nil
+}
+
 // CompareAppState compares application git state to the live app state, using the specified
 // revision and supplied source. If revision or overrides are empty, then compares against
 // revision and overrides in the app spec.
@@ -496,6 +501,35 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 					LastTransitionTime: &now,
 				})
 			}
+
+			// For the case when a namespace is managed with `managedNamespaceMetadata` AND it has resource tracking
+			// enabled (e.g. someone manually adds resource tracking labels or annotations), we need to do some
+			// bookkeeping in order to prevent the managed namespace from being pruned.
+			//
+			// 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, the difference here being that we create a managed namespace which is only used for comparison.
+			if isManagedNamespace(liveObj, app) {
+				nsSpec := &v1.Namespace{TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: kubeutil.NamespaceKind}, ObjectMeta: metav1.ObjectMeta{Name: liveObj.GetName()}}
+				managedNs, err := kubeutil.ToUnstructured(nsSpec)
+
+				if err != nil {
+					conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now})
+					failedToLoadObjs = true
+					continue
+				}
+
+				// No need to care about the return value here, we just want the modified managedNs
+				_, err = syncNamespace(m.resourceTracking, appLabelKey, trackingMethod, app.Name, app.Spec.SyncPolicy)(managedNs, liveObj)
+				if err != nil {
+					conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now})
+					failedToLoadObjs = true
+				} else {
+					targetObjs = append(targetObjs, managedNs)
+				}
+			}
 		}
 	}
 
@@ -590,12 +624,22 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 		} else {
 			diffResult = diff.DiffResult{Modified: false, NormalizedLive: []byte("{}"), PredictedLive: []byte("{}")}
 		}
+
+		// For the case when a namespace is managed with `managedNamespaceMetadata` AND it has resource tracking
+		// enabled (e.g. someone manually adds resource tracking labels or annotations), we need to do some
+		// bookkeeping in order to ensure that it's not considered `OutOfSync` (since it does not exist in source
+		// control).
+		//
+		// This is in addition to the bookkeeping we do (see `isManagedNamespace` and its references) to prevent said
+		// namespace from being pruned.
+		isManagedNs := isManagedNamespace(targetObj, app) && liveObj == nil
+
 		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 !isManagedNs && (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

controller/state_test.go

@@ -432,6 +432,47 @@ func TestCompareAppStateDuplicatedNamespacedResources(t *testing.T) {
 	assert.Equal(t, 4, len(compRes.resources))
 }
 
+func TestCompareAppStateManagedNamespaceMetadataWithLiveNsDoesNotGetPruned(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)
+	ns.SetAnnotations(map[string]string{"argocd.argoproj.io/sync-options": "ServerSideApply=true"})
+
+	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, []string{}, app.Spec.Sources, false, false, nil, false)
+
+	assert.NotNil(t, compRes)
+	assert.Equal(t, 0, len(app.Status.Conditions))
+	assert.NotNil(t, compRes)
+	assert.NotNil(t, compRes.syncStatus)
+	// Ensure that ns does not get pruned
+	assert.NotNil(t, compRes.reconciliationResult.Target[0])
+	assert.Equal(t, compRes.reconciliationResult.Target[0].GetName(), ns.GetName())
+	assert.Equal(t, compRes.reconciliationResult.Target[0].GetAnnotations(), ns.GetAnnotations())
+	assert.Equal(t, compRes.reconciliationResult.Target[0].GetLabels(), ns.GetLabels())
+	assert.Len(t, compRes.resources, 1)
+	assert.Len(t, compRes.managedResources, 1)
+}
+
 var defaultProj = argoappv1.AppProject{
 	ObjectMeta: metav1.ObjectMeta{
 		Name:      "default",

test/e2e/app_management_test.go

@@ -1754,6 +1754,40 @@ func TestCompareOptionIgnoreExtraneous(t *testing.T) {
 		Expect(SyncStatusIs(SyncStatusCodeSynced))
 }
 
+func TestSourceNamespaceCanBeMigratedToManagedNamespaceWithoutBeingPrunedOrOutOfSync(t *testing.T) {
+	Given(t).
+		Prune(true).
+		Path("guestbook-with-plain-namespace-manifest").
+		When().
+		PatchFile("guestbook-ui-namespace.yaml", fmt.Sprintf(`[{"op": "replace", "path": "/metadata/name", "value": "%s"}]`, DeploymentNamespace())).
+		CreateApp().
+		Sync().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		When().
+		PatchApp(`[{
+				"op": "add",
+				"path": "/spec/syncPolicy",
+				"value": { "prune": true, "syncOptions": ["PrunePropagationPolicy=foreground"], "managedNamespaceMetadata": { "labels": { "foo": "bar" } } }
+				}]`).
+		Sync().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		And(func(app *Application) {
+			assert.Equal(t, &ManagedNamespaceMetadata{Labels: map[string]string{"foo": "bar"}}, app.Spec.SyncPolicy.ManagedNamespaceMetadata)
+		}).
+		When().
+		DeleteFile("guestbook-ui-namespace.yaml").
+		Refresh(RefreshTypeHard).
+		Sync().
+		Wait().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced))
+}
+
 func TestSelfManagedApps(t *testing.T) {
 
 	Given(t).

test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-deployment.yaml

@@ -0,0 +1,23 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: guestbook-ui
+  labels:
+    test: "true"
+spec:
+  replicas: 0
+  revisionHistoryLimit: 3
+  selector:
+    matchLabels:
+      app: guestbook-ui
+  template:
+    metadata:
+      labels:
+        app: guestbook-ui
+    spec:
+      containers:
+      - image: quay.io/argoprojlabs/argocd-e2e-container:0.2
+        imagePullPolicy: IfNotPresent
+        name: guestbook-ui
+        ports:
+        - containerPort: 80

test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-namespace.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: guestbook-ui-with-namespace-manifest
+  labels:
+    test: "true"
+  annotations:
+    foo: bar
+    something: else

test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-svc.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: guestbook-ui
+spec:
+  ports:
+  - port: 80
+    targetPort: 80
+  selector:
+    app: guestbook-ui