Merge pull request #10957 from dlipovetsky/fix-create-target-object-m…

…utated 🐛 Ensure move uses mutated metadata when updating a target object
kubernetes-sigs · Aug 1, 2024 · 0119340 · 0119340
2 parents 3f21042 + 8d73c0e
commit 0119340
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 7 deletions.
diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go
@@ -946,12 +946,10 @@ func (o *objectMover) createTargetObject(ctx context.Context, nodeToCreate *node
 	obj := &unstructured.Unstructured{}
 	obj.SetAPIVersion(nodeToCreate.identity.APIVersion)
 	obj.SetKind(nodeToCreate.identity.Kind)
-	objKey := client.ObjectKey{
-		Namespace: nodeToCreate.identity.Namespace,
-		Name:      nodeToCreate.identity.Name,
-	}
+	obj.SetName(nodeToCreate.identity.Name)
+	obj.SetNamespace(nodeToCreate.identity.Namespace)
 
-	if err := cFrom.Get(ctx, objKey, obj); err != nil {
+	if err := cFrom.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
 		return errors.Wrapf(err, "error reading %q %s/%s",
 			obj.GroupVersionKind(), obj.GetNamespace(), obj.GetName())
 	}
@@ -1006,7 +1004,7 @@ func (o *objectMover) createTargetObject(ctx context.Context, nodeToCreate *node
 			existingTargetObj := &unstructured.Unstructured{}
 			existingTargetObj.SetAPIVersion(obj.GetAPIVersion())
 			existingTargetObj.SetKind(obj.GetKind())
-			if err := cTo.Get(ctx, objKey, existingTargetObj); err != nil {
+			if err := cTo.Get(ctx, client.ObjectKeyFromObject(obj), existingTargetObj); err != nil {
 				return errors.Wrapf(err, "error reading resource for %q %s/%s",
 					existingTargetObj.GroupVersionKind(), existingTargetObj.GetNamespace(), existingTargetObj.GetName())
 			}

diff --git a/cmd/clusterctl/client/cluster/mover_test.go b/cmd/clusterctl/client/cluster/mover_test.go
@@ -1956,6 +1956,7 @@ func Test_createTargetObject(t *testing.T) {
 		fromProxy Proxy
 		toProxy   Proxy
 		node      *node
+		mutators  []ResourceMutatorFunc
 	}
 
 	tests := []struct {
@@ -2075,6 +2076,52 @@ func Test_createTargetObject(t *testing.T) {
 				g.Expect(c.Annotations).To(BeEmpty())
 			},
 		},
+		{
+			name: "updates object whose namespace is mutated, if it already exists and the object is not Global/GlobalHierarchy",
+			args: args{
+				fromProxy: test.NewFakeProxy().WithObjs(
+					&clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: "ns1",
+						},
+					},
+				),
+				toProxy: test.NewFakeProxy().WithObjs(
+					&clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:        "foo",
+							Namespace:   "mutatedns1",
+							Annotations: map[string]string{"foo": "bar"},
+						},
+					},
+				),
+				node: &node{
+					identity: corev1.ObjectReference{
+						Kind:       "Cluster",
+						Namespace:  "ns1",
+						Name:       "foo",
+						APIVersion: "cluster.x-k8s.io/v1beta1",
+					},
+				},
+				mutators: []ResourceMutatorFunc{
+					func(u *unstructured.Unstructured) error {
+						return unstructured.SetNestedField(u.Object,
+							"mutatedns1",
+							"metadata", "namespace")
+					},
+				},
+			},
+			want: func(g *WithT, toClient client.Client) {
+				c := &clusterv1.Cluster{}
+				key := client.ObjectKey{
+					Namespace: "mutatedns1",
+					Name:      "foo",
+				}
+				g.Expect(toClient.Get(context.Background(), key, c)).ToNot(HaveOccurred())
+				g.Expect(c.Annotations).To(BeEmpty())
+			},
+		},
 		{
 			name: "should not update Global objects",
 			args: args{
@@ -2163,7 +2210,7 @@ func Test_createTargetObject(t *testing.T) {
 				fromProxy: tt.args.fromProxy,
 			}
 
-			err := mover.createTargetObject(ctx, tt.args.node, tt.args.toProxy, nil, sets.New[string]())
+			err := mover.createTargetObject(ctx, tt.args.node, tt.args.toProxy, tt.args.mutators, sets.New[string]())
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 				return