fix(sync): ApplyOutOfSyncOnly=true sync option is not honoured for cluster scoped resources (#765)

* Using live obj to get the resource key if not nil

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Fixed failing unit tests

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Added test case for validating cluster scoped resources with ApplyOutOfSyncOnly=true

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Fixed gofumpt formatting errors

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Corrected unit tests for cluster scoped resources

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Removed unwanted code comments

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Added comments for explaining the reason why ns is set from live object

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

* Added comments in the unit test

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

---------

Signed-off-by: Anand Francis Joseph <anjoseph@redhat.com>

Author: anandf

pkg/sync/sync_context_test.go

@@ -393,10 +393,13 @@ func TestSyncCreateFailure(t *testing.T) {
 func TestSync_ApplyOutOfSyncOnly(t *testing.T) {
 	pod1 := testingutils.NewPod()
 	pod1.SetName("pod-1")
+	pod1.SetNamespace("fake-argocd-ns")
 	pod2 := testingutils.NewPod()
 	pod2.SetName("pod-2")
+	pod2.SetNamespace("fake-argocd-ns")
 	pod3 := testingutils.NewPod()
 	pod3.SetName("pod-3")
+	pod3.SetNamespace("fake-argocd-ns")
 
 	syncCtx := newTestSyncCtx(nil)
 	syncCtx.applyOutOfSyncOnly = true
@@ -506,6 +509,70 @@ func TestSync_ApplyOutOfSyncOnly(t *testing.T) {
 	})
 }
 
+func TestSync_ApplyOutOfSyncOnly_ClusterResources(t *testing.T) {
+	ns1 := testingutils.NewNamespace()
+	ns1.SetName("ns-1")
+	ns1.SetNamespace("")
+
+	ns2 := testingutils.NewNamespace()
+	ns2.SetName("ns-2")
+	ns1.SetNamespace("")
+
+	ns3 := testingutils.NewNamespace()
+	ns3.SetName("ns-3")
+	ns3.SetNamespace("")
+
+	ns2Target := testingutils.NewNamespace()
+	ns2Target.SetName("ns-2")
+	// set namespace for a cluster scoped resource. This is to simulate the behaviour, where the Application's
+	// spec.destination.namespace is set for all resources that does not have a namespace set, irrespective of whether
+	// the resource is cluster scoped or namespace scoped.
+	//
+	// Refer to https://github.com/argoproj/gitops-engine/blob/8007df5f6c5dd78a1a8cef73569468ce4d83682c/pkg/sync/sync_context.go#L827-L833
+	ns2Target.SetNamespace("ns-2")
+
+	syncCtx := newTestSyncCtx(nil, WithResourceModificationChecker(true, diffResultListClusterResource()))
+	syncCtx.applyOutOfSyncOnly = true
+	fakeDisco := syncCtx.disco.(*fakedisco.FakeDiscovery)
+	fakeDisco.Resources = []*metav1.APIResourceList{
+		{
+			GroupVersion: "v1",
+			APIResources: []metav1.APIResource{
+				{Kind: "Namespace", Group: "", Version: "v1", Namespaced: false, Verbs: standardVerbs},
+			},
+		},
+	}
+
+	t.Run("cluster resource with target ns having namespace filled", func(t *testing.T) {
+		syncCtx.resources = groupResources(ReconciliationResult{
+			Live:   []*unstructured.Unstructured{nil, ns2, ns3},
+			Target: []*unstructured.Unstructured{ns1, ns2Target, ns3},
+		})
+
+		syncCtx.Sync()
+		phase, _, resources := syncCtx.GetState()
+		assert.Equal(t, synccommon.OperationSucceeded, phase)
+		assert.Len(t, resources, 1)
+		for _, r := range resources {
+			switch r.ResourceKey.Name {
+			case "ns-1":
+				// ns-1 namespace does not exist yet in the cluster, so it must create it and resource must go to
+				// synced state.
+				assert.Equal(t, synccommon.ResultCodeSynced, r.Status)
+			case "ns-2":
+				// ns-2 namespace already exist and is synced. However, the target resource has metadata.namespace set for
+				// a cluster resource. This namespace must not be synced again, as the object already exists and
+				// a change in namespace for a cluster resource has no meaning and hence must not be treated as an
+				// out-of-sync resource.
+				t.Error("ns-2 should have been skipped, as no change")
+			case "ns-3":
+				// ns-3 namespace exists and there is no change in the target's metadata.namespace value. So it must not try to sync again.
+				t.Error("ns-3 should have been skipped, as no change")
+			}
+		}
+	})
+}
+
 func TestSyncPruneFailure(t *testing.T) {
 	syncCtx := newTestSyncCtx(nil, WithOperationSettings(false, true, false, false))
 	mockKubectl := &kubetest.MockKubectlCmd{
@@ -2357,3 +2424,28 @@ func TestNeedsClientSideApplyMigration(t *testing.T) {
 		})
 	}
 }
+
+func diffResultListClusterResource() *diff.DiffResultList {
+	ns1 := testingutils.NewNamespace()
+	ns1.SetName("ns-1")
+	ns2 := testingutils.NewNamespace()
+	ns2.SetName("ns-2")
+	ns3 := testingutils.NewNamespace()
+	ns3.SetName("ns-3")
+
+	diffResultList := diff.DiffResultList{
+		Modified: true,
+		Diffs:    []diff.DiffResult{},
+	}
+
+	nsBytes, _ := json.Marshal(ns1)
+	diffResultList.Diffs = append(diffResultList.Diffs, diff.DiffResult{NormalizedLive: nsBytes, PredictedLive: nsBytes, Modified: false})
+
+	nsBytes, _ = json.Marshal(ns2)
+	diffResultList.Diffs = append(diffResultList.Diffs, diff.DiffResult{NormalizedLive: nsBytes, PredictedLive: nsBytes, Modified: false})
+
+	nsBytes, _ = json.Marshal(ns3)
+	diffResultList.Diffs = append(diffResultList.Diffs, diff.DiffResult{NormalizedLive: nsBytes, PredictedLive: nsBytes, Modified: false})
+
+	return &diffResultList
+}

pkg/sync/sync_task.go

@@ -148,5 +148,17 @@ func (t *syncTask) deleteOnPhaseFailed() bool {
 }
 
 func (t *syncTask) resourceKey() kube.ResourceKey {
-	return kube.GetResourceKey(t.obj())
+	resourceKey := kube.GetResourceKey(t.obj())
+	if t.liveObj != nil {
+		// t.targetObj has a namespace set for cluster-scoped resources, which causes kube.GetResourceKey()
+		// to produce an incorrect key with the namespace included. For example:
+		// rbac.authorization.k8s.io/ClusterRole/my-namespace/my-cluster-role
+		// instead of the correct rbac.authorization.k8s.io/ClusterRole//my-cluster-role.
+		// To prevent resource lookup issues, we always rely on the namespace of the live object if it is available.
+		// This logic will work for both cluster scoped and namespace scoped resources.
+		//
+		// Refer to https://github.com/argoproj/gitops-engine/blob/8007df5f6c5dd78a1a8cef73569468ce4d83682c/pkg/sync/sync_context.go#L827-L833
+		resourceKey.Namespace = t.liveObj.GetNamespace()
+	}
+	return resourceKey
 }