improve handling not existing/recreated objects

internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go

@@ -24,6 +24,7 @@ import (
 	"golang.org/x/net/context"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -53,6 +54,7 @@ type serverSidePatchHelper struct {
 	modified       *unstructured.Unstructured
 	hasChanges     bool
 	hasSpecChanges bool
+	originalUID    types.UID
 }
 
 // NewServerSidePatchHelper returns a new PatchHelper using server side apply.
@@ -78,7 +80,7 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
 
 		// If the object has been created with previous custom approach for tracking managed fields, cleanup the object.
 		if _, ok := original.GetAnnotations()[clusterv1.ClusterTopologyManagedFieldsAnnotation]; ok {
-			if err := cleanupLegacyManagedFields(originalUnstructured, c); err != nil {
+			if err := cleanupLegacyManagedFields(ctx, originalUnstructured, c); err != nil {
 				return nil, errors.Wrap(err, "failed to cleanup legacy managed fields from original object")
 			}
 		}
@@ -98,6 +100,7 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
 	// Determine if the intent defined in the modified object is going to trigger
 	// an actual change when running server side apply, and if this change might impact the object spec or not.
 	var hasChanges, hasSpecChanges bool
+	var originalUID types.UID
 	switch {
 	case isNil(original):
 		hasChanges, hasSpecChanges = true, true
@@ -108,15 +111,17 @@ func NewServerSidePatchHelper(ctx context.Context, original, modified client.Obj
 			modifiedUnstructured,
 			opts...)
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrap(err, "failed to determine changes via dryRunSSAPatch")
 		}
+		originalUID = originalUnstructured.GetUID()
 	}
 
 	return &serverSidePatchHelper{
 		client:         c,
 		modified:       modifiedUnstructured,
 		hasChanges:     hasChanges,
 		hasSpecChanges: hasSpecChanges,
+		originalUID:    originalUID,
 	}, nil
 }
 
