diff --git a/README.md b/README.md index bd2d5c3..e5d82eb 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,7 @@ - [Stash](#stash) - [Tracker](#tracker) - [Status](#status) + - [Finalizers](#finalizers) - [Contributing](#contributing) - [Acknowledgements](#acknowledgements) - [License](#license) @@ -74,6 +75,8 @@ The [`SubReconciler`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runti The [`SyncReconciler`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#SyncReconciler) is the minimal type-aware sub reconciler. It is used to manage a portion of the parent's reconciliation that is custom, or whose behavior is not covered by another sub reconciler type. Common uses include looking up reference data for the reconciliation, or controlling resources that are not kubernetes resources. +When a resource is deleted that has pending finalizers, the Finalize method is called instead of the Sync method. If the SyncDuringFinalization field is true, the Sync method will also by called. If creating state that must be manually cleaned up, it is the users responsibility to define and clear finalizers. Using the [parent finalizer helper methods](#finalizers) is strongly encouraged with working under a [ParentReconciler](#parentreconciler). + **Example:** While sync reconcilers have the ability to do anything a reconciler can do, it's best to keep them focused on a single goal, letting the parent reconciler structure multiple sub reconcilers together. In this case, we use the parent resource and the client to resolve the target image and stash the value on the parent's status. The status is a good place to stash simple values that can be made public. More [advanced forms of stashing](#stash) are also available. Learn more about [status and its contract](#status). @@ -111,6 +114,7 @@ The ChildReconciler is responsible for: - logging the reconcilers activities - recording child mutations and errors for the parent resource - adapting to child resource changes applied by mutating webhooks +- adding and clearing of a finalizer, if specified The implementor is responsible for: - defining the desired resource @@ -119,6 +123,10 @@ The implementor is responsible for: - updating the parent's status from the child - defining the status subresource [according to the contract](#status) +When a finalizer is defined, the parent resource is patched to add the finalizer before creating the child; it is removed after the child is deleted. If the parent resource is pending deletion, the desired child method is not called, and existing children are deleted. + +> Warning: It is crucial that each ChildReconciler that uses a finalizer 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 child resource when the parent is deleted, or the parent resource may never terminate. + **Example:** Now it's time to create the child Image resource that will do the work of building our Function. This reconciler looks more more complex than what we have seen so far, each function on the reconciler provides a focused hook into the lifecycle being orchestrated by the ChildReconciler. @@ -499,6 +507,16 @@ type MyResource struct { } ``` +### Finalizers + +[Finalizers](https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/) allow a reconciler to clean up state for a resource that has been deleted by a client, and not yet fully removed. Resources with finalizers will stay in this terminating state until all finalizers are cleared from the resource. While using the [Kubernetes garbage collector](https://kubernetes.io/docs/concepts/architecture/garbage-collection/) is recommended when possible, finalizer are useful for cases when state exists outside of the same cluster, scope, and namespace of the parent resource that needs to be cleaned up when no longer used. + +Deleting a resource that uses finalizers requires the controller to be running. + +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 method 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 `.metadata.resourceVersion` of the parent is updated based on the value returned from the server. An error from the server will cause the resource reconciliation to err. + ## Contributing The reconciler-runtime project team welcomes contributions from the community. If you wish to contribute code and you have not signed our contributor license agreement (CLA), our bot will update the issue when you open a Pull Request. For any questions about the CLA process, please refer to our [FAQ](https://cla.vmware.com/faq). For more detailed information, refer to [CONTRIBUTING.md](CONTRIBUTING.md). diff --git a/reconcilers/patch.go b/reconcilers/patch.go index 674ae91..f37eefa 100644 --- a/reconcilers/patch.go +++ b/reconcilers/patch.go @@ -62,5 +62,7 @@ func (p *Patch) Apply(rebase client.Object) error { if err != nil { return err } + // reset rebase to its empty value before unmarshaling into it + replaceWithEmpty(rebase) return json.Unmarshal(patchedBytes, rebase) } diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 2b126a0..4ae76ae 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -27,6 +27,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -49,6 +50,10 @@ type Config struct { Log logr.Logger } +func (c Config) IsEmpty() bool { + return c == Config{} +} + // NewConfig creates a Config for a specific API type. Typically passed into a // reconciler. func NewConfig(mgr ctrl.Manager, apiType client.Object, syncPeriod time.Duration) Config { @@ -95,6 +100,7 @@ func (r *ParentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manage WithValues("parentType", gvk(r.Type, r.Config.Scheme())) ctx = logr.NewContext(ctx, log) + ctx = StashParentConfig(ctx, r.Config) ctx = StashParentType(ctx, r.Type) ctx = StashCastParentType(ctx, r.Type) bldr := ctrl.NewControllerManagedBy(mgr).For(r.Type) @@ -111,6 +117,7 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr WithValues("parentType", gvk(r.Type, r.Config.Scheme())) ctx = logr.NewContext(ctx, log) + ctx = StashParentConfig(ctx, r.Config) ctx = StashParentType(ctx, r.Type) ctx = StashCastParentType(ctx, r.Type) originalParent := r.Type.DeepCopyObject().(client.Object) @@ -163,7 +170,8 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) { - if parent.GetDeletionTimestamp() != nil { + if parent.GetDeletionTimestamp() != nil && len(parent.GetFinalizers()) == 0 { + // resource is being deleted and has no pending finalizers, nothing to do return ctrl.Result{}, nil } @@ -226,9 +234,14 @@ func (r *ParentReconciler) syncLastTransitionTime(proposed, original []metav1.Co } } +const parentConfigStashKey StashKey = "reconciler-runtime:parentConfig" const parentTypeStashKey StashKey = "reconciler-runtime:parentType" const castParentTypeStashKey StashKey = "reconciler-runtime:castParentType" +func StashParentConfig(ctx context.Context, parentConfig Config) context.Context { + return context.WithValue(ctx, parentConfigStashKey, parentConfig) +} + func StashParentType(ctx context.Context, parentType client.Object) context.Context { return context.WithValue(ctx, parentTypeStashKey, parentType) } @@ -237,6 +250,14 @@ func StashCastParentType(ctx context.Context, currentType client.Object) context return context.WithValue(ctx, castParentTypeStashKey, currentType) } +func RetrieveParentConfig(ctx context.Context) Config { + value := ctx.Value(parentConfigStashKey) + if parentConfig, ok := value.(Config); ok { + return parentConfig + } + return Config{} +} + func RetrieveParentType(ctx context.Context) client.Object { value := ctx.Value(parentTypeStashKey) if parentType, ok := value.(client.Object); ok { @@ -282,13 +303,30 @@ type SyncReconciler struct { // +optional Setup func(ctx context.Context, mgr ctrl.Manager, bldr *builder.Builder) error - // Sync does whatever work is necessary for the reconciler + // SyncDuringFinalization indicates the the Sync method should be called regardless of whether + SyncDuringFinalization bool + + // Sync does whatever work is necessary for the reconciler. + // + // If SyncDuringFinalization is true this method is called when the resource is pending + // deletion. This is useful if the reconciler is managing reference data. // // Expected function signature: // func(ctx context.Context, parent client.Object) error // func(ctx context.Context, parent client.Object) (ctrl.Result, error) Sync interface{} + // Finalize does whatever work is necessary for the reconciler when the resource is pending + // deletion. If this reconciler sets a finalizer it should do the necessary work to clean up + // state the finalizer represents and then clear the finalizer. + // + // Expected function signature: + // func(ctx context.Context, parent client.Object) error + // func(ctx context.Context, parent client.Object) (ctrl.Result, error) + // + // +optional + Finalize interface{} + Config } @@ -338,6 +376,34 @@ func (r *SyncReconciler) validate(ctx context.Context) error { } } + // validate Finalize function signature: + // nil + // func(ctx context.Context, parent client.Object) error + // func(ctx context.Context, parent client.Object) (ctrl.Result, error) + if r.Finalize != nil { + castParentType := RetrieveCastParentType(ctx) + fn := reflect.TypeOf(r.Finalize) + err := fmt.Errorf("SyncReconciler must implement Finalize: nil | func(context.Context, %s) error | func(context.Context, %s) (ctrl.Result, error), found: %s", reflect.TypeOf(castParentType), reflect.TypeOf(castParentType), fn) + if fn.NumIn() != 2 || + !reflect.TypeOf((*context.Context)(nil)).Elem().AssignableTo(fn.In(0)) || + !reflect.TypeOf(castParentType).AssignableTo(fn.In(1)) { + return err + } + switch fn.NumOut() { + case 1: + if !reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(0)) { + return err + } + case 2: + if !reflect.TypeOf(ctrl.Result{}).AssignableTo(fn.Out(0)) || + !reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(1)) { + return err + } + default: + return err + } + } + return nil } @@ -348,10 +414,24 @@ func (r *SyncReconciler) Reconcile(ctx context.Context, parent client.Object) (c } ctx = logr.NewContext(ctx, log) - result, err := r.sync(ctx, parent) - if err != nil { - log.Error(err, "unable to sync") - return ctrl.Result{}, err + result := ctrl.Result{} + + if parent.GetDeletionTimestamp() == nil || r.SyncDuringFinalization { + syncResult, err := r.sync(ctx, parent) + if err != nil { + log.Error(err, "unable to sync") + return ctrl.Result{}, err + } + result = AggregateResults(result, syncResult) + } + + if parent.GetDeletionTimestamp() != nil { + finalizeResult, err := r.finalize(ctx, parent) + if err != nil { + log.Error(err, "unable to finalize") + return ctrl.Result{}, err + } + result = AggregateResults(result, finalizeResult) } return result, nil @@ -379,6 +459,32 @@ func (r *SyncReconciler) sync(ctx context.Context, parent client.Object) (ctrl.R return result, err } +func (r *SyncReconciler) finalize(ctx context.Context, parent client.Object) (ctrl.Result, error) { + if r.Finalize == nil { + return ctrl.Result{}, nil + } + + fn := reflect.ValueOf(r.Finalize) + out := fn.Call([]reflect.Value{ + reflect.ValueOf(ctx), + reflect.ValueOf(parent), + }) + result := ctrl.Result{} + var err error + switch len(out) { + case 2: + result = out[0].Interface().(ctrl.Result) + if !out[1].IsNil() { + err = out[1].Interface().(error) + } + case 1: + if !out[0].IsNil() { + err = out[0].Interface().(error) + } + } + return result, err +} + var ( OnlyReconcileChildStatus = errors.New("skip reconciler create/update/delete behavior for the child resource, while still reflecting the existing child's status on the parent") ) @@ -415,6 +521,18 @@ type ChildReconciler struct { // PodList is the list type for Pod ChildListType client.ObjectList + // Finalizer is set on the parent resource before a child resource is created, and cleared + // after a child resource is deleted. The value must be unique to this specific reconciler + // instance and not shared. Reusing a value may result in orphaned resources when the parent + // resource is deleted. + // + // Using a finalizer is encouraged when the Kubernetes garbage collector is unable to delete + // the child resource automatically, like when the parent and child are in different + // namespaces, scopes or clusters. + // + // +optional + Finalizer string + // Setup performs initialization on the manager and builder this reconciler // will run with. It's common to setup field indexes and watch resources. // @@ -641,7 +759,7 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) ( // the created child from a previous turn may be slow to appear in the informer cache, but shouldn't appear // on the parent as being not ready. apierr := err.(apierrs.APIStatus) - conflicted := r.ChildType.DeepCopyObject().(client.Object) + conflicted := newEmpty(r.ChildType).(client.Object) _ = r.APIReader.Get(ctx, types.NamespacedName{Namespace: parent.GetNamespace(), Name: apierr.Status().Details.Name}, conflicted) if metav1.IsControlledBy(conflicted, parent) { // skip updating the parent's status, fail and try again @@ -662,8 +780,8 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) ( func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) (client.Object, error) { log := logr.FromContextOrDiscard(ctx) - actual := r.ChildType.DeepCopyObject().(client.Object) - children := r.ChildListType.DeepCopyObject().(client.ObjectList) + actual := newEmpty(r.ChildType).(client.Object) + children := newEmpty(r.ChildListType).(client.ObjectList) if err := r.List(ctx, children, client.InNamespace(parent.GetNamespace())); err != nil { return nil, err } @@ -712,10 +830,18 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) ( } r.Recorder.Eventf(parent, corev1.EventTypeNormal, "Deleted", "Deleted %s %q", typeName(r.ChildType), actual.GetName()) + + if err := ClearParentFinalizer(ctx, parent, r.Finalizer); err != nil { + return nil, err + } } return nil, nil } + if err := AddParentFinalizer(ctx, parent, r.Finalizer); err != nil { + return nil, err + } + // create child if it doesn't exist if actual.GetName() == "" { log.Info("creating child", "child", r.sanitize(desired)) @@ -788,6 +914,11 @@ func (r *ChildReconciler) semanticEquals(a1, a2 client.Object) bool { } func (r *ChildReconciler) desiredChild(ctx context.Context, parent client.Object) (client.Object, error) { + if parent.GetDeletionTimestamp() != nil { + // the parent is pending deletion, cleanup the child resource + return nil, nil + } + fn := reflect.ValueOf(r.DesiredChild) out := fn.Call([]reflect.Value{ reflect.ValueOf(ctx), @@ -913,23 +1044,12 @@ func (r Sequence) Reconcile(ctx context.Context, parent client.Object) (ctrl.Res if err != nil { return ctrl.Result{}, err } - aggregateResult = r.aggregateResult(result, aggregateResult) + aggregateResult = AggregateResults(result, aggregateResult) } return aggregateResult, nil } -func (r Sequence) aggregateResult(result, aggregate ctrl.Result) ctrl.Result { - if result.RequeueAfter != 0 && (aggregate.RequeueAfter == 0 || result.RequeueAfter < aggregate.RequeueAfter) { - aggregate.RequeueAfter = result.RequeueAfter - } - if result.Requeue { - aggregate.Requeue = true - } - - return aggregate -} - // CastParent casts the ParentReconciler's type by projecting the resource data // onto a new struct. Casting the parent resource is useful to create cross // cutting reconcilers that can operate on common portion of multiple parent @@ -1019,7 +1139,7 @@ func (r *CastParent) cast(ctx context.Context, parent client.Object) (context.Co if err != nil { return nil, nil, err } - castParent := r.Type.DeepCopyObject().(client.Object) + castParent := newEmpty(r.Type).(client.Object) err = json.Unmarshal(data, castParent) if err != nil { return nil, nil, err @@ -1052,6 +1172,90 @@ func namespaceName(obj client.Object) types.NamespacedName { } } +// AddParentFinalizer ensures the desired finalizer exists on the parent resource. The client that +// loaded the parent resource is used to patch it with the finalizer if not already set. +func AddParentFinalizer(ctx context.Context, parent client.Object, finalizer string) error { + return ensureParentFinalizer(ctx, parent, finalizer, true) +} + +// ClearParentFinalizer ensures the desired finalizer does not exist on the parent resource. The +// client that loaded the parent resource is used to patch it with the finalizer if set. +func ClearParentFinalizer(ctx context.Context, parent client.Object, finalizer string) error { + return ensureParentFinalizer(ctx, parent, finalizer, false) +} + +func ensureParentFinalizer(ctx context.Context, parent client.Object, finalizer string, add bool) error { + config := RetrieveParentConfig(ctx) + if config.IsEmpty() { + panic(fmt.Errorf("parent config must exist on the context. Check that the context from a ParentReconciler")) + } + parentType := RetrieveParentType(ctx) + if parentType == nil { + panic(fmt.Errorf("parent type must exist on the context. Check that the context from a ParentReconciler")) + } + + if finalizer == "" || controllerutil.ContainsFinalizer(parent, finalizer) == add { + // nothing to do + return nil + } + + // cast the current object back to the parent so scheme-aware, typed client can operate on it + cast := &CastParent{ + Type: parentType, + Reconciler: &SyncReconciler{ + Config: config, + SyncDuringFinalization: true, + Sync: func(ctx context.Context, current client.Object) error { + log := logr.FromContextOrDiscard(ctx) + + desired := current.DeepCopyObject().(client.Object) + if add { + log.Info("adding parent finalizer", "finalizer", finalizer) + controllerutil.AddFinalizer(desired, finalizer) + } else { + log.Info("removing parent finalizer", "finalizer", finalizer) + controllerutil.RemoveFinalizer(desired, finalizer) + } + + patch := client.StrategicMergeFrom(current, client.MergeFromWithOptimisticLock{}) + if err := config.Patch(ctx, desired, patch); err != nil { + log.Error(err, "unable to patch parent finalizers", "finalizer", finalizer) + config.Recorder.Eventf(current, corev1.EventTypeWarning, "FinalizerPatchFailed", + "Failed to patch finalizer %q: %s", finalizer, err) + return err + } + config.Recorder.Eventf(current, corev1.EventTypeNormal, "FinalizerPatched", + "Patched finalizer %q", finalizer) + + // update current object with values from the api server after patching + current.SetFinalizers(desired.GetFinalizers()) + current.SetResourceVersion(desired.GetResourceVersion()) + current.SetGeneration(desired.GetGeneration()) + + return nil + }, + }, + } + + _, err := cast.Reconcile(ctx, parent) + return err +} + +// AggregateResults combines multiple results into a single result. If any result requests +// requeue, the aggregate is requeued. The shortest non-zero requeue after is the aggregate value. +func AggregateResults(results ...ctrl.Result) ctrl.Result { + aggregate := ctrl.Result{} + for _, result := range results { + if result.RequeueAfter != 0 && (aggregate.RequeueAfter == 0 || result.RequeueAfter < aggregate.RequeueAfter) { + aggregate.RequeueAfter = result.RequeueAfter + } + if result.Requeue { + aggregate.Requeue = true + } + } + return aggregate +} + // MergeMaps flattens a sequence of maps into a single map. Keys in latter maps // overwrite previous keys. None of the arguments are mutated. func MergeMaps(maps ...map[string]string) map[string]string { @@ -1063,3 +1267,15 @@ func MergeMaps(maps ...map[string]string) map[string]string { } return out } + +// replaceWithEmpty overwrite the underlying value with it's empty value +func replaceWithEmpty(x interface{}) { + v := reflect.ValueOf(x).Elem() + v.Set(reflect.Zero(v.Type())) +} + +// newEmpty returns a new empty value of the same underlying type, preserving the existing value +func newEmpty(x interface{}) interface{} { + t := reflect.TypeOf(x).Elem() + return reflect.New(t).Interface() +} diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index a0eceea..c25190c 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -302,6 +302,31 @@ func TestParentReconciler(t *testing.T) { } }, }, + }, { + Name: "context has parent config", + Key: testKey, + GivenObjects: []client.Object{ + resource, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + Sync: func(ctx context.Context, parent *resources.TestResource) error { + if config := reconcilers.RetrieveParentConfig(ctx); config != c { + t.Errorf("expected config in context, found %#v", config) + } + if parentType, ok := reconcilers.RetrieveParentType(ctx).(*resources.TestResource); !ok { + t.Errorf("expected parent type not in context, found %#v", parentType) + } + if castParentType, ok := reconcilers.RetrieveCastParentType(ctx).(*resources.TestResource); !ok { + t.Errorf("expected cast parent type not in context, found %#v", castParentType) + } + return nil + }, + } + }, + }, }} rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.ReconcilerTestCase, c reconcilers.Config) reconcile.Reconciler { @@ -317,6 +342,8 @@ func TestSyncReconciler(t *testing.T) { testNamespace := "test-namespace" testName := "test-resource" + now := metav1.Now() + scheme := runtime.NewScheme() _ = resources.AddToScheme(scheme) @@ -385,6 +412,123 @@ func TestSyncReconciler(t *testing.T) { }, }, ShouldPanic: true, + }, { + Name: "should not finalize non-deleted resources", + Parent: resource, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + t.Errorf("reconciler should not call finalize for non-deleted resources") + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "should not finalize deleted resources", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + t.Errorf("reconciler should not call sync for deleted resources") + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 3 * time.Hour}, + }, { + Name: "should finalize and sync deleted resources when asked to", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + SyncDuringFinalization: true, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "should finalize and sync deleted resources when asked to, shorter resync wins", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + SyncDuringFinalization: true, + Sync: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 3 * time.Hour}, nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) (ctrl.Result, error) { + return ctrl.Result{RequeueAfter: 2 * time.Hour}, nil + }, + } + }, + }, + ExpectedResult: reconcile.Result{RequeueAfter: 2 * time.Hour}, + }, { + Name: "finalize is optional", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + Sync: func(ctx context.Context, parent *resources.TestResource) error { + return nil + }, + } + }, + }, + }, { + Name: "finalize error", + Parent: resource. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&now) + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return &reconcilers.SyncReconciler{ + Config: c, + Sync: func(ctx context.Context, parent *resources.TestResource) error { + return nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) error { + return fmt.Errorf("syncreconciler finalize error") + }, + } + }, + }, + ShouldErr: true, }} rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.SubReconcilerTestCase, c reconcilers.Config) reconcilers.SubReconciler { @@ -395,6 +539,7 @@ func TestSyncReconciler(t *testing.T) { func TestChildReconciler(t *testing.T) { testNamespace := "test-namespace" testName := "test-resource" + testFinalizer := "test.finalizer" scheme := runtime.NewScheme() _ = resources.AddToScheme(scheme) @@ -405,6 +550,7 @@ func TestChildReconciler(t *testing.T) { d.Namespace(testNamespace) d.Name(testName) d.CreationTimestamp(metav1.NewTime(time.UnixMilli(1000))) + d.ResourceVersion("999") }). StatusDie(func(d *dies.TestResourceStatusDie) { d.ConditionsDie( @@ -525,6 +671,49 @@ func TestChildReconciler(t *testing.T) { ExpectCreates: []client.Object{ configMapCreate, }, + }, { + Name: "create child with finalizer", + Parent: resource. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Created", + `Created ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("test.finalizer") + d.ResourceVersion("1000") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + ExpectCreates: []client.Object{ + configMapCreate, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.StrategicMergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, }, { Name: "update child", Parent: resourceReady. @@ -560,6 +749,102 @@ func TestChildReconciler(t *testing.T) { configMapGiven. AddData("new", "field"), }, + }, { + Name: "update child, preserve finalizers", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer, "some.other.finalizer") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", + `Updated ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer, "some.other.finalizer") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }), + ExpectUpdates: []client.Object{ + configMapGiven. + AddData("new", "field"), + }, + }, { + Name: "update child, restoring missing finalizer", + Parent: resourceReady. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", + `Updated ConfigMap %q`, testName), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + d.ResourceVersion("1000") + }). + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }). + StatusDie(func(d *dies.TestResourceStatusDie) { + d.AddField("foo", "bar") + d.AddField("new", "field") + }), + ExpectUpdates: []client.Object{ + configMapGiven. + AddData("new", "field"), + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.StrategicMergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, }, { Name: "delete child", Parent: resourceReady, @@ -578,6 +863,69 @@ func TestChildReconciler(t *testing.T) { ExpectDeletes: []rtesting.DeleteRef{ {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, }, + }, { + Name: "delete child, clearing finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched", + `Patched finalizer %q`, testFinalizer), + }, + ExpectParent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers() + d.ResourceVersion("1000") + }), + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.StrategicMergePatchType, + Patch: []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`), + }, + }, + }, { + Name: "delete child, preserve other finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("some.other.finalizer") + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + }, + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, }, { Name: "ignore extraneous children", Parent: resourceReady, @@ -801,6 +1149,76 @@ func TestChildReconciler(t *testing.T) { configMapCreate, }, ShouldErr: true, + }, { + Name: "error adding finalizer", + Parent: resource. + SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("foo", "bar") + }), + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.InduceFailure("patch", "TestResource"), + }, + 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.StrategicMergePatchType, + Patch: []byte(`{"metadata":{"finalizers":["test.finalizer"],"resourceVersion":"999"}}`), + }, + }, + ShouldErr: true, + }, { + Name: "error clearing finalizer", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers(testFinalizer) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + r := defaultChildReconciler(c) + r.Finalizer = testFinalizer + return r + }, + }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.InduceFailure("patch", "TestResource"), + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Deleted", + `Deleted ConfigMap %q`, testName), + rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "FinalizerPatchFailed", + `Failed to patch finalizer %q: inducing failure for patch TestResource`, testFinalizer), + }, + ExpectDeletes: []rtesting.DeleteRef{ + {Group: "", Kind: "ConfigMap", Namespace: testNamespace, Name: testName}, + }, + ExpectPatches: []rtesting.PatchRef{ + { + Group: "testing.reconciler.runtime", + Kind: "TestResource", + Namespace: testNamespace, + Name: testName, + PatchType: types.StrategicMergePatchType, + Patch: []byte(`{"metadata":{"finalizers":null,"resourceVersion":"999"}}`), + }, + }, + ShouldErr: true, }, { Name: "error updating child", Parent: resourceReady. diff --git a/reconcilers/reconcilers_validate_test.go b/reconcilers/reconcilers_validate_test.go index d6db56d..6892ed7 100644 --- a/reconcilers/reconcilers_validate_test.go +++ b/reconcilers/reconcilers_validate_test.go @@ -94,6 +94,94 @@ func TestSyncReconciler_validate(t *testing.T) { }, shouldErr: "SyncReconciler must implement Sync: func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) (reconcile.Result, string)", }, + { + name: "valid Finalize", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + }, + }, + { + name: "valid Finalize with result", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) (ctrl.Result, error) { + return ctrl.Result{}, nil + }, + }, + }, + { + name: "Finalize num in", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func() error { + return nil + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func() error", + }, + { + name: "Finalize in 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.Secret) error { + return nil + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.Secret) error", + }, + { + name: "Finalize num out", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) { + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap)", + }, + { + name: "Finalize out 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) string { + return "" + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) string", + }, + { + name: "Finalize result out 1", + parent: &corev1.ConfigMap{}, + reconciler: &SyncReconciler{ + Sync: func(ctx context.Context, parent *corev1.ConfigMap) error { + return nil + }, + Finalize: func(ctx context.Context, parent *corev1.ConfigMap) (ctrl.Result, string) { + return ctrl.Result{}, "" + }, + }, + shouldErr: "SyncReconciler must implement Finalize: nil | func(context.Context, *v1.ConfigMap) error | func(context.Context, *v1.ConfigMap) (ctrl.Result, error), found: func(context.Context, *v1.ConfigMap) (reconcile.Result, string)", + }, } for _, c := range tests { diff --git a/testing/reconciler.go b/testing/reconciler.go index 7dc8075..04bcceb 100644 --- a/testing/reconciler.go +++ b/testing/reconciler.go @@ -164,7 +164,7 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory }) if (err != nil) != tc.ShouldErr { - t.Errorf("Reconcile() error = %v, ExpectErr %v", err, tc.ShouldErr) + t.Errorf("Reconcile() error = %v, ShouldErr %v", err, tc.ShouldErr) } if err == nil { // result is only significant if there wasn't an error diff --git a/testing/subreconciler.go b/testing/subreconciler.go index 4c06999..ba2ebd0 100644 --- a/testing/subreconciler.go +++ b/testing/subreconciler.go @@ -132,13 +132,14 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto scheme: scheme, } log := logrtesting.NewTestLogger(t) - c := factory(t, tc, reconcilers.Config{ + c := reconcilers.Config{ Client: clientWrapper, APIReader: apiReader, Tracker: tracker, Recorder: recorder, Log: log, - }) + } + r := factory(t, tc, c) if tc.CleanUp != nil { defer func() { @@ -161,6 +162,7 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto reconcilers.StashValue(ctx, k, v) } + ctx = reconcilers.StashParentConfig(ctx, c) parent := tc.Parent.DeepCopyObject().(client.Object) ctx = reconcilers.StashParentType(ctx, parent.DeepCopyObject().(client.Object)) ctx = reconcilers.StashCastParentType(ctx, parent.DeepCopyObject().(client.Object)) @@ -175,11 +177,11 @@ func (tc *SubReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, facto }() } - return c.Reconcile(ctx, parent) + return r.Reconcile(ctx, parent) }(ctx, parent) if (err != nil) != tc.ShouldErr { - t.Errorf("Reconcile() error = %v, ExpectErr %v", err, tc.ShouldErr) + t.Errorf("Reconcile() error = %v, ShouldErr %v", err, tc.ShouldErr) } if err == nil { // result is only significant if there wasn't an error