Handle pointer items in ObjectList with ChildReconciler (#480)

* Handle pointer items in ObjectList with ChildReconciler Canonically, Items in an ObjectList are a slice of structs. Some types, like Istio, use a slice of pointers intead. We now detect and handle both. Signed-off-by: Scott Andrews <scott@andrews.me> * Review feedback Signed-off-by: Scott Andrews <scott@andrews.me> --------- Signed-off-by: Scott Andrews <scott@andrews.me>
vmware-labs · Feb 19, 2024 · 07e174c · 07e174c
1 parent ae28210
commit 07e174c
Show file tree

Hide file tree

Showing 7 changed files with 335 additions and 13 deletions.
diff --git a/internal/resources/resource_list_invalid.go b/internal/resources/resource_list_invalid.go
@@ -0,0 +1,19 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package resources
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// +kubebuilder:object:root=true
+
+type TestResourceInvalidList struct {
+	metav1.TypeMeta `json:",inline"`
+	metav1.ListMeta `json:"metadata"`
+
+	Items []string `json:"items"`
+}
diff --git a/internal/resources/resource_list_with_interface_items.go b/internal/resources/resource_list_with_interface_items.go
@@ -0,0 +1,53 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package resources
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	runtime "k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+type TestResourceInterfaceList struct {
+	metav1.TypeMeta `json:",inline"`
+	metav1.ListMeta `json:"metadata"`
+
+	Items []client.Object `json:"items"`
+}
+
+// Directly host DeepCopy methods since controller-gen cannot handle the interface
+
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *TestResourceInterfaceList) DeepCopyInto(out *TestResourceInterfaceList) {
+	*out = *in
+	out.TypeMeta = in.TypeMeta
+	in.ListMeta.DeepCopyInto(&out.ListMeta)
+	if in.Items != nil {
+		in, out := &in.Items, &out.Items
+		*out = make([]client.Object, len(*in))
+		for i := range *in {
+			(*out)[i] = (*in)[i].DeepCopyObject().(client.Object)
+		}
+	}
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TestResourceInterfaceList.
+func (in *TestResourceInterfaceList) DeepCopy() *TestResourceInterfaceList {
+	if in == nil {
+		return nil
+	}
+	out := new(TestResourceInterfaceList)
+	in.DeepCopyInto(out)
+	return out
+}
+
+// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object.
+func (in *TestResourceInterfaceList) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	}
+	return nil
+}
diff --git a/internal/resources/resource_list_with_pointer_items.go b/internal/resources/resource_list_with_pointer_items.go
@@ -0,0 +1,19 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package resources
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// +kubebuilder:object:root=true
+
+type TestResourcePointerList struct {
+	metav1.TypeMeta `json:",inline"`
+	metav1.ListMeta `json:"metadata"`
+
+	Items []*TestResource `json:"items"`
+}
diff --git a/internal/resources/zz_generated.deepcopy.go b/internal/resources/zz_generated.deepcopy.go
diff --git a/reconcilers/child.go b/reconcilers/child.go
@@ -9,7 +9,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"reflect"
 	"sync"
 
 	"github.com/go-logr/logr"
@@ -381,15 +380,3 @@ func (r *ChildReconciler[T, CT, CLT]) ourChild(resource T, obj CT) bool {
 	}
 	return r.OurChild(resource, obj)
 }
-
-// extractItems returns a typed slice of objects from an object list
-func extractItems[T client.Object](list client.ObjectList) []T {
-	items := []T{}
-	listValue := reflect.ValueOf(list).Elem()
-	itemsValue := listValue.FieldByName("Items")
-	for i := 0; i < itemsValue.Len(); i++ {
-		item := itemsValue.Index(i).Addr().Interface().(T)
-		items = append(items, item)
-	}
-	return items
-}
diff --git a/reconcilers/util.go b/reconcilers/util.go
@@ -0,0 +1,36 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package reconcilers
+
+import (
+	"fmt"
+	"reflect"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// extractItems returns a typed slice of objects from an object list
+func extractItems[T client.Object](list client.ObjectList) []T {
+	items := []T{}
+	listValue := reflect.ValueOf(list).Elem()
+	itemsValue := listValue.FieldByName("Items")
+	for i := 0; i < itemsValue.Len(); i++ {
+		itemValue := itemsValue.Index(i)
+		var item T
+		switch itemValue.Kind() {
+		case reflect.Pointer:
+			item = itemValue.Interface().(T)
+		case reflect.Interface:
+			item = itemValue.Interface().(T)
+		case reflect.Struct:
+			item = itemValue.Addr().Interface().(T)
+		default:
+			panic(fmt.Errorf("unknown type %s for Items slice, expected Pointer or Struct", itemValue.Kind().String()))
+		}
+		items = append(items, item)
+	}
+	return items
+}
diff --git a/reconcilers/util_test.go b/reconcilers/util_test.go
@@ -0,0 +1,142 @@
+/*
+Copyright 2024 VMware, Inc.
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package reconcilers
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/vmware-labs/reconciler-runtime/internal/resources"
+	corev1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func TestExtractItems(t *testing.T) {
+	tests := map[string]struct {
+		list        client.ObjectList
+		expected    []*resources.TestResource
+		shouldPanic bool
+	}{
+		"empty": {
+			list: &resources.TestResourceList{
+				Items: []resources.TestResource{},
+			},
+			expected: []*resources.TestResource{},
+		},
+		"struct items": {
+			list: &resources.TestResourceList{
+				Items: []resources.TestResource{
+					{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj1",
+						},
+					},
+					{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj2",
+						},
+					},
+				},
+			},
+			expected: []*resources.TestResource{
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj1",
+					},
+				},
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj2",
+					},
+				},
+			},
+		},
+		"struct-pointer items": {
+			list: &resources.TestResourcePointerList{
+				Items: []*resources.TestResource{
+					{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj1",
+						},
+					},
+					{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj2",
+						},
+					},
+				},
+			},
+			expected: []*resources.TestResource{
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj1",
+					},
+				},
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj2",
+					},
+				},
+			},
+		},
+		"interface items": {
+			list: &resources.TestResourceInterfaceList{
+				Items: []client.Object{
+					&resources.TestResource{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj1",
+						},
+					},
+					&resources.TestResource{
+						ObjectMeta: corev1.ObjectMeta{
+							Name: "obj2",
+						},
+					},
+				},
+			},
+			expected: []*resources.TestResource{
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj1",
+					},
+				},
+				{
+					ObjectMeta: corev1.ObjectMeta{
+						Name: "obj2",
+					},
+				},
+			},
+		},
+		"invalid items": {
+			list: &resources.TestResourceInvalidList{
+				Items: []string{
+					"boom",
+				},
+			},
+			shouldPanic: true,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			defer func() {
+				if r := recover(); r != nil {
+					if !tc.shouldPanic {
+						t.Errorf("unexpected panic: %s", r)
+					}
+				}
+			}()
+			actual := extractItems[*resources.TestResource](tc.list)
+			if tc.shouldPanic {
+				t.Errorf("expected to panic")
+			}
+			expected := tc.expected
+			if diff := cmp.Diff(expected, actual); diff != "" {
+				t.Errorf("expected items to match actual items: %s", diff)
+			}
+		})
+	}
+}