fix(clusterCache): don't miss finding live obj if obj is cluster-scoped and namespacedResources is in transition (#597)

* sync.Reconcile: guard against incomplete discovery

When Reconcile performs its logic to compare the desired state (target
objects) against the actual state (live objects), it looks up each live
object based on a key comprised of data from the target object: API
group, API kind, namespace, and name. While group, kind, and name will
always be accurate, there is a chance that the value for namespace is
not. If a cluster-scoped target object has a namespace (because it
incorrectly has a namespace from its source) or the namespace parameter
passed into the Reconcile method has a non-empty value (indicating a
default value to use on namespace-scoped objects that don't have it set
in the source), AND the resInfo ResourceInfoProvider has incomplete or
missing API discovery data, the call to IsNamespacedOrUnknown will
return true when the information is unknown. This leads to the key being
incorrect - it will have a value for namespace when it shouldn't. As a
result, indexing into liveObjByKey will fail. This failure results in
the reconciliation containing incorrect data: there will be a nil entry
appended to targetObjs when there shouldn't be.

Signed-off-by: Andy Goldstein <[email protected]>

* Address code review comments

Signed-off-by: Andy Goldstein <[email protected]>

---------

Signed-off-by: Andy Goldstein <[email protected]>

Author: ncdc

pkg/sync/reconcile.go

@@ -76,18 +76,36 @@ func Reconcile(targetObjs []*unstructured.Unstructured, liveObjByKey map[kube.Re
 	managedLiveObj := make([]*unstructured.Unstructured, len(targetObjs))
 	for i, obj := range targetObjs {
 		gvk := obj.GroupVersionKind()
+
 		ns := text.FirstNonEmpty(obj.GetNamespace(), namespace)
-		if namespaced := kubeutil.IsNamespacedOrUnknown(resInfo, obj.GroupVersionKind().GroupKind()); !namespaced {
-			ns = ""
+
+		namespaced, err := resInfo.IsNamespaced(gvk.GroupKind())
+		unknownScope := err != nil
+
+		var keysToCheck []kubeutil.ResourceKey
+		// If we get an error, we don't know whether the resource is namespaced. So we need to check for both in the
+		// live objects. If we don't check for both, then we risk missing the object and deleting it.
+		if namespaced || unknownScope {
+			keysToCheck = append(keysToCheck, kubeutil.NewResourceKey(gvk.Group, gvk.Kind, ns, obj.GetName()))
 		}
-		key := kubeutil.NewResourceKey(gvk.Group, gvk.Kind, ns, obj.GetName())
-		if liveObj, ok := liveObjByKey[key]; ok {
-			managedLiveObj[i] = liveObj
-			delete(liveObjByKey, key)
-		} else {
+		if !namespaced || unknownScope {
+			keysToCheck = append(keysToCheck, kubeutil.NewResourceKey(gvk.Group, gvk.Kind, "", obj.GetName()))
+		}
+
+		found := false
+		for _, key := range keysToCheck {
+			if liveObj, ok := liveObjByKey[key]; ok {
+				managedLiveObj[i] = liveObj
+				delete(liveObjByKey, key)
+				found = true
+				break
+			}
+		}
+		if !found {
 			managedLiveObj[i] = nil
 		}
 	}
+
 	for _, obj := range liveObjByKey {
 		targetObjs = append(targetObjs, nil)
 		managedLiveObj = append(managedLiveObj, obj)

pkg/sync/reconcile_test.go

@@ -0,0 +1,52 @@
+package sync
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/argoproj/gitops-engine/pkg/utils/kube"
+)
+
+type unknownResourceInfoProvider struct{}
+
+func (e *unknownResourceInfoProvider) IsNamespaced(gk schema.GroupKind) (bool, error) {
+	return false, errors.New("unknown")
+}
+
+func TestReconcileWithUnknownDiscoveryDataForClusterScopedResources(t *testing.T) {
+	targetObjs := []*unstructured.Unstructured{
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Namespace",
+				"metadata": map[string]interface{}{
+					"name": "my-namespace",
+				},
+			},
+		},
+	}
+
+	liveNS := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "Namespace",
+			"metadata": map[string]interface{}{
+				"name": "my-namespace",
+				"uid":  "c99ff56d-1921-495d-8512-d66cdfcb5740",
+			},
+		},
+	}
+	liveObjByKey := map[kube.ResourceKey]*unstructured.Unstructured{
+		kube.NewResourceKey("", "Namespace", "", "my-namespace"): liveNS,
+	}
+
+	result := Reconcile(targetObjs, liveObjByKey, "some-namespace", &unknownResourceInfoProvider{})
+	require.Len(t, result.Target, 1)
+	require.Equal(t, result.Target[0], targetObjs[0])
+	require.Len(t, result.Live, 1)
+	require.Equal(t, result.Live[0], liveNS)
+}