@@ -139,6 +144,7 @@ func (h *serverSidePatchHelper) Patch(ctx context.Context) error {
 	log := ctrl.LoggerFrom(ctx)
 	log.V(5).Info("Patching object", "Intent", h.modified)
 
+	h.modified.SetUID(h.originalUID)
 	options := []client.PatchOption{
 		client.FieldOwner(TopologyManagerName),
 		// NOTE: we are using force ownership so in case of conflicts the topology controller
@@ -157,15 +163,11 @@ func dryRunSSAPatch(ctx context.Context, c client.Client, originalUnstructured,
 	dryRunHelperOptions.allowedPaths = append(allowedPaths, []string{"metadata", "managedFields"})
 
 	dryRunUnstructured := modifiedUnstructured.DeepCopy()
+	dryRunUnstructured.SetUID(originalUnstructured.GetUID())
 
 	err := c.Patch(ctx, dryRunUnstructured, client.Apply, client.DryRunAll, client.FieldOwner(TopologyManagerName), client.ForceOwnership)
 	if err != nil {
-		return false, false, err
-	}
-
-	// Ensure it would not create a new object.
-	if dryRunUnstructured.GetUID() != originalUnstructured.GetUID() {
-		return false, false, errors.New("the uid of is not expected to change during server side apply patch dry run")
+		return false, false, errors.Wrap(err, "failed to request dry-run server side apply")
 	}
 
 	// Drop output of dry run all the fields that are not part of our intent.
@@ -175,34 +177,34 @@ func dryRunSSAPatch(ctx context.Context, c client.Client, originalUnstructured,
 	// Compare the output of dry run to the original object.
 	originalJSON, err := json.Marshal(originalUnstructured)
 	if err != nil {
-		return false, false, err
+		return false, false, errors.Wrap(err, "failed to marshal originalUnstructured")
 	}
 	dryRunJSON, err := json.Marshal(dryRunUnstructured)
 	if err != nil {
-		return false, false, err
+		return false, false, errors.Wrap(err, "failed to marshal dryRunUnstructured")
 	}
 
 	rawDiff, err := jsonpatch.CreateMergePatch(originalJSON, dryRunJSON)
 	if err != nil {
-		return false, false, err
+		return false, false, errors.Wrap(err, "failed to create MergePatch for original and dryRun JSON")
 	}
 
 	// Determine if there are changes to the spec and object.
-	diff := &unstructured.Unstructured{}
-	if err := json.Unmarshal(rawDiff, &diff.Object); err != nil {
-		return false, false, err
+	diff := map[string]interface{}{}
+	if err := json.Unmarshal(rawDiff, &diff); err != nil {
+		return false, false, errors.Wrap(err, "failed to unmarshal diff")
 	}
 
-	hasChanges := len(diff.Object) > 0
-	_, hasSpecChanges := diff.Object["spec"]
+	hasChanges := len(diff) > 0
+	_, hasSpecChanges := diff["spec"]
 
 	return hasChanges, hasSpecChanges, nil
 }
 
 // cleanupLegacyManagedFields cleanups managed field management in place before introducing SSA.
 // NOTE: this operation can trigger a machine rollout, but this is considered acceptable given that ClusterClass is still alpha
 // and SSA adoption align the topology controller with K8s recommended solution for many controllers authoring the same object.
-func cleanupLegacyManagedFields(obj *unstructured.Unstructured, c client.Client) error {
+func cleanupLegacyManagedFields(ctx context.Context, obj *unstructured.Unstructured, c client.Client) error {
 	base := obj.DeepCopyObject().(*unstructured.Unstructured)
 
 	// Remove the topology.cluster.x-k8s.io/managed-field-paths annotation
@@ -248,5 +250,5 @@ func cleanupLegacyManagedFields(obj *unstructured.Unstructured, c client.Client)
 
 	obj.SetManagedFields(managedFields)
 
-	return c.Patch(context.TODO(), obj, client.MergeFrom(base))
+	return c.Patch(ctx, obj, client.MergeFrom(base))
 }

internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go

@@ -436,6 +436,42 @@ func TestServerSideApply(t *testing.T) {
 		g.Expect(p0.HasChanges()).To(BeFalse())
 		g.Expect(p0.HasSpecChanges()).To(BeFalse())
 	})
+	t.Run("Error on object which has another uid due to immutability", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Get the current object (assumes tests to be run in sequence).
+		original := obj.DeepCopy()
+		g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(original), original)).To(Succeed())
+
+		// Create a patch helper for a modified object with some changes to what previously applied by th topology manager.
+		modified := obj.DeepCopy()
+		g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed())
+		g.Expect(unstructured.SetNestedField(modified.Object, "changed-by-topology-controller", "spec", "bar")).To(Succeed())
+
+		// Set an other uid to original
+		original.SetUID("a-wrong-one")
+		modified.SetUID("")
+
+		// Create a patch helper which should fail because original's real UID changed.
+		_, err := NewServerSidePatchHelper(ctx, original, modified, env.GetClient())
+		g.Expect(err).To(HaveOccurred())
+	})
+	t.Run("Error on object which does not exist (anymore) but was expected to get updated", func(t *testing.T) {
+		original := builder.TestInfrastructureCluster(ns.Name, "obj3").WithSpecFields(map[string]interface{}{
+			"spec.controlPlaneEndpoint.host": "1.2.3.4",
+			"spec.controlPlaneEndpoint.port": int64(1234),
+			"spec.foo":                       "", // this field is then explicitly ignored by the patch helper
+		}).Build()
+
+		modified := original.DeepCopy()
+
+		// Set a not existing uid to the not existing original object
+		original.SetUID("does-not-exist")
+
+		// Create a patch helper which should fail because original does not exist.
+		_, err = NewServerSidePatchHelper(ctx, original, modified, env.GetClient())
+		g.Expect(err).To(HaveOccurred())
+	})
 }
 
 func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) {