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 <[email protected]>
reconcilerio · Mar 7, 2024 · e9224d1 · e9224d1
1 parent 36a3451
commit e9224d1
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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{},