WithFinalizer (#228)

WithFinalizer is a SubReconciler that allows external state to be
allocated and then cleaned up once the parent resource is deleted. When
the parent resource is not terminating, the finalizer is set on the
parent resource before the nested reconciler is called. When the parent
resource is terminating, the finalizer is cleared only after the nested
reconciler returns without an error.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>

README.md

@@ -17,6 +17,7 @@
 		- [CastParent](#castparent)
 		- [Sequence](#sequence)
 		- [WithConfig](#withconfig)
+		- [WithFinalizer](#withfinalizer)
 - [Testing](#testing)
 	- [ReconcilerTestSuite](#reconcilertestsuite)
 	- [SubReconcilerTestSuite](#subreconcilertestsuite)
@@ -342,6 +343,36 @@ func SwapRESTConfig(rc *rest.Config) *reconcilers.SubReconciler {
 }
 ```
 
+#### WithFinalizer
+
+[`WithFinalizer`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#WithFinalizer) allows external state to be allocated and then cleaned up once the parent resource is deleted. When the parent resource is not terminating, the finalizer is set on the parent resource before the nested reconciler is called. When the parent resource is terminating, the finalizer is cleared only after the nested reconciler returns without an error.
+
+The [Finalizers](#finalizers) utilities are used to manage the finalizer on the parent resource.
+
+> Warning: It is crucial that each WithFinalizer have a unique and stable finalizer name. Two reconcilers that use the same finalizer, or a reconciler that changed the name of its finalizer, may leak the external state when the parent is deleted, or the parent resource may never terminate.
+
+**Example:**
+
+`WithFinalizer` can be used to wrap any other [SubReconciler](#subreconciler), which can then safely allocate external state while the parent resource is not terminating, and then cleanup that state once the parent resource is terminating.
+
+```go
+func SyncExternalState() *reconcilers.SubReconciler {
+	return &reconcilers.WithFinalizer{
+		Finalizer: "unique.finalizer.name"
+		Reconciler: &reconcilers.SyncReconciler{
+			Sync: func(ctx context.Context, parent *resources.TestResource) error {
+				// allocate external state
+				return nil
+			},
+			Finalize: func(ctx context.Context, parent *resources.TestResource) error {
+				// cleanup the external state
+				return nil
+			},
+		},
+	}
+}
+```
+
 ## Testing
 
 While `controller-runtime` focuses its testing efforts on integration testing by spinning up a new API Server and etcd, `reconciler-runtime` focuses on unit testing reconcilers. The state for each test case is pure, preventing side effects from one test case impacting the next.
@@ -583,6 +614,10 @@ type MyResource struct {
 
 Deleting a resource that uses finalizers requires the controller to be running.
 
+> Note: [WithFinalizer](#withfinalizer) can be used in lieu of, or in conjunction with, [ChildReconciler](#childreconciler)#Finalizer. The distinction is the scope within the reconciler tree where a finalizer is applied. While a reconciler can define as many finalizer on the resource as it desires, in practice, it's best to minimize the number of finalizers as setting and clearing each finalizer makes a request to the API Server. 
+>
+> A single WithFinalizer will always add a finalizer to the parent resource. It can then compose multiple ChildReconcilers, as well as other reconcilers that do not natively support managing finalizers (e.g. SyncReconciler). On the other hand, the ChildReconciler will only set the finalizer when it is required potentially reducing the number of finalizers, but only covers that exact sub-reconciler. It's important the external state that needs to be cleaned up be covered by a finalizer, it does not matter which finalizer is used.
+
 The [AddParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#AddParentFinalizer) and [ClearParentFinalizer](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#ClearParentFinalizer) functions patch the parent resource to update its finalizers. These methods work with [CastParents](#castparent) resources and use the same client the [ParentReconciler](#parentreconciler) used to originally load the parent resource. They can be called inside [SubReconcilers](#subreconciler) that may use a different client.
 
 When an update is required, only the `.metadata.finalizers` field is patched. The parent's `.metadata.resourceVersion` is used as an optimistic concurrency lock, and is updated with the value returned from the server. Any error from the server will cause the resource reconciliation to err. When testing with the [SubReconcilerTestSuite](#subreconcilertestsuite), the resource version of the parent defaults to `"999"`, the patch bytes include the resource version and the response increments the parent's resource version. For a parent with the default resource version that patches a finalizer, the expected parent will have a resource version of `"1000"`.

reconcilers/reconcilers.go

@@ -1260,6 +1260,64 @@ func (r *WithConfig) Reconcile(ctx context.Context, parent client.Object) (ctrl.
 	return r.Reconciler.Reconcile(ctx, parent)
 }
 
+// WithFinalizer ensures the resource being reconciled has the desired finalizer set so that state
+// can be cleaned up upon the resource being deleted. The finalizer is added to the resource, if not
+// already set, before calling the nested reconciler. When the resource is terminating, the
+// finalizer is cleared after returning from the nested reconciler without error.
+type WithFinalizer struct {
+	// Finalizer to set on the parent resource. The value must be unique to this specific
+	// reconciler instance and not shared. Reusing a value may result in orphaned state when
+	// the parent resource is deleted.
+	//
+	// Using a finalizer is encouraged when state needs to be manually cleaned up before a resource
+	// is fully deleted. This commonly include state allocated outside of the current cluster.
+	Finalizer string
+
+	// Reconciler is called for each reconciler request with the parent
+	// resource being reconciled. Typically a Sequence is used to compose
+	// multiple SubReconcilers.
+	Reconciler SubReconciler
+}
+
+func (r *WithFinalizer) SetupWithManager(ctx context.Context, mgr ctrl.Manager, bldr *builder.Builder) error {
+	if err := r.validate(ctx); err != nil {
+		return err
+	}
+	return r.Reconciler.SetupWithManager(ctx, mgr, bldr)
+}
+
+func (r *WithFinalizer) validate(ctx context.Context) error {
+	// validate Finalizer value
+	if r.Finalizer == "" {
+		return fmt.Errorf("Finalizer must be defined")
+	}
+
+	// validate Reconciler value
+	if r.Reconciler == nil {
+		return fmt.Errorf("Reconciler must be defined")
+	}
+
+	return nil
+}
+
+func (r *WithFinalizer) Reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) {
+	if parent.GetDeletionTimestamp() == nil {
+		if err := AddParentFinalizer(ctx, parent, r.Finalizer); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+	result, err := r.Reconciler.Reconcile(ctx, parent)
+	if err != nil {
+		return result, err
+	}
+	if parent.GetDeletionTimestamp() != nil {
+		if err := ClearParentFinalizer(ctx, parent, r.Finalizer); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+	return result, err
+}
+
 func typeName(i interface{}) string {
 	t := reflect.TypeOf(i)
 	// TODO do we need this?

reconcilers/reconcilers_test.go

@@ -1889,3 +1889,167 @@ func TestWithConfig(t *testing.T) {
 		return rtc.Metadata["SubReconciler"].(func(*testing.T, reconcilers.Config) reconcilers.SubReconciler)(t, c)
 	})
 }
+
+func TestWithFinalizer(t *testing.T) {
+	testNamespace := "test-namespace"
+	testName := "test-resource"
+	testFinalizer := "test-finalizer"
+
+	now := &metav1.Time{Time: time.Now().Truncate(time.Second)}
+
+	scheme := runtime.NewScheme()
+	_ = resources.AddToScheme(scheme)
+
+	resource := dies.TestResourceBlank.
+		MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+			d.Namespace(testNamespace)
+			d.Name(testName)
+			d.CreationTimestamp(metav1.NewTime(time.UnixMilli(1000)))
+		})
+
+	rts := rtesting.SubReconcilerTestSuite{{
+		Name: "in sync",
+		Parent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.Finalizers(testFinalizer)
+			}),
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Sync", ""),
+		},
+	}, {
+		Name:   "add finalizer",
+		Parent: resource,
+		ExpectParent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.Finalizers(testFinalizer)
+				d.ResourceVersion("1000")
+			}),
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched",
+				`Patched finalizer %q`, testFinalizer),
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Sync", ""),
+		},
+		ExpectPatches: []rtesting.PatchRef{
+			{
+				Group:     "testing.reconciler.runtime",
+				Kind:      "TestResource",
+				Namespace: testNamespace,
+				Name:      testName,
+				PatchType: types.MergePatchType,
+				Patch:     []byte(`{"metadata":{"finalizers":["test-finalizer"],"resourceVersion":"999"}}`),
+			},
+		},
+	}, {
+		Name:   "error adding finalizer",
+		Parent: resource,
+		WithReactors: []rtesting.ReactionFunc{
+			rtesting.InduceFailure("patch", "TestResource"),
+		},
+		ShouldErr: true,
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "FinalizerPatchFailed",
+				`Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer),
+		},
+		ExpectPatches: []rtesting.PatchRef{
+			{
+				Group:     "testing.reconciler.runtime",
+				Kind:      "TestResource",
+				Namespace: testNamespace,
+				Name:      testName,
+				PatchType: types.MergePatchType,
+				Patch:     []byte(`{"metadata":{"finalizers":["test-finalizer"],"resourceVersion":"999"}}`),
+			},
+		},
+	}, {
+		Name: "clear finalizer",
+		Parent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.DeletionTimestamp(now)
+				d.Finalizers(testFinalizer)
+			}),
+		ExpectParent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.DeletionTimestamp(now)
+				d.ResourceVersion("1000")
+			}),
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Finalize", ""),
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched",
+				`Patched finalizer %q`, testFinalizer),
+		},
+		ExpectPatches: []rtesting.PatchRef{
+			{
+				Group:     "testing.reconciler.runtime",
+				Kind:      "TestResource",
+				Namespace: testNamespace,
+				Name:      testName,
+				PatchType: types.MergePatchType,
+				Patch:     []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`),
+			},
+		},
+	}, {
+		Name: "error clearing finalizer",
+		Parent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.DeletionTimestamp(now)
+				d.Finalizers(testFinalizer)
+			}),
+		WithReactors: []rtesting.ReactionFunc{
+			rtesting.InduceFailure("patch", "TestResource"),
+		},
+		ShouldErr: true,
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Finalize", ""),
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "FinalizerPatchFailed",
+				`Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer),
+		},
+		ExpectPatches: []rtesting.PatchRef{
+			{
+				Group:     "testing.reconciler.runtime",
+				Kind:      "TestResource",
+				Namespace: testNamespace,
+				Name:      testName,
+				PatchType: types.MergePatchType,
+				Patch:     []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`),
+			},
+		},
+	}, {
+		Name: "keep finalizer on error",
+		Parent: resource.
+			MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+				d.DeletionTimestamp(now)
+				d.Finalizers(testFinalizer)
+			}),
+		ShouldErr: true,
+		ExpectEvents: []rtesting.Event{
+			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Finalize", ""),
+		},
+		Metadata: map[string]interface{}{
+			"FinalizerError": fmt.Errorf("finalize error"),
+		},
+	}}
+
+	rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.SubReconcilerTestCase, c reconcilers.Config) reconcilers.SubReconciler {
+		var syncErr, finalizeErr error
+		if err, ok := rtc.Metadata["SyncError"]; ok {
+			syncErr = err.(error)
+		}
+		if err, ok := rtc.Metadata["FinalizerError"]; ok {
+			finalizeErr = err.(error)
+		}
+
+		return &reconcilers.WithFinalizer{
+			Finalizer: testFinalizer,
+			Reconciler: &reconcilers.SyncReconciler{
+				Sync: func(ctx context.Context, parent *resources.TestResource) error {
+					c.Recorder.Event(resource, corev1.EventTypeNormal, "Sync", "")
+					return syncErr
+				},
+				Finalize: func(ctx context.Context, parent *resources.TestResource) error {
+					c.Recorder.Event(resource, corev1.EventTypeNormal, "Finalize", "")
+					return finalizeErr
+				},
+			},
+		}
+	})
+}

reconcilers/reconcilers_validate_test.go

@@ -896,3 +896,53 @@ func TestWithConfig_validate(t *testing.T) {
 		})
 	}
 }
+
+func TestWithFinalizer_validate(t *testing.T) {
+	tests := []struct {
+		name       string
+		parent     client.Object
+		reconciler *WithFinalizer
+		shouldErr  string
+	}{
+		{
+			name:       "empty",
+			parent:     &corev1.ConfigMap{},
+			reconciler: &WithFinalizer{},
+			shouldErr:  "Finalizer must be defined",
+		},
+		{
+			name:   "valid",
+			parent: &corev1.ConfigMap{},
+			reconciler: &WithFinalizer{
+				Reconciler: &Sequence{},
+				Finalizer:  "my-finalizer",
+			},
+		},
+		{
+			name:   "missing finalizer",
+			parent: &corev1.ConfigMap{},
+			reconciler: &WithFinalizer{
+				Reconciler: &Sequence{},
+			},
+			shouldErr: "Finalizer must be defined",
+		},
+		{
+			name:   "missing reconciler",
+			parent: &corev1.ConfigMap{},
+			reconciler: &WithFinalizer{
+				Finalizer: "my-finalizer",
+			},
+			shouldErr: "Reconciler must be defined",
+		},
+	}
+
+	for _, c := range tests {
+		t.Run(c.name, func(t *testing.T) {
+			ctx := context.TODO()
+			err := c.reconciler.validate(ctx)
+			if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
+				t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
+			}
+		})
+	}
+}