refactor: util should not check UIDs

Modifies PointsTo and introduces IsControlledBy, both of which search
through the owner reference list of their first argument (similar to
metav1.IsControlledBy).

Unlike the metav1 function, they're looking for a match on name plus
api group (not version) and kind. PointsTo returns true if any
OwnerReference matches the pointee, whereas IsControlledBy only returns
true if the sole permitted (by the API) controller reference matches.

This is a behavior change not least for tests, which typically do not
set either Kind or GroupVersion on various objects that end up acting as
referents.

Author: sethp-nr

controllers/cluster_controller.go

@@ -404,7 +404,7 @@ func (c clusterDescendants) filterOwnedDescendants(cluster *clusterv1.Cluster) (
 			return nil
 		}
 
-		if util.PointsTo(acc.GetOwnerReferences(), &cluster.ObjectMeta) {
+		if util.PointsTo(acc, cluster) {
 			ownedDescendants = append(ownedDescendants, o)
 		}
 

controllers/cluster_controller_test.go

@@ -703,6 +703,10 @@ func TestFilterOwnedDescendants(t *testing.T) {
 	g := NewWithT(t)
 
 	c := clusterv1.Cluster{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: clusterv1.GroupVersion.String(),
+			Kind:       "Cluster",
+		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "c",
 		},

controlplane/kubeadm/controllers/controller.go

@@ -511,7 +511,8 @@ func (r *KubeadmControlPlaneReconciler) adoptOwnedSecrets(ctx context.Context, k
 	}
 
 	for _, s := range secrets.Items {
-		if !util.PointsTo(s.GetOwnerReferences(), currentOwner) {
+		s := s
+		if !util.IsControlledBy(&s, currentOwner) {
 			continue
 		}
 		// avoid taking ownership of the bootstrap data secret

controlplane/kubeadm/controllers/controller_test.go

@@ -1179,6 +1179,10 @@ func createClusterWithControlPlane() (*clusterv1.Cluster, *controlplanev1.Kubead
 	}
 
 	kcp := &controlplanev1.KubeadmControlPlane{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: controlplanev1.GroupVersion.String(),
+			Kind:       "KubeadmControlPlane",
+		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "kcp-foo",
 			Namespace: cluster.Namespace,
@@ -1249,7 +1253,6 @@ func createMachineNodePair(name string, cluster *clusterv1.Cluster, kcp *control
 			},
 		}
 	}
-
 	return machine, node
 }
 

controlplane/kubeadm/internal/machinefilters/machine_filters.go

@@ -23,6 +23,7 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3"
+	"sigs.k8s.io/cluster-api/util"
 )
 
 type Func func(machine *clusterv1.Machine) bool
@@ -94,16 +95,12 @@ func InFailureDomains(failureDomains ...*string) Func {
 
 // OwnedMachines returns a filter to find all owned control plane machines.
 // Usage: managementCluster.GetMachinesForCluster(ctx, cluster, machinefilters.OwnedMachines(controlPlane))
-func OwnedMachines(owner metav1.Object) func(machine *clusterv1.Machine) bool {
+func OwnedMachines(owner util.Object) func(machine *clusterv1.Machine) bool {
 	return func(machine *clusterv1.Machine) bool {
 		if machine == nil {
 			return false
 		}
-		controllerRef := metav1.GetControllerOf(machine)
-		if controllerRef == nil {
-			return false
-		}
-		return controllerRef.Kind == "KubeadmControlPlane" && controllerRef.Name == owner.GetName()
+		return util.PointsTo(machine, owner)
 	}
 }
 

test/framework/convenience_test.go

@@ -29,8 +29,6 @@ import (
 func TestTypeToKind(t *testing.T) {
 	g := NewWithT(t)
 
-	type hello struct{}
-
 	g.Expect(framework.TypeToKind(&hello{})).To(Equal("hello"))
 }
 
@@ -42,6 +40,6 @@ func (*hello) GetObjectKind() schema.ObjectKind {
 	return schema.EmptyObjectKind
 }
 
-func (h *hello) GetObjectKind() schema.ObjectKind {
+func (h *hello) DeepCopyObject() runtime.Object {
 	return h
 }

util/util.go

@@ -404,16 +404,41 @@ func referSameObject(a, b metav1.OwnerReference) bool {
 }
 
 // PointsTo returns true if any of the owner references point to the given target
-func PointsTo(refs []metav1.OwnerReference, target metav1.Object) bool {
-	for _, ref := range refs {
-		if ref.UID == target.GetUID() {
+func PointsTo(obj metav1.Object, target Object) bool {
+	for _, ref := range obj.GetOwnerReferences() {
+		ref := ref
+		if refersTo(&ref, target) {
 			return true
 		}
 	}
-
 	return false
 }
 
+type Object interface {
+	metav1.Object
+	GroupVersionKind() schema.GroupVersionKind
+}
+
+// IsControlledBy differs from metav1.IsControlledBy in that it checks the group (but not version), kind, and name vs uid
+func IsControlledBy(obj metav1.Object, owner Object) bool {
+	controllerRef := metav1.GetControllerOfNoCopy(obj)
+	if controllerRef == nil {
+		return false
+	}
+	return refersTo(controllerRef, owner)
+}
+
+// Returns true if ref refers to obj
+func refersTo(ref *metav1.OwnerReference, obj Object) bool {
+	refGv, err := schema.ParseGroupVersion(ref.APIVersion)
+	if err != nil {
+		return false
+	}
+
+	gvk := obj.GroupVersionKind()
+	return refGv.Group == gvk.Group && ref.Kind == gvk.Kind && ref.Name == obj.GetName()
+}
+
 // UnstructuredUnmarshalField is a wrapper around json and unstructured objects to decode and copy a specific field
 // value into an object.
 func UnstructuredUnmarshalField(obj *unstructured.Unstructured, v interface{}, fields ...string) error {

util/util_test.go

@@ -27,7 +27,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -313,51 +312,107 @@ func TestHasOwner(t *testing.T) {
 func TestPointsTo(t *testing.T) {
 	g := NewWithT(t)
 
-	targetID := "fri3ndsh1p"
+	targetGroup := "ponies.info"
+	targetKind := "Rainbow"
+	targetName := "fri3ndsh1p"
 
-	meta := metav1.ObjectMeta{
-		UID: types.UID(targetID),
+	meta := struct {
+		metav1.ObjectMeta
+		metav1.TypeMeta
+	}{
+		metav1.ObjectMeta{
+			Name: targetName,
+		},
+		metav1.TypeMeta{
+			APIVersion: "ponies.info/v1",
+			Kind:       targetKind,
+		},
 	}
 
 	tests := []struct {
 		name     string
-		refIDs   []string
+		refs     []metav1.OwnerReference
 		expected bool
 	}{
 		{
 			name: "empty owner list",
 		},
 		{
-			name:   "single wrong owner ref",
-			refIDs: []string{"m4g1c"},
+			name: "single wrong name owner ref",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       "m4g1c",
+			}},
 		},
 		{
-			name:     "single right owner ref",
-			refIDs:   []string{targetID},
+			name: "single wrong group owner ref",
+			refs: []metav1.OwnerReference{{
+				APIVersion: "dazzlings.info/v1",
+				Kind:       "Twilight",
+				Name:       "m4g1c",
+			}},
+		},
+		{
+			name: "single wrong kind owner ref",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v1",
+				Kind:       "Twilight",
+				Name:       "m4g1c",
+			}},
+		},
+		{
+			name: "single right owner ref",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       targetName,
+			}},
 			expected: true,
 		},
 		{
-			name:   "multiple wrong refs",
-			refIDs: []string{"m4g1c", "h4rm0ny"},
+			name: "single right owner ref (different version)",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v2alpha2",
+				Kind:       targetKind,
+				Name:       targetName,
+			}},
+			expected: true,
 		},
 		{
-			name:     "multiple refs one right",
-			refIDs:   []string{"m4g1c", targetID},
+			name: "multiple wrong refs",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       "m4g1c",
+			}, {
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       "h4rm0ny",
+			}},
+		},
+		{
+			name: "multiple refs one right",
+			refs: []metav1.OwnerReference{{
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       "m4g1c",
+			}, {
+				APIVersion: targetGroup + "/v1",
+				Kind:       targetKind,
+				Name:       targetName,
+			}},
 			expected: true,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pointer := &metav1.ObjectMeta{}
-
-			for _, ref := range test.refIDs {
-				pointer.OwnerReferences = append(pointer.OwnerReferences, metav1.OwnerReference{
-					UID: types.UID(ref),
-				})
+			pointer := &metav1.ObjectMeta{
+				OwnerReferences: test.refs,
 			}
 
-			g.Expect(PointsTo(pointer.OwnerReferences, &meta)).To(Equal(test.expected))
+			g.Expect(PointsTo(pointer, &meta)).To(Equal(test.expected), "Could not find a ref to %+v in %+v", meta, test.refs)
 		})
 	}
 }