fix: update managed namespace metadata (argoproj#13074)

* fix: update managed namespace metadata This commit fixes an issue where a namespace does not get updated unless a sync is performed. Since the `managedNamespaceMetadata` is not a part of the Application Git state, we need a way to force a sync once the metadata has changed. In order to do that, we need to add state to compare with. Once a sync is performed, the `ManagedNamespaceMetadata` gets copied to `SyncResult`, which will then be compared with on subsequent syncs. If there's a mismatch between `app.Spec.SyncPolicy.ManagedNamespaceMetadata` and `app.Status.OperationState.SyncResult.ManagedNamespaceMetadata` we mark the Application as `OutOfSync`. Fixes argoproj#12661. Signed-off-by: Blake Pettersson <[email protected]> * fix: nil check Signed-off-by: Blake Pettersson <[email protected]> * fix: allow empty apps to be updated If an app is empty but still differs in terms of `managedNamespaceMetadata`, it should still be kept up to date. Signed-off-by: Blake Pettersson <[email protected]> * test: add unit tests in appcontroller Signed-off-by: Blake Pettersson <[email protected]> * fix: rebase Signed-off-by: Blake Pettersson <[email protected]> * refactor: extract method Consolidate checks to `app.HasChangedManagedNamespaceMetadata()` Signed-off-by: Blake Pettersson <[email protected]> * chore: make codegen Signed-off-by: Blake Pettersson <[email protected]> --------- Signed-off-by: Blake Pettersson <[email protected]>
tkasuz · May 19, 2023 · aed1be6 · aed1be6
1 parent f356a54
commit aed1be6
Show file tree

Hide file tree

Showing 16 changed files with 945 additions and 632 deletions.
diff --git a/assets/swagger.json b/assets/swagger.json
@@ -5685,7 +5685,7 @@
     },
     "v1alpha1ApplicationCondition": {
       "type": "object",
-      "title": "ApplicationCondition contains details about an application condition, which is usally an error or warning",
+      "title": "ApplicationCondition contains details about an application condition, which is usually an error or warning",
       "properties": {
         "lastTransitionTime": {
           "$ref": "#/definitions/v1Time"
@@ -8442,6 +8442,9 @@
       "type": "object",
       "title": "SyncOperationResult represent result of sync operation",
       "properties": {
+        "managedNamespaceMetadata": {
+          "$ref": "#/definitions/v1alpha1ManagedNamespaceMetadata"
+        },
         "resources": {
           "type": "array",
           "title": "Resources contains a list of sync result items for each individual resource in a sync operation",

diff --git a/controller/appcontroller.go b/controller/appcontroller.go
@@ -1487,7 +1487,7 @@ func currentSourceEqualsSyncedSource(app *appv1.Application) bool {
 
 // needRefreshAppStatus answers if application status needs to be refreshed.
 // Returns true if application never been compared, has changed or comparison result has expired.
-// Additionally returns whether full refresh was requested or not.
+// Additionally, it returns whether full refresh was requested or not.
 // If full refresh is requested then target and live state should be reconciled, else only live state tree should be updated.
 func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application, statusRefreshTimeout, statusHardRefreshTimeout time.Duration) (bool, appv1.RefreshType, CompareWith) {
 	logCtx := log.WithFields(log.Fields{"application": app.QualifiedName()})
@@ -1524,6 +1524,8 @@ func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application,
 			}
 		} else if !app.Spec.Destination.Equals(app.Status.Sync.ComparedTo.Destination) {
 			reason = "spec.destination differs"
+		} else if app.HasChangedManagedNamespaceMetadata() {
+			reason = "spec.syncPolicy.managedNamespaceMetadata differs"
 		} else if requested, level := ctrl.isRefreshRequested(app.QualifiedName()); requested {
 			compareWith = level
 			reason = "controller refresh requested"
@@ -1729,7 +1731,7 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *
 	return nil
 }
 
-// alreadyAttemptedSync returns whether or not the most recent sync was performed against the
+// alreadyAttemptedSync returns whether the most recent sync was performed against the
 // commitSHA and with the same app source config which are currently set in the app
 func alreadyAttemptedSync(app *appv1.Application, commitSHA string, commitSHAsMS []string, hasMultipleSources bool) (bool, synccommon.OperationPhase) {
 	if app.Status.OperationState == nil || app.Status.OperationState.Operation.Sync == nil || app.Status.OperationState.SyncResult == nil {

diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go
@@ -1040,6 +1040,55 @@ func TestNeedRefreshAppStatus(t *testing.T) {
 	}
 }
 
+func TestUpdatedManagedNamespaceMetadata(t *testing.T) {
+	ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})
+	app := newFakeApp()
+	app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{
+		Labels: map[string]string{
+			"foo": "bar",
+		},
+		Annotations: map[string]string{
+			"foo": "bar",
+		},
+	}
+	app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
+	app.Status.Sync.ComparedTo.Destination = app.Spec.Destination
+
+	// Ensure that hard/soft refresh isn't triggered due to reconciledAt being expired
+	reconciledAt := metav1.NewTime(time.Now().UTC().Add(15 * time.Minute))
+	app.Status.ReconciledAt = &reconciledAt
+	needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 30*time.Minute, 2*time.Hour)
+
+	assert.True(t, needRefresh)
+	assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+	assert.Equal(t, CompareWithLatest, compareWith)
+}
+
+func TestUnchangedManagedNamespaceMetadata(t *testing.T) {
+	ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})
+	app := newFakeApp()
+	app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{
+		Labels: map[string]string{
+			"foo": "bar",
+		},
+		Annotations: map[string]string{
+			"foo": "bar",
+		},
+	}
+	app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
+	app.Status.Sync.ComparedTo.Destination = app.Spec.Destination
+	app.Status.OperationState.SyncResult.ManagedNamespaceMetadata = app.Spec.SyncPolicy.ManagedNamespaceMetadata
+
+	// Ensure that hard/soft refresh isn't triggered due to reconciledAt being expired
+	reconciledAt := metav1.NewTime(time.Now().UTC().Add(15 * time.Minute))
+	app.Status.ReconciledAt = &reconciledAt
+	needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 30*time.Minute, 2*time.Hour)
+
+	assert.False(t, needRefresh)
+	assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+	assert.Equal(t, CompareWithLatest, compareWith)
+}
+
 func TestRefreshAppConditions(t *testing.T) {
 	defaultProj := argoappv1.AppProject{
 		ObjectMeta: metav1.ObjectMeta{

diff --git a/controller/state.go b/controller/state.go
@@ -651,6 +651,8 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
 
 	if failedToLoadObjs {
 		syncCode = v1alpha1.SyncStatusCodeUnknown
+	} else if app.HasChangedManagedNamespaceMetadata() {
+		syncCode = v1alpha1.SyncStatusCodeOutOfSync
 	}
 	var revision string
 

diff --git a/controller/state_test.go b/controller/state_test.go
@@ -51,6 +51,91 @@ func TestCompareAppStateEmpty(t *testing.T) {
 	assert.Len(t, app.Status.Conditions, 0)
 }
 
+// TestCompareAppStateNamespaceMetadataDiffers tests comparison when managed namespace metadata differs
+func TestCompareAppStateNamespaceMetadataDiffers(t *testing.T) {
+	app := newFakeApp()
+	app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{
+		Labels: map[string]string{
+			"foo": "bar",
+		},
+		Annotations: map[string]string{
+			"foo": "bar",
+		},
+	}
+	app.Status.OperationState = &argoappv1.OperationState{
+		SyncResult: &argoappv1.SyncOperationResult{},
+	}
+
+	data := fakeData{
+		manifestResponse: &apiclient.ManifestResponse{
+			Manifests: []string{},
+			Namespace: test.FakeDestNamespace,
+			Server:    test.FakeClusterURL,
+			Revision:  "abc123",
+		},
+		managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
+	}
+	ctrl := newFakeController(&data)
+	sources := make([]argoappv1.ApplicationSource, 0)
+	sources = append(sources, app.Spec.GetSource())
+	revisions := make([]string, 0)
+	revisions = append(revisions, "")
+	compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false)
+	assert.NotNil(t, compRes)
+	assert.NotNil(t, compRes.syncStatus)
+	assert.Equal(t, argoappv1.SyncStatusCodeOutOfSync, compRes.syncStatus.Status)
+	assert.Len(t, compRes.resources, 0)
+	assert.Len(t, compRes.managedResources, 0)
+	assert.Len(t, app.Status.Conditions, 0)
+}
+
+// TestCompareAppStateNamespaceMetadataIsTheSame tests comparison when managed namespace metadata is the same
+func TestCompareAppStateNamespaceMetadataIsTheSame(t *testing.T) {
+	app := newFakeApp()
+	app.Spec.SyncPolicy.ManagedNamespaceMetadata = &argoappv1.ManagedNamespaceMetadata{
+		Labels: map[string]string{
+			"foo": "bar",
+		},
+		Annotations: map[string]string{
+			"foo": "bar",
+		},
+	}
+	app.Status.OperationState = &argoappv1.OperationState{
+		SyncResult: &argoappv1.SyncOperationResult{
+			ManagedNamespaceMetadata: &argoappv1.ManagedNamespaceMetadata{
+				Labels: map[string]string{
+					"foo": "bar",
+				},
+				Annotations: map[string]string{
+					"foo": "bar",
+				},
+			},
+		},
+	}
+
+	data := fakeData{
+		manifestResponse: &apiclient.ManifestResponse{
+			Manifests: []string{},
+			Namespace: test.FakeDestNamespace,
+			Server:    test.FakeClusterURL,
+			Revision:  "abc123",
+		},
+		managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
+	}
+	ctrl := newFakeController(&data)
+	sources := make([]argoappv1.ApplicationSource, 0)
+	sources = append(sources, app.Spec.GetSource())
+	revisions := make([]string, 0)
+	revisions = append(revisions, "")
+	compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, sources, false, false, nil, false)
+	assert.NotNil(t, compRes)
+	assert.NotNil(t, compRes.syncStatus)
+	assert.Equal(t, argoappv1.SyncStatusCodeSynced, compRes.syncStatus.Status)
+	assert.Len(t, compRes.resources, 0)
+	assert.Len(t, compRes.managedResources, 0)
+	assert.Len(t, app.Status.Conditions, 0)
+}
+
 // TestCompareAppStateMissing tests when there is a manifest defined in the repo which doesn't exist in live
 func TestCompareAppStateMissing(t *testing.T) {
 	app := newFakeApp()

diff --git a/controller/sync.go b/controller/sync.go
@@ -323,6 +323,10 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 	state.Phase, state.Message, resState = syncCtx.GetState()
 	state.SyncResult.Resources = nil
 
+	if app.Spec.SyncPolicy != nil {
+		state.SyncResult.ManagedNamespaceMetadata = app.Spec.SyncPolicy.ManagedNamespaceMetadata
+	}
+
 	var apiVersion []kube.APIResourceInfo
 	for _, res := range resState {
 		augmentedMsg, err := argo.AugmentSyncMsg(res, func() ([]kube.APIResourceInfo, error) {

diff --git a/controller/sync_test.go b/controller/sync_test.go
@@ -59,6 +59,46 @@ func TestPersistRevisionHistory(t *testing.T) {
 	assert.Equal(t, "abc123", updatedApp.Status.History[0].Revision)
 }
 
+func TestPersistManagedNamespaceMetadataState(t *testing.T) {
+	app := newFakeApp()
+	app.Status.OperationState = nil
+	app.Status.History = nil
+	app.Spec.SyncPolicy.ManagedNamespaceMetadata = &v1alpha1.ManagedNamespaceMetadata{
+		Labels: map[string]string{
+			"foo": "bar",
+		},
+		Annotations: map[string]string{
+			"foo": "bar",
+		},
+	}
+
+	defaultProject := &v1alpha1.AppProject{
+		ObjectMeta: v1.ObjectMeta{
+			Namespace: test.FakeArgoCDNamespace,
+			Name:      "default",
+		},
+	}
+	data := fakeData{
+		apps: []runtime.Object{app, defaultProject},
+		manifestResponse: &apiclient.ManifestResponse{
+			Manifests: []string{},
+			Namespace: test.FakeDestNamespace,
+			Server:    test.FakeClusterURL,
+			Revision:  "abc123",
+		},
+		managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
+	}
+	ctrl := newFakeController(&data)
+
+	// Sync with source unspecified
+	opState := &v1alpha1.OperationState{Operation: v1alpha1.Operation{
+		Sync: &v1alpha1.SyncOperation{},
+	}}
+	ctrl.appStateManager.SyncAppState(app, opState)
+	// Ensure we record spec.syncPolicy.managedNamespaceMetadata into sync result
+	assert.Equal(t, app.Spec.SyncPolicy.ManagedNamespaceMetadata, opState.SyncResult.ManagedNamespaceMetadata)
+}
+
 func TestPersistRevisionHistoryRollback(t *testing.T) {
 	app := newFakeApp()
 	app.Status.OperationState = nil

diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml
@@ -1492,7 +1492,7 @@ spec:
                   conditions
                 items:
                   description: ApplicationCondition contains details about an application
-                    condition, which is usally an error or warning
+                    condition, which is usually an error or warning
                   properties:
                     lastTransitionTime:
                       description: LastTransitionTime is the time the condition was
@@ -2941,6 +2941,19 @@ spec:
                   syncResult:
                     description: SyncResult is the result of a Sync operation
                     properties:
+                      managedNamespaceMetadata:
+                        description: ManagedNamespaceMetadata contains the current
+                          sync state of managed namespace metadata
+                        properties:
+                          annotations:
+                            additionalProperties:
+                              type: string
+                            type: object
+                          labels:
+                            additionalProperties:
+                              type: string
+                            type: object
+                        type: object
                       resources:
                         description: Resources contains a list of sync result items
                           for each individual resource in a sync operation

diff --git a/manifests/crds/application-crd.yaml b/manifests/crds/application-crd.yaml
@@ -1491,7 +1491,7 @@ spec:
                   conditions
                 items:
                   description: ApplicationCondition contains details about an application
-                    condition, which is usally an error or warning
+                    condition, which is usually an error or warning
                   properties:
                     lastTransitionTime:
                       description: LastTransitionTime is the time the condition was
@@ -2940,6 +2940,19 @@ spec:
                   syncResult:
                     description: SyncResult is the result of a Sync operation
                     properties:
+                      managedNamespaceMetadata:
+                        description: ManagedNamespaceMetadata contains the current
+                          sync state of managed namespace metadata
+                        properties:
+                          annotations:
+                            additionalProperties:
+                              type: string
+                            type: object
+                          labels:
+                            additionalProperties:
+                              type: string
+                            type: object
+                        type: object
                       resources:
                         description: Resources contains a list of sync result items
                           for each individual resource in a sync operation

diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml
@@ -1492,7 +1492,7 @@ spec:
                   conditions
                 items:
                   description: ApplicationCondition contains details about an application
-                    condition, which is usally an error or warning
+                    condition, which is usually an error or warning
                   properties:
                     lastTransitionTime:
                       description: LastTransitionTime is the time the condition was
@@ -2941,6 +2941,19 @@ spec:
                   syncResult:
                     description: SyncResult is the result of a Sync operation
                     properties:
+                      managedNamespaceMetadata:
+                        description: ManagedNamespaceMetadata contains the current
+                          sync state of managed namespace metadata
+                        properties:
+                          annotations:
+                            additionalProperties:
+                              type: string
+                            type: object
+                          labels:
+                            additionalProperties:
+                              type: string
+                            type: object
+                        type: object
                       resources:
                         description: Resources contains a list of sync result items
                           for each individual resource in a sync operation

diff --git a/manifests/install.yaml b/manifests/install.yaml
@@ -1492,7 +1492,7 @@ spec:
                   conditions
                 items:
                   description: ApplicationCondition contains details about an application
-                    condition, which is usally an error or warning
+                    condition, which is usually an error or warning
                   properties:
                     lastTransitionTime:
                       description: LastTransitionTime is the time the condition was
@@ -2941,6 +2941,19 @@ spec:
                   syncResult:
                     description: SyncResult is the result of a Sync operation
                     properties:
+                      managedNamespaceMetadata:
+                        description: ManagedNamespaceMetadata contains the current
+                          sync state of managed namespace metadata
+                        properties:
+                          annotations:
+                            additionalProperties:
+                              type: string
+                            type: object
+                          labels:
+                            additionalProperties:
+                              type: string
+                            type: object
+                        type: object
                       resources:
                         description: Resources contains a list of sync result items
                           for each individual resource in a sync operation