From d0ae4183b877cb2136f74d78d0557c63f425f491 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Thu, 24 Aug 2023 17:55:45 -0400 Subject: [PATCH 1/4] add function to controllerutil RemoveControllerReference Signed-off-by: Troy Connor --- .../controllerutil/controllerutil.go | 21 ++++++++++ .../controllerutil/controllerutil_test.go | 41 ++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index f76e012ea8..d7d1e36834 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -91,6 +91,27 @@ func SetControllerReference(owner, controlled metav1.Object, scheme *runtime.Sch return nil } +// RemoveControllerReference is a helper method to make sure the given object removes an controller reference to the object provided. +// This allows you to remove the owner to establish a new owner of the object in a subsequent call. +func RemoveControllerReference(owner, controlled metav1.Object) error { + owners := controlled.GetOwnerReferences() + length := len(owners) + if length < 1 { + return fmt.Errorf("%T does not have any owner references", controlled) + } + index := 0 + for i := 0; i < length; i++ { + if owners[i].Name == owner.GetName() { + owners = append(owners[:index], owners[index+1:]...) + } + index++ + } + if length == len(owners) { + return fmt.Errorf("%T does not have an owner reference for %T", controlled, owner) + } + return nil +} + // SetOwnerReference is a helper method to make sure the given object contains an object reference to the object provided. // This allows you to declare that owner has a dependency on the object without specifying it as a controller. // If a reference to the same object already exists, it'll be overwritten with the newly provided version. diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index a058ce0714..5f47e65134 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/pkg/controller/controllerutil/controllerutil_test.go @@ -103,7 +103,6 @@ var _ = Describe("Controllerutil", func() { })) }) }) - Describe("SetControllerReference", func() { It("should set the OwnerReference if it can find the group version kind", func() { rs := &appsv1.ReplicaSet{} @@ -256,6 +255,46 @@ var _ = Describe("Controllerutil", func() { })) }) }) + Describe("RemoveControllerReference", func() { + It("should remove the owner reference established by the SetControllerReference function", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + t := true + Expect(rs.OwnerReferences).To(ConsistOf(metav1.OwnerReference{ + Name: "foo", + Kind: "Deployment", + APIVersion: "extensions/v1beta1", + UID: "foo-uid", + Controller: &t, + BlockOwnerDeletion: &t, + })) + + Expect(controllerutil.RemoveControllerReference(dep, rs)).NotTo(HaveOccurred()) + }) + It("should fail and return an error if the length is less than 1", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + Expect(controllerutil.RemoveControllerReference(dep, rs)).To(HaveOccurred()) + }) + It("should fail and return an error because the owner doesn't exist to remove", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + dep2 := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "bar", UID: "bar-uid"}, + } + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(dep2, rs)).To(HaveOccurred()) + }) + + }) Describe("CreateOrUpdate", func() { var deploy *appsv1.Deployment From 7bdfa405544eeca3ce43a2852ae2c758a739b053 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Fri, 25 Aug 2023 09:57:08 -0400 Subject: [PATCH 2/4] add more testing to assert on cases not handled Signed-off-by: Troy Connor --- .../controllerutil/controllerutil.go | 23 ++++++++++--------- .../controllerutil/controllerutil_test.go | 16 ++++++++++++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index d7d1e36834..0c56967ce9 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -91,24 +91,25 @@ func SetControllerReference(owner, controlled metav1.Object, scheme *runtime.Sch return nil } -// RemoveControllerReference is a helper method to make sure the given object removes an controller reference to the object provided. +// RemoveControllerReference is a helper method to make sure the given object removes a controller reference to the object provided. // This allows you to remove the owner to establish a new owner of the object in a subsequent call. -func RemoveControllerReference(owner, controlled metav1.Object) error { - owners := controlled.GetOwnerReferences() +func RemoveControllerReference(owner, object metav1.Object) error { + owners := object.GetOwnerReferences() length := len(owners) + result := []metav1.OwnerReference{} if length < 1 { - return fmt.Errorf("%T does not have any owner references", controlled) + return fmt.Errorf("%T does not have any owner references", object) } - index := 0 - for i := 0; i < length; i++ { - if owners[i].Name == owner.GetName() { - owners = append(owners[:index], owners[index+1:]...) + for _, ownerref := range owners { + if ownerref.Name == owner.GetName() { + continue } - index++ + result = append(result, ownerref) } - if length == len(owners) { - return fmt.Errorf("%T does not have an owner reference for %T", controlled, owner) + if len(result) == len(owners) { + return fmt.Errorf("%T does not have an owner reference for %T", object, owner) } + object.SetOwnerReferences(result) return nil } diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index 5f47e65134..d07ef43eac 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/pkg/controller/controllerutil/controllerutil_test.go @@ -272,8 +272,8 @@ var _ = Describe("Controllerutil", func() { Controller: &t, BlockOwnerDeletion: &t, })) - Expect(controllerutil.RemoveControllerReference(dep, rs)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(0)) }) It("should fail and return an error if the length is less than 1", func() { rs := &appsv1.ReplicaSet{} @@ -293,6 +293,20 @@ var _ = Describe("Controllerutil", func() { Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) Expect(controllerutil.RemoveControllerReference(dep2, rs)).To(HaveOccurred()) }) + It("should only delete the controller reference and not the other owner references", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + dep2 := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "bar", UID: "bar-uid"}, + } + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(2)) + Expect(controllerutil.RemoveControllerReference(dep, rs)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) + }) }) From 08a95b1a167c092d09a74bd4dba6481e8e046e38 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Mon, 28 Aug 2023 16:46:35 -0400 Subject: [PATCH 3/4] change to use indexOwnerRef Signed-off-by: Troy Connor --- .../controllerutil/controllerutil.go | 27 +++++++---- .../controllerutil/controllerutil_test.go | 48 +++++++++++++++++-- 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index 0c56967ce9..8503634406 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -93,23 +93,32 @@ func SetControllerReference(owner, controlled metav1.Object, scheme *runtime.Sch // RemoveControllerReference is a helper method to make sure the given object removes a controller reference to the object provided. // This allows you to remove the owner to establish a new owner of the object in a subsequent call. -func RemoveControllerReference(owner, object metav1.Object) error { +func RemoveControllerReference(owner, object metav1.Object, scheme *runtime.Scheme) error { owners := object.GetOwnerReferences() length := len(owners) - result := []metav1.OwnerReference{} if length < 1 { return fmt.Errorf("%T does not have any owner references", object) } - for _, ownerref := range owners { - if ownerref.Name == owner.GetName() { - continue - } - result = append(result, ownerref) + ro, ok := owner.(runtime.Object) + if !ok { + return fmt.Errorf("%T is not a runtime.Object, cannot call RemoveControllerReference", owner) + } + gvk, err := apiutil.GVKForObject(ro, scheme) + if err != nil { + return err } - if len(result) == len(owners) { + + index := indexOwnerRef(owners, metav1.OwnerReference{ + APIVersion: gvk.GroupVersion().String(), + Name: owner.GetName(), + Kind: gvk.Kind, + }) + if index == -1 { return fmt.Errorf("%T does not have an owner reference for %T", object, owner) } - object.SetOwnerReferences(result) + + owners = append(owners[:index], owners[index+1:]...) + object.SetOwnerReferences(owners) return nil } diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index d07ef43eac..5212605ae3 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/pkg/controller/controllerutil/controllerutil_test.go @@ -272,7 +272,7 @@ var _ = Describe("Controllerutil", func() { Controller: &t, BlockOwnerDeletion: &t, })) - Expect(controllerutil.RemoveControllerReference(dep, rs)).NotTo(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(0)) }) It("should fail and return an error if the length is less than 1", func() { @@ -280,7 +280,7 @@ var _ = Describe("Controllerutil", func() { dep := &extensionsv1beta1.Deployment{ ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, } - Expect(controllerutil.RemoveControllerReference(dep, rs)).To(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(dep, rs, scheme.Scheme)).To(HaveOccurred()) }) It("should fail and return an error because the owner doesn't exist to remove", func() { rs := &appsv1.ReplicaSet{} @@ -291,7 +291,7 @@ var _ = Describe("Controllerutil", func() { ObjectMeta: metav1.ObjectMeta{Name: "bar", UID: "bar-uid"}, } Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) - Expect(controllerutil.RemoveControllerReference(dep2, rs)).To(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(dep2, rs, scheme.Scheme)).To(HaveOccurred()) }) It("should only delete the controller reference and not the other owner references", func() { rs := &appsv1.ReplicaSet{} @@ -304,10 +304,48 @@ var _ = Describe("Controllerutil", func() { Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).NotTo(HaveOccurred()) Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(2)) - Expect(controllerutil.RemoveControllerReference(dep, rs)).NotTo(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) + }) + It("should only delete the controller reference and not the other owner references in different order", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + dep2 := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "bar", UID: "bar-uid"}, + } + Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(2)) + Expect(controllerutil.RemoveControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) + }) + It("should only fail because the scheme is wrong for the object", func() { + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + dep2 := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "bar", UID: "bar-uid"}, + } + Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(2)) + Expect(controllerutil.RemoveControllerReference(dep, rs, runtime.NewScheme())).To(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(2)) + }) + It("should only fail because the object is not a runtime.Object", func() { + var obj metav1.Object + rs := &appsv1.ReplicaSet{} + dep := &extensionsv1beta1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid"}, + } + Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) + Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) + Expect(controllerutil.RemoveControllerReference(obj, rs, runtime.NewScheme())).To(HaveOccurred()) Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) }) - }) Describe("CreateOrUpdate", func() { From 16fc13cb88c99b160b0173bc2f457797c311b331 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Wed, 30 Aug 2023 17:06:08 -0400 Subject: [PATCH 4/4] use actual scheme to enforce error being the object and not the scheme Signed-off-by: Troy Connor --- pkg/controller/controllerutil/controllerutil_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/controllerutil/controllerutil_test.go b/pkg/controller/controllerutil/controllerutil_test.go index 5212605ae3..5c481447df 100644 --- a/pkg/controller/controllerutil/controllerutil_test.go +++ b/pkg/controller/controllerutil/controllerutil_test.go @@ -343,7 +343,7 @@ var _ = Describe("Controllerutil", func() { } Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).NotTo(HaveOccurred()) Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) - Expect(controllerutil.RemoveControllerReference(obj, rs, runtime.NewScheme())).To(HaveOccurred()) + Expect(controllerutil.RemoveControllerReference(obj, rs, scheme.Scheme)).To(HaveOccurred()) Expect(len(rs.GetOwnerReferences())).To(BeEquivalentTo(1)) }) })