fix(refmanager): adopt() should update child not parent object (#89)

Author: zoumo

controller/refmanager/ref_manager_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/samber/lo"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -250,7 +251,7 @@ func (s *ownerReferenceTestSute) TestClaimWhenCanAdoptFailed() {
 func (s *ownerReferenceTestSute) TestClaimAllOf() {
 	writer := NewOwnerRefWriter(s.client)
 	match, err := LabelSelectorAsMatch(s.owner.Spec.Selector)
-	s.NoError(err)
+	s.Require().NoError(err)
 	m := NewObjectControllerRefManager(writer, s.owner, s.ownerGVK, match)
 
 	adopted := []metav1.Object{
@@ -273,15 +274,43 @@ func (s *ownerReferenceTestSute) TestClaimAllOf() {
 	allObjects = append(allObjects, released...)
 	allObjects = append(allObjects, noChanged...)
 
+	for _, obj := range allObjects {
+		err := s.client.Create(context.TODO(), obj.(client.Object))
+		s.Require().NoError(err)
+	}
+
 	result, err := m.ClaimAllOf(context.Background(), allObjects)
-	if s.NoError(err) {
-		s.Len(result, len(adopted))
-		resultNames := lo.Map(result, func(o metav1.Object, _ int) string {
-			return o.GetName()
-		})
-		adoptedNames := lo.Map(adopted, func(o metav1.Object, _ int) string {
-			return o.GetName()
-		})
-		s.EqualValues(adoptedNames, resultNames)
+	s.Require().NoError(err)
+	// check adopted objects
+	s.Len(result, len(adopted))
+	resultNames := lo.Map(result, func(o metav1.Object, _ int) string {
+		return o.GetName()
+	})
+	adoptedNames := lo.Map(adopted, func(o metav1.Object, _ int) string {
+		return o.GetName()
+	})
+	s.EqualValues(adoptedNames, resultNames)
+
+	forEarch := func(objs []metav1.Object, check func(ownerRef *metav1.OwnerReference)) {
+		for _, obj := range objs {
+			pod := &corev1.Pod{}
+			err := s.client.Get(context.Background(), client.ObjectKeyFromObject(obj.(client.Object)), pod)
+			s.Require().NoError(err)
+			ownerRef := metav1.GetControllerOf(pod)
+			check(ownerRef)
+		}
 	}
+
+	// check adopted object owners
+	forEarch(adopted, func(ownerRef *metav1.OwnerReference) {
+		s.True(ReferSameObject(*ownerRef, s.ownerRef), "object should be adopted")
+	})
+	// check released objects
+	forEarch(released, func(ownerRef *metav1.OwnerReference) {
+		s.Require().Nil(ownerRef)
+	})
+	// check no changed objects
+	forEarch(noChanged, func(ownerRef *metav1.OwnerReference) {
+		s.True(ReferSameObject(*ownerRef, s.deferentOwnerRef), "object should not be changed")
+	})
 }

controller/refmanager/writer.go

@@ -53,7 +53,7 @@ func (w *ownerRefWriter) Adopt(ctx context.Context, parent metav1.Object, parent
 	}
 
 	upsertOwner(child, *newOwner)
-	clientObj, ok := parent.(client.Object)
+	clientObj, ok := child.(client.Object)
 	if !ok {
 		return fmt.Errorf("failed to convert parent object to controller-runtime client.Object")
 	}

controller/refmanager/writer_test.go

@@ -19,6 +19,7 @@ package refmanager
 import (
 	"context"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -28,20 +29,20 @@ func (s *ownerReferenceTestSute) TestAdopt() {
 
 	testcase := []struct {
 		name    string
-		object  client.Object
+		pod     *corev1.Pod
 		wantErr bool
 	}{
 		{
-			name:   "adopt orpha pod",
-			object: newFakePod(nil),
+			name: "adopt orpha pod",
+			pod:  newFakePod(nil),
 		},
 		{
-			name:   "adopt owned pod",
-			object: newFakePod(s.owner),
+			name: "adopt owned pod",
+			pod:  newFakePod(s.owner),
 		},
 		{
 			name:    "adopt pod with conflict",
-			object:  newFakePod(s.deferentOwner),
+			pod:     newFakePod(s.deferentOwner),
 			wantErr: true,
 		},
 	}
@@ -50,15 +51,20 @@ func (s *ownerReferenceTestSute) TestAdopt() {
 		test := testcase[i]
 		s.Run(test.name, func() {
 			// create object
-			err := s.client.Create(context.TODO(), test.object)
+			err := s.client.Create(context.TODO(), test.pod)
 			s.NoError(err)
 
-			err = writer.Adopt(context.TODO(), s.owner, s.ownerGVK, test.object)
+			err = writer.Adopt(context.TODO(), s.owner, s.ownerGVK, test.pod)
 			if test.wantErr {
 				s.Error(err)
 			} else {
 				s.NoError(err)
-				owner := metav1.GetControllerOfNoCopy(test.object)
+				// get pod from client
+				pod := &corev1.Pod{}
+				err = s.client.Get(context.TODO(), client.ObjectKeyFromObject(test.pod), pod)
+				s.Require().NoError(err)
+				owner := metav1.GetControllerOfNoCopy(pod)
+				s.Require().NotNil(owner)
 				s.True(ReferSameObject(s.ownerRef, *owner), "the controller owner reference must be the same")
 			}
 		})
@@ -69,33 +75,37 @@ func (s *ownerReferenceTestSute) TestRelease() {
 	writer := NewOwnerRefWriter(s.client)
 
 	testcase := []struct {
-		name   string
-		object client.Object
+		name string
+		pod  client.Object
 	}{
 		{
-			name:   "release orphan pod, nothing changed",
-			object: newFakePod(nil),
+			name: "release orphan pod, nothing changed",
+			pod:  newFakePod(nil),
 		},
 		{
-			name:   "release owned pod",
-			object: newFakePod(s.owner),
+			name: "release owned pod",
+			pod:  newFakePod(s.owner),
 		},
 		{
-			name:   "release pod controlled by other object",
-			object: newFakePod(s.deferentOwner),
+			name: "release pod controlled by other object",
+			pod:  newFakePod(s.deferentOwner),
 		},
 	}
 
 	for i := range testcase {
 		test := testcase[i]
 		s.Run(test.name, func() {
 			// create object
-			err := s.client.Create(context.TODO(), test.object)
+			err := s.client.Create(context.TODO(), test.pod)
 			s.NoError(err)
 
-			writer.Release(context.TODO(), s.owner, test.object) // nolint
-			owner := metav1.GetControllerOfNoCopy(test.object)
+			writer.Release(context.TODO(), s.owner, test.pod) // nolint
 
+			// get pod from client
+			pod := &corev1.Pod{}
+			err = s.client.Get(context.TODO(), client.ObjectKeyFromObject(test.pod), pod)
+			s.Require().NoError(err)
+			owner := metav1.GetControllerOfNoCopy(pod)
 			if owner != nil {
 				s.False(ReferSameObject(s.ownerRef, *owner), "the controller owner reference must not be the same")
 			}