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]>

pkg/sync/reconcile.go

@@ -76,18 +76,39 @@ 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 = ""
 		}
-		key := kubeutil.NewResourceKey(gvk.Group, gvk.Kind, ns, obj.GetName())
-		if liveObj, ok := liveObjByKey[key]; ok {
-			managedLiveObj[i] = liveObj
-			delete(liveObjByKey, key)
-		} else {
+
+		keys := []kubeutil.ResourceKey{
+			kubeutil.NewResourceKey(gvk.Group, gvk.Kind, ns, obj.GetName()),
+		}
+		if ns != "" {
+			// If IsNamespacedOrUnknown returned namespaced=true because it's actually unknown, there is a chance
+			// that we incorrectly use a non-empty value for ns for a cluster-scoped resource for the resource
+			// key, when it should actually be empty. This happens if the namespace parameter passed in to this
+			// method has a value. Therefore, we check a key both with and without the namespace set. Otherwise,
+			// we risk indicating that the live object does not exist when it in fact does, which then
+			// incorrectly appends a nil to targetObjs and potentially causes data loss.
+			keys = append(keys, kubeutil.NewResourceKey(gvk.Group, gvk.Kind, "", obj.GetName()))
+		}
+
+		found := false
+		for _, key := range keys {
+			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)
+}