Optionally block patches until composition deletion

docs/advanced-synthesis.md

@@ -55,3 +55,20 @@ patch:
   ops:
     - { "op": "add", "path": "/metadata/deletionTimestamp", "value": "anything" }
 ```
+
+Used alongside the `only-during-deletion` annotation, resources can be easily cleaned up when removing the controller that created them.
+
+```yaml
+apiVersion: eno.azure.io/v1
+kind: Patch
+metadata:
+  name: resource-cleanup
+  namespace: default
+  annotations:
+    eno.azure.io/only-during-deletion: "true"
+patch:
+  apiVersion: v1
+  kind: ConfigMap
+  ops:
+    - { "op": "add", "path": "/metadata/deletionTimestamp", "value": "anything" }
+```

internal/controllers/reconciliation/controller.go

@@ -115,6 +115,12 @@ func (c *Controller) Reconcile(ctx context.Context, req *reconstitution.Request)
 	logger = logger.WithValues("resourceKind", resource.Ref.Kind, "resourceName", resource.Ref.Name, "resourceNamespace", resource.Ref.Namespace)
 	ctx = logr.NewContext(ctx, logger)
 
+	// Patches can be deferred until composition deletion
+	if resource.Patch != nil && resource.OnlyDuringDeletion && comp.DeletionTimestamp == nil {
+		c.writeBuffer.PatchStatusAsync(ctx, &resource.ManifestRef, patchResourceState(false, ptr.To(metav1.Now())))
+		return ctrl.Result{}, nil
+	}
+
 	// Keep track of the last reconciliation time and report on it relative to the resource's reconcile interval
 	// This is useful for identifying cases where the loop can't keep up
 	if resource.ReconcileInterval != nil {

internal/controllers/reconciliation/patch_test.go

@@ -6,23 +6,17 @@ import (
 	"time"
 
 	apiv1 "github.com/Azure/eno/api/v1"
-	testv1 "github.com/Azure/eno/internal/controllers/reconciliation/fixtures/v1"
 	"github.com/Azure/eno/internal/testutil"
 	krmv1 "github.com/Azure/eno/pkg/krm/functions/api/v1"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // TestPatchCreation proves that a patch resource will not be created if it doesn't exist.
 func TestPatchCreation(t *testing.T) {
-	scheme := runtime.NewScheme()
-	corev1.SchemeBuilder.AddToScheme(scheme)
-	testv1.SchemeBuilder.AddToScheme(scheme)
-
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)
 	upstream := mgr.GetClient()
@@ -79,10 +73,6 @@ func TestPatchCreation(t *testing.T) {
 
 // TestPatchDeletion proves that a patch resource can delete the resource it references.
 func TestPatchDeletion(t *testing.T) {
-	scheme := runtime.NewScheme()
-	corev1.SchemeBuilder.AddToScheme(scheme)
-	testv1.SchemeBuilder.AddToScheme(scheme)
-
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)
 	upstream := mgr.GetClient()
@@ -132,10 +122,6 @@ func TestPatchDeletion(t *testing.T) {
 // TestPatchDeletionBeforeCreation proves that a patch resource can delete the resource it references before the resource is created.
 // Basically, this is the same behavior as Helm hook event with delete policy "before-hook-creation".
 func TestPatchDeletionBeforeCreation(t *testing.T) {
-	scheme := runtime.NewScheme()
-	corev1.SchemeBuilder.AddToScheme(scheme)
-	testv1.SchemeBuilder.AddToScheme(scheme)
-
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)
 	upstream := mgr.GetClient()
@@ -211,10 +197,6 @@ func TestPatchDeletionBeforeCreation(t *testing.T) {
 // TestPatchDeletionBeforeUpgrade proves that a patch resource can delete the resource it references before the resource is upgraded.
 // Basically, this is the same behavior as Helm hook event with delete policy "before-hook-creation".
 func TestPatchDeletionBeforeUpgrade(t *testing.T) {
-	scheme := runtime.NewScheme()
-	corev1.SchemeBuilder.AddToScheme(scheme)
-	testv1.SchemeBuilder.AddToScheme(scheme)
-
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)
 	upstream := mgr.GetClient()
@@ -304,10 +286,6 @@ func TestPatchDeletionBeforeUpgrade(t *testing.T) {
 // TestPatchDeletionForResourceWithReconciliationFromInput proves that a patch resource won't be triggered to
 // delete the resource with reconcile-interval it references if the patch with lower readiness group
 func TestPatchDeletionForResourceWithReconciliationFromInput(t *testing.T) {
-	scheme := runtime.NewScheme()
-	corev1.SchemeBuilder.AddToScheme(scheme)
-	testv1.SchemeBuilder.AddToScheme(scheme)
-
 	ctx := testutil.NewContext(t)
 	mgr := testutil.NewManager(t)
 	upstream := mgr.GetClient()
@@ -387,3 +365,56 @@ func TestPatchDeletionForResourceWithReconciliationFromInput(t *testing.T) {
 	// Verify the configmap is not re-created.
 	require.Equal(t, uid, newUID)
 }
+
+// TestCleanupPatch proves that deletion patches can be used to cleanup unmanaged resources after the composition is deleted.
+func TestCleanupPatch(t *testing.T) {
+	ctx := testutil.NewContext(t)
+	mgr := testutil.NewManager(t)
+	upstream := mgr.GetClient()
+	downstream := mgr.DownstreamClient
+
+	registerControllers(t, mgr)
+	testutil.WithFakeExecutor(t, mgr, func(ctx context.Context, s *apiv1.Synthesizer, input *krmv1.ResourceList) (*krmv1.ResourceList, error) {
+		obj := &unstructured.Unstructured{
+			Object: map[string]any{
+				"apiVersion": "eno.azure.io/v1",
+				"kind":       "Patch",
+				"metadata": map[string]any{
+					"name":        "test-obj",
+					"namespace":   "default",
+					"annotations": map[string]any{"eno.azure.io/only-during-deletion": "true"},
+				},
+				"patch": map[string]any{
+					"apiVersion": "v1",
+					"kind":       "ConfigMap",
+					"ops": []map[string]any{
+						{"op": "add", "path": "/metadata/deletionTimestamp", "value": "2024-01-22T19:13:15Z"},
+					},
+				},
+			},
+		}
+		return &krmv1.ResourceList{Items: []*unstructured.Unstructured{obj}}, nil
+	})
+
+	cm := &corev1.ConfigMap{}
+	cm.Name = "test-obj"
+	cm.Namespace = "default"
+	require.NoError(t, downstream.Create(ctx, cm))
+
+	setupTestSubject(t, mgr)
+	mgr.Start(t)
+	_, comp := writeGenericComposition(t, upstream)
+
+	// The cm should still exist after the composition becomes ready
+	testutil.Eventually(t, func() bool {
+		err := upstream.Get(ctx, client.ObjectKeyFromObject(comp), comp)
+		return err == nil && comp.Status.CurrentSynthesis != nil && comp.Status.CurrentSynthesis.Ready != nil
+	})
+	require.NoError(t, downstream.Get(ctx, client.ObjectKeyFromObject(cm), cm))
+
+	// The cm should be deleted when the composition is deleted
+	require.NoError(t, upstream.Delete(ctx, comp))
+	testutil.Eventually(t, func() bool {
+		return errors.IsNotFound(downstream.Get(ctx, client.ObjectKeyFromObject(cm), cm))
+	})
+}

internal/resource/resource.go

@@ -44,16 +44,17 @@ type Resource struct {
 	lastSeenMeta
 	lastReconciledMeta
 
-	Ref               Ref
-	Manifest          *apiv1.Manifest
-	ManifestRef       ManifestRef
-	ReconcileInterval *metav1.Duration
-	GVK               schema.GroupVersionKind
-	SliceDeleted      bool
-	ReadinessChecks   readiness.Checks
-	Patch             jsonpatch.Patch
-	DisableUpdates    bool
-	ReadinessGroup    int
+	Ref                Ref
+	Manifest           *apiv1.Manifest
+	ManifestRef        ManifestRef
+	ReconcileInterval  *metav1.Duration
+	GVK                schema.GroupVersionKind
+	SliceDeleted       bool
+	ReadinessChecks    readiness.Checks
+	Patch              jsonpatch.Patch
+	OnlyDuringDeletion bool
+	DisableUpdates     bool
+	ReadinessGroup     int
 
 	// DefinedGroupKind is set on CRDs to represent the resource type they define.
 	DefinedGroupKind *schema.GroupKind
@@ -202,6 +203,10 @@ func NewResource(ctx context.Context, renv *readiness.Env, slice *apiv1.Resource
 	res.ReadinessGroup = int(rg)
 	delete(anno, readinessGroupKey)
 
+	const onlyDuringDeletionKey = "eno.azure.io/only-during-deletion"
+	res.OnlyDuringDeletion = res.Patch != nil && anno[onlyDuringDeletionKey] == "true"
+	delete(anno, onlyDuringDeletionKey)
+
 	for key, value := range anno {
 		if !strings.HasPrefix(key, "eno.azure.io/readiness") {
 			continue

internal/resource/resource_test.go

@@ -31,6 +31,7 @@ var newResourceTests = []struct {
 					"eno.azure.io/readiness-group": "250",
 					"eno.azure.io/readiness": "true",
 					"eno.azure.io/readiness-test": "false",
+					"eno.azure.io/only-during-deletion": "true",
 					"eno.azure.io/disable-updates": "true"
 				}
 			}
@@ -123,6 +124,7 @@ var newResourceTests = []struct {
 		Assert: func(t *testing.T, r *Resource) {
 			assert.Equal(t, schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, r.GVK)
 			assert.Len(t, r.Patch, 1)
+			assert.False(t, r.OnlyDuringDeletion)
 
 			cm := &unstructured.Unstructured{Object: map[string]any{
 				"apiVersion": "v1",
@@ -137,6 +139,32 @@ var newResourceTests = []struct {
 			assert.False(t, r.NeedsToBePatched(cm))
 		},
 	},
+	{
+		Name: "patch-only-during-deletion",
+		Manifest: `{
+			"apiVersion": "eno.azure.io/v1",
+			"kind": "Patch",
+			"metadata": {
+				"name": "foo",
+				"namespace": "bar",
+				"annotations": {
+				  "eno.azure.io/only-during-deletion": "true"
+				}
+			},
+			"patch": {
+				"apiVersion": "v1",
+				"kind": "ConfigMap",
+				"ops": [
+					{ "op": "add", "path": "/data/foo", "value": "bar" }
+				]
+			}
+		}`,
+		Assert: func(t *testing.T, r *Resource) {
+			assert.Equal(t, schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, r.GVK)
+			assert.Len(t, r.Patch, 1)
+			assert.True(t, r.OnlyDuringDeletion)
+		},
+	},
 	{
 		Name: "deletionPatch",
 		Manifest: `{