From 7ac1acecdc462a7821beb2f7888f849cc9ed89bb Mon Sep 17 00:00:00 2001 From: Jordan Olshevski Date: Mon, 4 Nov 2024 19:25:02 +0000 Subject: [PATCH] Add only-during-deletion --- docs/advanced-synthesis.md | 17 +++++ .../controllers/reconciliation/controller.go | 6 ++ .../controllers/reconciliation/patch_test.go | 75 +++++++++++++------ internal/resource/resource.go | 25 ++++--- internal/resource/resource_test.go | 28 +++++++ 5 files changed, 119 insertions(+), 32 deletions(-) diff --git a/docs/advanced-synthesis.md b/docs/advanced-synthesis.md index 42ba913a..593a7b30 100644 --- a/docs/advanced-synthesis.md +++ b/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" } +``` diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index ebf39f7e..c0467b23 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/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 { diff --git a/internal/controllers/reconciliation/patch_test.go b/internal/controllers/reconciliation/patch_test.go index 2cbc4fa9..4435d709 100644 --- a/internal/controllers/reconciliation/patch_test.go +++ b/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)) + }) +} diff --git a/internal/resource/resource.go b/internal/resource/resource.go index 8987cfce..1dcf1b71 100644 --- a/internal/resource/resource.go +++ b/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 diff --git a/internal/resource/resource_test.go b/internal/resource/resource_test.go index 9e4ffb88..356c499f 100644 --- a/internal/resource/resource_test.go +++ b/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: `{