Skip to content

Commit

Permalink
Require ListOptions when SkipOwnerReference is true (#493)
Browse files Browse the repository at this point in the history 
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 <scott@andrews.me>
  • Loading branch information
@scothis
scothis committed Mar 7, 2024
1 parent 36a3451 commit e9224d1
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 2 deletions.
13 changes: 12 additions & 1 deletion reconcilers/child.go
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down
13 changes: 12 additions & 1 deletion reconcilers/childset.go
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down
33 changes: 33 additions & 0 deletions reconcilers/validate_test.go
View file
Expand Up @@ -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{},
Expand Down Expand Up @@ -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{},
Expand Down

0 comments on commit e9224d1

Please sign in to comment.