diff --git a/controller/state.go b/controller/state.go index a2765f249bc82..155d02ea1b621 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" @@ -335,6 +336,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. @@ -494,6 +499,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) + } + } } } @@ -588,12 +622,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 diff --git a/controller/state_test.go b/controller/state_test.go index 537c0208e734b..8b18a5e3ca787 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -433,6 +433,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", diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 00c5cbf549661..2a4c7d1461ef5 100644 --- a/test/e2e/app_management_test.go +++ b/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). diff --git a/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-deployment.yaml b/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-deployment.yaml new file mode 100644 index 0000000000000..bf3375672f70c --- /dev/null +++ b/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 diff --git a/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-namespace.yaml b/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-namespace.yaml new file mode 100644 index 0000000000000..ceda849716ccb --- /dev/null +++ b/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 diff --git a/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-svc.yaml b/test/e2e/testdata/guestbook-with-plain-namespace-manifest/guestbook-ui-svc.yaml new file mode 100644 index 0000000000000..e8a4a27fbae40 --- /dev/null +++ b/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