Fix composition deletion when a resource is missing namespace (#180)

If a resource returned by a synthesizer doesn't have a namespace,
getting its current state will fail, which prevents it from being marked
as deleted, which blocks composition deletion.

Co-authored-by: Jordan Olshevski <[email protected]>

internal/controllers/reconciliation/controller.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"strings"
 	"time"
 
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -150,7 +151,7 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request)
 
 	// Fetch the current resource
 	current, hasChanged, err := c.getCurrent(ctx, resource)
-	if client.IgnoreNotFound(err) != nil {
+	if client.IgnoreNotFound(err) != nil && !isErrMissingNS(err) {
 		return ctrl.Result{}, fmt.Errorf("getting current state: %w", err)
 	}
 
@@ -405,3 +406,14 @@ func patchResourceState(deleted bool, ready *metav1.Time) flowcontrol.StatusPatc
 		}
 	}
 }
+
+// isErrMissingNS returns true when given the client-go error returned by mutating requests that do not include a namespace.
+// Sadly, this error isn't exposed anywhere - it's just a plain string, so we have to do string matching here.
+//
+// https://github.com/kubernetes/kubernetes/blob/9edabd617945cd23111fd46cfc9a09fe37ed194a/staging/src/k8s.io/client-go/rest/request.go#L1048
+func isErrMissingNS(err error) bool {
+	if err == nil {
+		return false
+	}
+	return strings.Contains(err.Error(), "an empty namespace may not be set")
+}

internal/controllers/reconciliation/edgecase_test.go

@@ -0,0 +1,107 @@
+package reconciliation
+
+import (
+	"context"
+	"sync/atomic"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/client-go/util/retry"
+	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	apiv1 "github.com/Azure/eno/api/v1"
+	"github.com/Azure/eno/internal/testutil"
+	krmv1 "github.com/Azure/eno/pkg/krm/functions/api/v1"
+)
+
+// TestMissingNamespace proves that resynthesis is not blocked by resources that lack a namespace.
+func TestMissingNamespace(t *testing.T) {
+	ctx := testutil.NewContext(t)
+	mgr := testutil.NewManager(t)
+	upstream := mgr.GetClient()
+
+	registerControllers(t, mgr)
+	namespace := atomic.Pointer[string]{}
+	testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) {
+		output := &krmv1.ResourceList{}
+		output.Items = []*unstructured.Unstructured{{
+			Object: map[string]any{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]any{
+					"name":      "test-obj",
+					"namespace": namespace.Load(), // bad news
+				},
+			},
+		}}
+		return output, nil
+	})
+
+	// Test subject
+	setupTestSubject(t, mgr)
+	mgr.Start(t)
+	syn, comp := writeGenericComposition(t, upstream)
+
+	// Initial synthesis
+	testutil.Eventually(t, func() bool {
+		err := upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)
+		return err == nil && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil && comp.Status.CurrentSynthesis.ObservedSynthesizerGeneration == syn.Generation
+	})
+
+	// Fixing the namespace should be possible
+	namespace.Store(ptr.To("default"))
+	err := retry.RetryOnConflict(testutil.Backoff, func() error {
+		upstream.Get(ctx, client.ObjectKeyFromObject(syn), syn)
+		syn.Spec.Image = "updated"
+		return upstream.Update(ctx, syn)
+	})
+	require.NoError(t, err)
+
+	testutil.Eventually(t, func() bool {
+		err := upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)
+		return err == nil && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Reconciled != nil && comp.Status.CurrentSynthesis.ObservedSynthesizerGeneration == syn.Generation
+	})
+}
+
+// TestMissingNamespaceDeletion proves that composition deletion is not blocked by resources that lack a namespace.
+func TestMissingNamespaceDeletion(t *testing.T) {
+	ctx := testutil.NewContext(t)
+	mgr := testutil.NewManager(t)
+	upstream := mgr.GetClient()
+
+	registerControllers(t, mgr)
+	testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) {
+		output := &krmv1.ResourceList{}
+		output.Items = []*unstructured.Unstructured{{
+			Object: map[string]any{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+				"metadata": map[string]any{
+					"name":      "test-obj",
+					"namespace": "", // bad news
+				},
+			},
+		}}
+		return output, nil
+	})
+
+	// Test subject
+	setupTestSubject(t, mgr)
+	mgr.Start(t)
+	syn, comp := writeGenericComposition(t, upstream)
+
+	// Initial synthesis
+	testutil.Eventually(t, func() bool {
+		err := upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)
+		return err == nil && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Synthesized != nil && comp.Status.CurrentSynthesis.ObservedSynthesizerGeneration == syn.Generation
+	})
+
+	// Deleting the composition should succeed eventually
+	require.NoError(t, upstream.Delete(ctx, comp))
+	testutil.Eventually(t, func() bool {
+		return errors.IsNotFound(upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp))
+	})
+}