Merge pull request kubernetes-sigs#6742 from chrischdi/pr-ssa-prevent-orphaned-creation

🐛 Carry over metadata.uid at ServerSidePatchHelper

Author: k8s-ci-robot

internal/controllers/topology/cluster/reconcile_state_test.go

@@ -1660,7 +1660,7 @@ func TestReconcileMachineDeployments(t *testing.T) {
 	md8Update := newFakeMachineDeploymentTopologyState("md-8-update", infrastructureMachineTemplate8Update, bootstrapTemplate8Update, nil)
 	infrastructureMachineTemplate8UpdateWithChanges := infrastructureMachineTemplate8Update.DeepCopy()
 	g.Expect(unstructured.SetNestedField(infrastructureMachineTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed())
-	bootstrapTemplate8UpdateWithChanges := bootstrapTemplate3.DeepCopy()
+	bootstrapTemplate8UpdateWithChanges := bootstrapTemplate8Update.DeepCopy()
 	g.Expect(unstructured.SetNestedField(bootstrapTemplate8UpdateWithChanges.Object, "foo", "spec", "template", "spec", "foo")).To(Succeed())
 	md8UpdateWithRotatedTemplates := newFakeMachineDeploymentTopologyState("md-8-update", infrastructureMachineTemplate8UpdateWithChanges, bootstrapTemplate8UpdateWithChanges, nil)
 

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

@@ -31,6 +31,8 @@ var (
 		{"kind"},
 		{"metadata", "name"},
 		{"metadata", "namespace"},
+		// uid is optional for a server side apply intent but sets the expectation of an object getting created or a specific one updated.
+		{"metadata", "uid"},
 		// the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only.
 		{"metadata", "labels"},
 		{"metadata", "annotations"},
@@ -49,6 +51,8 @@ var (
 		{"kind"},
 		{"metadata", "name"},
 		{"metadata", "namespace"},
+		// uid is optional for a server side apply intent but sets the expectation of an object getting created or a specific one updated.
+		{"metadata", "uid"},
 		// the topology controller controls/has an opinion for the labels ClusterLabelName
 		// and ClusterTopologyOwnedLabel as well as infrastructureRef and controlPlaneRef in spec.
 		{"metadata", "labels", clusterv1.ClusterLabelName},

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

@@ -81,6 +81,14 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client,
 	}
 	filterObject(modifiedUnstructured, helperOptions)
 
+	// Carry over uid to match the intent to:
+	// * create (uid==""): Setting no uid ensures an object gets created, not updated.
+	// * update (uid!=""): Setting an uid ensures an object which has the same uid gets updated.
+	modifiedUnstructured.SetUID("")
+	if originalUnstructured != nil {
+		modifiedUnstructured.SetUID(originalUnstructured.GetUID())
+	}
+
 	// 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

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

@@ -386,6 +386,49 @@ 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.
+		p0, err := NewServerSidePatchHelper(original, modified, env.GetClient())
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(p0.HasChanges()).To(BeTrue())
+		g.Expect(p0.HasSpecChanges()).To(BeTrue())
+		g.Expect(p0.Patch(ctx)).ToNot(Succeed())
+	})
+	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()
+		g.Expect(unstructured.SetNestedField(modified.Object, "changed", "spec", "controlPlaneEndpoint", "host")).To(Succeed())
+
+		// 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.
+		p0, err := NewServerSidePatchHelper(original, modified, env.GetClient())
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(p0.HasChanges()).To(BeTrue())
+		g.Expect(p0.HasSpecChanges()).To(BeTrue())
+		g.Expect(p0.Patch(ctx)).ToNot(Succeed())
+	})
 }
 
 func TestServerSideApply_CleanupLegacyManagedFields(t *testing.T) {