diff --git a/README.md b/README.md index f9c11a7..0e33744 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,11 @@ The parent is responsible for: - updates the resource status if it was modified - logging the reconcilers activities - records events for mutations and errors +- adding and removing a finalizer if needed The implementor is responsible for: - defining the set of sub reconcilers +- providing instructions on how to clean up on deletion if needed **Example:** diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 3e75975..9acf509 100644 --- a/reconcilers/reconcilers.go +++ b/reconcilers/reconcilers.go @@ -25,6 +25,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" @@ -70,7 +71,7 @@ type ParentReconciler struct { // Reconciler is called for each reconciler request with the parent // resource being reconciled. Typically, Reconciler is a Sequence of - // multiple SubReconcilers. + // multiple SubReconcilers. It is not called on a delete request. Reconciler SubReconciler Config @@ -83,6 +84,7 @@ func (r *ParentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manage if err := r.Reconciler.SetupWithManager(ctx, mgr, bldr); err != nil { return err } + return bldr.Complete(r) } @@ -117,6 +119,7 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr initializeConditions.Call([]reflect.Value{}) } } + result, err := r.reconcile(ctx, parent) if r.hasStatus(originalParent) { @@ -137,15 +140,23 @@ func (r *ParentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr "Updated status") } } + + if !equality.Semantic.DeepEqual(parent.GetFinalizers(), originalParent.GetFinalizers()) { + log.Info("updating finalizers", "diff", cmp.Diff(parent.GetFinalizers(), originalParent.GetFinalizers())) + if updateErr := r.Update(ctx, parent); updateErr != nil { + log.Error(updateErr, "unable to update", typeName(r.Type), parent) + r.Recorder.Eventf(parent, corev1.EventTypeWarning, "UpdateFailed", + "Failed to update finalizers: %v", updateErr) + return ctrl.Result{}, updateErr + } + r.Recorder.Eventf(parent, corev1.EventTypeNormal, "Updated", "Updated finalizers") + } + // return original reconcile result return result, err } func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) { - if parent.GetDeletionTimestamp() != nil { - return ctrl.Result{}, nil - } - result, err := r.Reconciler.Reconcile(ctx, parent) if err != nil { return ctrl.Result{}, err @@ -154,6 +165,7 @@ func (r *ParentReconciler) reconcile(ctx context.Context, parent client.Object) r.copyGeneration(parent) return result, nil } + func (r *ParentReconciler) hasStatus(obj client.Object) bool { return reflect.ValueOf(obj).Elem().FieldByName("Status").IsValid() } @@ -263,6 +275,15 @@ type SyncReconciler struct { // func(ctx context.Context, parent client.Object) (ctrl.Result, error) Sync interface{} + // Finalize is called on a delete request. Typically, Finalize cleans + // up any state, created by previous reconciles, of which the parent is + // not a Kubernetes owner. + // + // Expected function signature: + // func(ctx context.Context, parent client.Object) error + // +optional + Finalize interface{} + Config } @@ -306,10 +327,56 @@ func (r *SyncReconciler) validate(ctx context.Context) error { } } + // validate Finalize function signature: + // nil + // func(ctx context.Context, parent client.Object) error + if r.Finalize != nil { + castParentType := RetrieveCastParentType(ctx) + fn := reflect.TypeOf(r.Finalize) + if fn.NumIn() != 2 || fn.NumOut() != 1 || + !reflect.TypeOf((*context.Context)(nil)).Elem().AssignableTo(fn.In(0)) || + !reflect.TypeOf(castParentType).AssignableTo(fn.In(1)) || + !reflect.TypeOf((*error)(nil)).Elem().AssignableTo(fn.Out(0)) { + return fmt.Errorf("SyncReconciler Finalize must have correct signature: func(context.Context, %s) error, found: %s", reflect.TypeOf(castParentType), fn) + } + } + return nil } +func UniqueFinalizer(ctx context.Context, group string) string { + finalizerCount, ok := RetrieveValue(ctx, "finalizer-count").(int) + if !ok { + finalizerCount = 1 + } + finalizer := fmt.Sprintf("%s/%d-finalizer", group, finalizerCount) + StashValue(ctx, "finalizer-count", finalizerCount+1) + + return finalizer +} + func (r *SyncReconciler) Reconcile(ctx context.Context, parent client.Object) (ctrl.Result, error) { + finalizer := UniqueFinalizer(ctx, parent.GetObjectKind().GroupVersionKind().Group) + if r.Finalize != nil { + controllerutil.AddFinalizer(parent, finalizer) + } else { + controllerutil.RemoveFinalizer(parent, finalizer) + } + + if r.Finalize == nil && parent.GetDeletionTimestamp() != nil { + return ctrl.Result{}, nil + } + + if parent.GetDeletionTimestamp() != nil { + if err := r.finalize(ctx, parent); err != nil { + return ctrl.Result{}, err + } + + controllerutil.RemoveFinalizer(parent, finalizer) + + return ctrl.Result{}, nil + } + result, err := r.sync(ctx, parent) if err != nil { r.Log.Error(err, "unable to sync", typeName(parent), parent) @@ -341,6 +408,18 @@ 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) error { + fn := reflect.ValueOf(r.Finalize) + out := fn.Call([]reflect.Value{ + reflect.ValueOf(ctx), + reflect.ValueOf(parent), + }) + if !out[0].IsNil() { + return out[0].Interface().(error) + } + return nil +} + 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") ) @@ -576,6 +655,10 @@ func (r *ChildReconciler) Reconcile(ctx context.Context, parent client.Object) ( r.mutationCache = cache.NewExpiring() } + if parent.GetDeletionTimestamp() != nil { + return ctrl.Result{}, nil + } + child, err := r.reconcile(ctx, parent) if err != nil { parentType := RetrieveParentType(ctx) diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index 63c6c48..a2ee8e2 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -110,25 +110,6 @@ func TestParentReconciler(t *testing.T) { } }, }, - }, { - Name: "ignore deleted resource", - Key: testKey, - GivenObjects: []client.Object{ - resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { - d.DeletionTimestamp(&deletedAt) - }), - }, - 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 { - t.Error("should not be called") - return nil - }, - } - }, - }, }, { Name: "error fetching resource", Key: testKey, @@ -231,6 +212,36 @@ func TestParentReconciler(t *testing.T) { d.AddField("Reconciler", "ran") }), }, + }, { + Name: "reconciler mutated finalizers", + 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 { + return nil + }, + Finalize: func(ctx context.Context, parent *resources.TestResource) error { + return nil + }, + } + }, + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated", + `Updated finalizers`), + }, + ExpectUpdates: []client.Object{ + resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("testing.reconciler.runtime/1-finalizer") + }).SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("Defaulter", "ran") + }), + }, }, { Name: "sub reconciler erred", Key: testKey, @@ -302,6 +313,42 @@ func TestParentReconciler(t *testing.T) { } }, }, + }, { + Name: "finalizers update failed", + Key: testKey, + GivenObjects: []client.Object{ + resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers() + }), + }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.InduceFailure("update", "TestResource"), + }, + ExpectEvents: []rtesting.Event{ + rtesting.NewEvent(resource, scheme, corev1.EventTypeWarning, "UpdateFailed", + `Failed to update finalizers: inducing failure for update TestResource`), + }, + ExpectUpdates: []client.Object{ + resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("testing.reconciler.runtime/1-finalizer") + }).SpecDie(func(d *dies.TestResourceSpecDie) { + d.AddField("Defaulter", "ran") + }), + }, + 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 nil + }, + } + }, + }, + ShouldErr: true, }} rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.ReconcilerTestCase, c reconcilers.Config) reconcile.Reconciler { @@ -331,6 +378,7 @@ func TestSyncReconciler(t *testing.T) { diemetav1.ConditionBlank.Type(apis.ConditionReady).Status(metav1.ConditionUnknown).Reason("Initializing"), ) }) + deletedAt := metav1.NewTime(time.UnixMilli(2000)) rts := rtesting.SubReconcilerTestSuite{{ Name: "sync success", @@ -385,6 +433,104 @@ func TestSyncReconciler(t *testing.T) { }, }, ShouldPanic: true, + }, { + Name: "has Finalize", + Parent: resource, + ExpectParent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("/1-finalizer") + }), + 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 nil + }, + } + }, + }, + }, { + Name: "has finalizer", + Parent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("/1-finalizer") + }), + ExpectParent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers() + }), + 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: "has Finalize and has finalizer", + Parent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.Finalizers("/1-finalizer") + }), + 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 nil + }, + } + }, + }, + }, { + Name: "has finalize, has finalizer, and is deleted", + Parent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&deletedAt) + d.Finalizers("/1-finalizer") + }), + ExpectParent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&deletedAt) + d.Finalizers() + }), + 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 nil + }, + } + }, + }, + }, { + Name: "Finalize erred", + Parent: resource.MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&deletedAt) + d.Finalizers("/1-finalizer") + }), + 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("Finalize error") + }, + } + }, + }, + ShouldErr: true, }} rts.Test(t, scheme, func(t *testing.T, rtc *rtesting.SubReconcilerTestCase, c reconcilers.Config) reconcilers.SubReconciler { @@ -418,6 +564,8 @@ func TestChildReconciler(t *testing.T) { ) }) + deletionTimestamp := metav1.NewTime(time.UnixMilli(2000)) + configMapCreate := diecorev1.ConfigMapBlank. MetadataDie(func(d *diemetav1.ObjectMetaDie) { d.Namespace(testNamespace) @@ -484,6 +632,20 @@ func TestChildReconciler(t *testing.T) { return defaultChildReconciler(c) }, }, + }, { + Name: "ignore deleted", + Parent: resourceReady. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.DeletionTimestamp(&deletionTimestamp) + }), + GivenObjects: []client.Object{ + configMapGiven, + }, + Metadata: map[string]interface{}{ + "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler { + return defaultChildReconciler(c) + }, + }, }, { Name: "child is in sync", Parent: resourceReady. @@ -1419,3 +1581,17 @@ func TestCastParent(t *testing.T) { return rtc.Metadata["SubReconciler"].(func(*testing.T, reconcilers.Config) reconcilers.SubReconciler)(t, c) }) } + +func TestUniqueFinalizer(t *testing.T) { + ctx := reconcilers.WithStash(context.Background()) + + finalizer1 := reconcilers.UniqueFinalizer(ctx, "group.com") + if diff := cmp.Diff("group.com/1-finalizer", finalizer1); diff != "" { + t.Errorf("Unexpected delete (-expected, +actual): %s", diff) + } + + finalizer2 := reconcilers.UniqueFinalizer(ctx, "group.com") + if diff := cmp.Diff("group.com/2-finalizer", finalizer2); diff != "" { + t.Errorf("Unexpected delete (-expected, +actual): %s", diff) + } +} diff --git a/reconcilers/reconcilers_validate_test.go b/reconcilers/reconcilers_validate_test.go index d6db56d..b1f0838 100644 --- a/reconcilers/reconcilers_validate_test.go +++ b/reconcilers/reconcilers_validate_test.go @@ -36,6 +36,18 @@ func TestSyncReconciler_validate(t *testing.T) { }, }, }, + { + name: "valid with 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 Sync with result", parent: &corev1.ConfigMap{}, @@ -94,6 +106,58 @@ 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: "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 Finalize must have correct signature: func(context.Context, *v1.ConfigMap) 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 Finalize must have correct signature: func(context.Context, *v1.ConfigMap) 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) (ctrl.Result, error) { + return ctrl.Result{}, nil + }, + }, + shouldErr: "SyncReconciler Finalize must have correct signature: func(context.Context, *v1.ConfigMap) error, found: func(context.Context, *v1.ConfigMap) (reconcile.Result, error)", + }, + { + 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) *ctrl.Result { + return nil + }, + }, + shouldErr: "SyncReconciler Finalize must have correct signature: func(context.Context, *v1.ConfigMap) error, found: func(context.Context, *v1.ConfigMap) *reconcile.Result", + }, } for _, c := range tests {