Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WithFinalizer #228

Merged
merged 2 commits into from May 9, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions README.md
Expand Up @@ -17,6 +17,7 @@
- [CastParent](#castparent)
- [Sequence](#sequence)
- [WithConfig](#withconfig)
- [WithFinalizer](#withfinalizer)
- [Testing](#testing)
- [ReconcilerTestSuite](#reconcilertestsuite)
- [SubReconcilerTestSuite](#subreconcilertestsuite)
Expand Down Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion]: Can you motivate usage of a WithFinalizer subreconciler over a ChildReconciler/SyncRenciler with Finalize?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a note under the finalizers section. We can continue to iterate on it


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.
Expand Down
58 changes: 58 additions & 0 deletions reconcilers/reconcilers.go
Expand Up @@ -1259,6 +1259,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?
Expand Down
164 changes: 164 additions & 0 deletions reconcilers/reconcilers_test.go
Expand Up @@ -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
},
},
}
})
}
50 changes: 50 additions & 0 deletions reconcilers/reconcilers_validate_test.go
Expand Up @@ -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)
}
})
}
}