SetControllerReference sanity checks

* remove same namespace requirement as discussed in #100
* return a specific error if the object is already owned as per review

Author: calind

pkg/controller/controllerutil/controllerutil.go

@@ -25,6 +25,26 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 )
 
+// AlreadyOwnedError is an error returned if the object you are trying to assign
+// an controller reference is already owned by another controller
+// Object is the subject and Owner is the reference for the current owner
+type AlreadyOwnedError struct {
+	Object v1.Object
+	Owner  v1.OwnerReference
+}
+
+func (e *AlreadyOwnedError) Error() string {
+	return fmt.Sprintf("Object %s/%s is already owned by another %s controller %s", e.Object.GetNamespace(), e.Object.GetName(), e.Owner.Kind, e.Owner.Name)
+}
+
+// NewAlreadyOwnedError creates a new AlreadyOwnedError
+func NewAlreadyOwnedError(Object v1.Object, Owner v1.OwnerReference) *AlreadyOwnedError {
+	return &AlreadyOwnedError{
+		Object: Object,
+		Owner:  Owner,
+	}
+}
+
 // SetControllerReference sets owner as a Controller OwnerReference on owned.
 // This is used for garbage collection of the owned object and for
 // reconciling the owner object on changes to owned (with a Watch + EnqueueRequestForOwner).
@@ -41,21 +61,16 @@ func SetControllerReference(owner, object v1.Object, scheme *runtime.Scheme) err
 		return err
 	}
 
-	// Make sure that object and owner are in the same namespace
-	if object.GetNamespace() != owner.GetNamespace() {
-		return fmt.Errorf("Object %s/%s must be in the same namespace as owner %s/%s", object.GetNamespace(), object.GetName(), owner.GetNamespace(), owner.GetName())
-	}
-
 	// Create a new ref
 	ref := *v1.NewControllerRef(owner, schema.GroupVersionKind{Group: gvk.Group, Version: gvk.Version, Kind: gvk.Kind})
 
 	existingRefs := object.GetOwnerReferences()
 	fi := -1
 	for i, r := range existingRefs {
-		if r.Kind == ref.Kind && r.Name == ref.Name && r.UID == ref.UID {
+		if sameOwnerReference(ref, r) {
 			fi = i
-		} else if *r.Controller {
-			return fmt.Errorf("Object %s/%s is already owned by another %s controller %s", object.GetNamespace(), object.GetName(), r.Kind, r.Name)
+		} else if r.Controller != nil && *r.Controller {
+			return NewAlreadyOwnedError(object, r)
 		}
 	}
 	if fi == -1 {
@@ -68,3 +83,18 @@ func SetControllerReference(owner, object v1.Object, scheme *runtime.Scheme) err
 	object.SetOwnerReferences(existingRefs)
 	return nil
 }
+
+// Returns true if a and b point to the same object
+func sameOwnerReference(a, b v1.OwnerReference) bool {
+	aGV, err := schema.ParseGroupVersion(a.APIVersion)
+	if err != nil {
+		return false
+	}
+
+	bGV, err := schema.ParseGroupVersion(b.APIVersion)
+	if err != nil {
+		return false
+	}
+
+	return aGV == bGV && a.Kind == b.Kind && a.Name == b.Name
+}

pkg/controller/controllerutil/controllerutil_test.go

@@ -44,18 +44,11 @@ var _ = Describe("Controllerutil", func() {
 			Expect(controllerutil.SetControllerReference(&errMetaObj{}, rs, scheme.Scheme)).To(HaveOccurred())
 		})
 
-		It("should return an error if object and owner are not in the same namespace", func() {
-			rs := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "one"}}
-			dep := &extensionsv1beta1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "two"}}
-
-			Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).To(HaveOccurred())
-		})
-
 		It("should return an error if object is already owned by another controller", func() {
 			t := true
 			rsOwners := []metav1.OwnerReference{
 				metav1.OwnerReference{
-					Name:               "foo",
+					Name:               "bar",
 					Kind:               "Deployment",
 					APIVersion:         "extensions/v1beta1",
 					UID:                "bar-uid",
@@ -66,7 +59,9 @@ var _ = Describe("Controllerutil", func() {
 			rs := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default", OwnerReferences: rsOwners}}
 			dep := &extensionsv1beta1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default", UID: "foo-uid"}}
 
-			Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).To(HaveOccurred())
+			Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).To(MatchError(
+				controllerutil.NewAlreadyOwnedError(rs, rsOwners[0]),
+			))
 		})
 
 		It("should not duplicate existing owner reference", func() {