From b54df3753564e0158b5a59ea059cc7aedc61088b Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Thu, 23 Jun 2022 17:59:49 +0200 Subject: [PATCH] Carry over metadata.uid at ServerSidePatchHelper This prevents the race condition of running server side apply on an object which got: * deleted in the meantime * recreated in the meantime and because of that the reconciler may use outdated information. --- .../topology/cluster/reconcile_state_test.go | 2 +- .../structuredmerge/serversidepathhelper.go | 12 +++++- .../serversidepathhelper_test.go | 43 +++++++++++++++++++ 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 3fca565f4a22..fb192a26baba 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/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) diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go index ed09aa500aa0..809aeaf47a41 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go +++ b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go @@ -50,6 +50,8 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, {"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"}, @@ -59,7 +61,6 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, // If required, convert the original and modified objects to unstructured and filter out all the information // not relevant for the topology controller. - var originalUnstructured *unstructured.Unstructured if !isNil(original) { originalUnstructured = &unstructured.Unstructured{} @@ -93,6 +94,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 @@ -107,7 +116,6 @@ func NewServerSidePatchHelper(original, modified client.Object, c client.Client, modified: modifiedUnstructured.Object, }) } - return &serverSidePatchHelper{ client: c, modified: modifiedUnstructured, diff --git a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go b/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go index 38754108af98..c335e390dec1 100644 --- a/internal/controllers/topology/cluster/structuredmerge/serversidepathhelper_test.go +++ b/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) {