From e9224d11695a6a155d72fedb705ff2d9044d68be Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Thu, 7 Mar 2024 10:55:11 -0500 Subject: [PATCH] Require ListOptions when SkipOwnerReference is true (#493) The most common reason to disable owner references for a child resource is when that resource is in a different namespace. In that case, the default ListOptions (filter by the parent's namespace) is ineffective and will orphan the child resource. We now validate that ListOptions is defined when SkipOwnerReference is true and fail fast. Signed-off-by: Scott Andrews --- reconcilers/child.go | 13 ++++++++++++- reconcilers/childset.go | 13 ++++++++++++- reconcilers/validate_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/reconcilers/child.go b/reconcilers/child.go index 2296041..3cab787 100644 --- a/reconcilers/child.go +++ b/reconcilers/child.go @@ -121,13 +121,19 @@ type ChildReconciler[Type, ChildType client.Object, ChildListType client.ObjectL MergeBeforeUpdate func(current, desired ChildType) // ListOptions allows custom options to be use when listing potential child resources. Each - // resource retrieved as part of the listing is confirmed via OurChild. + // resource retrieved as part of the listing is confirmed via OurChild. There is a performance + // benefit to limiting the number of resource return for each List operation, however, + // excluding an actual child will orphan that resource. // // Defaults to filtering by the reconciled resource's namespace: // []client.ListOption{ // client.InNamespace(resource.GetNamespace()), // } // + // ListOptions is required when a Finalizer is defined or SkipOwnerReference is true. An empty + // list is often sufficient although it may incur a performance penalty, especially when + // querying the API sever instead of an informer cache. + // // +optional ListOptions func(ctx context.Context, resource Type) []client.ListOption @@ -229,6 +235,11 @@ func (r *ChildReconciler[T, CT, CLT]) validate(ctx context.Context) error { return fmt.Errorf("ChildReconciler %q must implement OurChild since owner references are not used", r.Name) } + if r.ListOptions == nil && r.SkipOwnerReference { + // ListOptions is required when SkipOwnerReference is true + return fmt.Errorf("ChildReconciler %q must implement ListOptions since owner references are not used", r.Name) + } + // require MergeBeforeUpdate if r.MergeBeforeUpdate == nil { return fmt.Errorf("ChildReconciler %q must implement MergeBeforeUpdate", r.Name) diff --git a/reconcilers/childset.go b/reconcilers/childset.go index 2020572..b659bf3 100644 --- a/reconcilers/childset.go +++ b/reconcilers/childset.go @@ -116,13 +116,19 @@ type ChildSetReconciler[Type, ChildType client.Object, ChildListType client.Obje MergeBeforeUpdate func(current, desired ChildType) // ListOptions allows custom options to be use when listing potential child resources. Each - // resource retrieved as part of the listing is confirmed via OurChild. + // resource retrieved as part of the listing is confirmed via OurChild. There is a performance + // benefit to limiting the number of resource return for each List operation, however, + // excluding an actual child will orphan that resource. // // Defaults to filtering by the reconciled resource's namespace: // []client.ListOption{ // client.InNamespace(resource.GetNamespace()), // } // + // ListOptions is required when a Finalizer is defined or SkipOwnerReference is true. An empty + // list is often sufficient although it may incur a performance penalty, especially when + // querying the API sever instead of an informer cache. + // // +optional ListOptions func(ctx context.Context, resource Type) []client.ListOption @@ -257,6 +263,11 @@ func (r *ChildSetReconciler[T, CT, CLT]) validate(ctx context.Context) error { return fmt.Errorf("ChildSetReconciler %q must implement OurChild since owner references are not used", r.Name) } + if r.ListOptions == nil && r.SkipOwnerReference { + // ListOptions is required when SkipOwnerReference is true + return fmt.Errorf("ChildSetReconciler %q must implement ListOptions since owner references are not used", r.Name) + } + // require IdentifyChild if r.IdentifyChild == nil { return fmt.Errorf("ChildSetReconciler %q must implement IdentifyChild", r.Name) diff --git a/reconcilers/validate_test.go b/reconcilers/validate_test.go index ac8b4ef..f3ebd5c 100644 --- a/reconcilers/validate_test.go +++ b/reconcilers/validate_test.go @@ -463,6 +463,22 @@ func TestChildReconciler_validate(t *testing.T) { ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} }, }, }, + { + name: "ListOptions missing", + parent: &corev1.ConfigMap{}, + reconciler: &ChildReconciler[*corev1.ConfigMap, *corev1.Pod, *corev1.PodList]{ + Name: "ListOptions missing", + ChildType: &corev1.Pod{}, + ChildListType: &corev1.PodList{}, + DesiredChild: func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil }, + ReflectChildStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, child *corev1.Pod, err error) {}, + MergeBeforeUpdate: func(current, desired *corev1.Pod) {}, + SkipOwnerReference: true, + // ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} }, + OurChild: func(resource *corev1.ConfigMap, child *corev1.Pod) bool { return true }, + }, + shouldErr: `ChildReconciler "ListOptions missing" must implement ListOptions since owner references are not used`, + }, { name: "Finalizer without OurChild", parent: &corev1.ConfigMap{}, @@ -622,6 +638,23 @@ func TestChildSetReconciler_validate(t *testing.T) { IdentifyChild: func(child *corev1.Pod) string { return "" }, }, }, + { + name: "ListOptions missing", + parent: &corev1.ConfigMap{}, + reconciler: &ChildSetReconciler[*corev1.ConfigMap, *corev1.Pod, *corev1.PodList]{ + Name: "ListOptions missing", + ChildType: &corev1.Pod{}, + ChildListType: &corev1.PodList{}, + DesiredChildren: func(ctx context.Context, parent *corev1.ConfigMap) ([]*corev1.Pod, error) { return nil, nil }, + ReflectChildrenStatusOnParent: func(ctx context.Context, parent *corev1.ConfigMap, result ChildSetResult[*corev1.Pod]) {}, + MergeBeforeUpdate: func(current, desired *corev1.Pod) {}, + SkipOwnerReference: true, + // ListOptions: func(ctx context.Context, parent *corev1.ConfigMap) []client.ListOption { return []client.ListOption{} }, + OurChild: func(resource *corev1.ConfigMap, child *corev1.Pod) bool { return true }, + IdentifyChild: func(child *corev1.Pod) string { return "" }, + }, + shouldErr: `ChildSetReconciler "ListOptions missing" must implement ListOptions since owner references are not used`, + }, { name: "Finalizer without OurChild", parent: &corev1.ConfigMap{},