From d347ff385c1701ba2dbc545b04c1bb56e9fe4767 Mon Sep 17 00:00:00 2001
From: Hasan Turken <turkenh@gmail.com>
Date: Wed, 13 Mar 2024 16:11:09 +0300
Subject: [PATCH] Allow group changes in composed templates

Fixes #5473

Signed-off-by: Hasan Turken <turkenh@gmail.com>
---
 .../composite/composition_render.go           | 14 ++++++-----
 .../composite/composition_render_test.go      | 23 +++++++++----------
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/internal/controller/apiextensions/composite/composition_render.go b/internal/controller/apiextensions/composite/composition_render.go
index 2cb2cb2ff3b..ee7e2dd024f 100644
--- a/internal/controller/apiextensions/composite/composition_render.go
+++ b/internal/controller/apiextensions/composite/composition_render.go
@@ -32,8 +32,8 @@ const (
 	errMarshalProtoStruct = "cannot marshal protobuf Struct to JSON"
 	errSetControllerRef   = "cannot set controller reference"
 
-	errFmtKindOrGroupChanged = "cannot change the kind or group of a composed resource from %s to %s (possible composed resource template mismatch)"
-	errFmtNamePrefixLabel    = "cannot find top-level composite resource name label %q in composite resource metadata"
+	errFmtKindChanged     = "cannot change the kind of a composed resource from %s to %s (possible composed resource template mismatch)"
+	errFmtNamePrefixLabel = "cannot find top-level composite resource name label %q in composite resource metadata"
 
 	// TODO(negz): Include more detail such as field paths if they exist.
 	// Perhaps require each patch type to have a String() method to help
@@ -61,16 +61,18 @@ func RenderFromJSON(o resource.Object, data []byte) error {
 	o.SetName(name)
 	o.SetNamespace(namespace)
 
-	// This resource already had a GK (probably because it already exists), but
+	// This resource already had a Kind (probably because it already exists), but
 	// when we rendered its template it changed. This shouldn't happen. Either
-	// someone changed the kind or group in the template, or we're trying to use the
+	// someone changed the kind in the template, or we're trying to use the
 	// wrong template (e.g. because the order of an array of anonymous templates
 	// changed).
 	// Please note, we don't check for version changes, as versions can change. For example,
 	// if a composed resource was created with a template that has a version of "v1alpha1",
 	// and then the template is updated to "v1beta1", the composed resource will still be valid.
-	if !gvk.Empty() && o.GetObjectKind().GroupVersionKind().GroupKind() != gvk.GroupKind() {
-		return errors.Errorf(errFmtKindOrGroupChanged, gvk, o.GetObjectKind().GroupVersionKind())
+	// We also don't check for group changes, as groups can change during
+	// migrations.
+	if !gvk.Empty() && o.GetObjectKind().GroupVersionKind().Kind != gvk.Kind {
+		return errors.Errorf(errFmtKindChanged, gvk, o.GetObjectKind().GroupVersionKind())
 	}
 
 	return nil
diff --git a/internal/controller/apiextensions/composite/composition_render_test.go b/internal/controller/apiextensions/composite/composition_render_test.go
index e4ecfb8a3f0..a2dd96f24a2 100644
--- a/internal/controller/apiextensions/composite/composition_render_test.go
+++ b/internal/controller/apiextensions/composite/composition_render_test.go
@@ -62,38 +62,37 @@ func TestRenderFromJSON(t *testing.T) {
 				err: errors.Wrap(errInvalidChar, errUnmarshalJSON),
 			},
 		},
-		"ExistingGroupChanged": {
-			reason: "We should return an error if unmarshalling the base template changed the composed resource's group.",
+		"ExistingKindChanged": {
+			reason: "We should return an error if unmarshalling the base template changed the composed resource's kind.",
 			args: args{
 				o: composed.New(composed.FromReference(corev1.ObjectReference{
 					APIVersion: "example.org/v1",
 					Kind:       "Potato",
 				})),
-				data: []byte(`{"apiVersion": "foo.io/v1", "kind": "Potato"}`),
+				data: []byte(`{"apiVersion": "example.org/v1", "kind": "Different"}`),
 			},
 			want: want{
 				o: composed.New(composed.FromReference(corev1.ObjectReference{
-					APIVersion: "foo.io/v1",
-					Kind:       "Potato",
+					APIVersion: "example.org/v1",
+					Kind:       "Different",
 				})),
-				err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "foo.io/v1, Kind=Potato"),
+				err: errors.Errorf(errFmtKindChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"),
 			},
 		},
-		"ExistingKindChanged": {
-			reason: "We should return an error if unmarshalling the base template changed the composed resource's kind.",
+		"GroupCanChange": {
+			reason: "We should accept group changes in the base template.",
 			args: args{
 				o: composed.New(composed.FromReference(corev1.ObjectReference{
 					APIVersion: "example.org/v1",
 					Kind:       "Potato",
 				})),
-				data: []byte(`{"apiVersion": "example.org/v1", "kind": "Different"}`),
+				data: []byte(`{"apiVersion": "foo.io/v1", "kind": "Potato"}`),
 			},
 			want: want{
 				o: composed.New(composed.FromReference(corev1.ObjectReference{
-					APIVersion: "example.org/v1",
-					Kind:       "Different",
+					APIVersion: "foo.io/v1",
+					Kind:       "Potato",
 				})),
-				err: errors.Errorf(errFmtKindOrGroupChanged, "example.org/v1, Kind=Potato", "example.org/v1, Kind=Different"),
 			},
 		},
 		"VersionCanChange": {