From 8d1192734d4f90f3ad99c8517668442724f62a8a Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Wed, 3 Jul 2024 08:33:48 -0700 Subject: [PATCH] bug: Patch helper should be able to patch non-spec objects Currently, patch helper is only useful for CRD or any other object that has spec/status fields, where status is considered a subresource. Some types like ConfigMap (and others) don't rely on spec being a top level resource, but instead have other top level fields like `data`. Signed-off-by: Vince Prignano --- util/patch/patch.go | 22 ++++++++++------- util/patch/patch_test.go | 51 ++++++++++++++++++++++++++++++++++++++++ util/patch/utils.go | 22 ++++++++++------- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/util/patch/patch.go b/util/patch/patch.go index dc807abe7804..de6601bd52e6 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -44,7 +45,7 @@ type Helper struct { beforeObject client.Object before *unstructured.Unstructured after *unstructured.Unstructured - changes map[string]bool + changes sets.Set[string] isConditionsSetter bool } @@ -157,7 +158,7 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e // patch issues a patch for metadata and spec. func (h *Helper) patch(ctx context.Context, obj client.Object) error { - if !h.shouldPatch("metadata") && !h.shouldPatch("spec") { + if !h.shouldPatch(specPatch) { return nil } beforeObject, afterObject, err := h.calculatePatch(obj, specPatch) @@ -169,7 +170,7 @@ func (h *Helper) patch(ctx context.Context, obj client.Object) error { // patchStatus issues a patch if the status has changed. func (h *Helper) patchStatus(ctx context.Context, obj client.Object) error { - if !h.shouldPatch("status") { + if !h.shouldPatch(statusPatch) { return nil } beforeObject, afterObject, err := h.calculatePatch(obj, statusPatch) @@ -285,13 +286,18 @@ func (h *Helper) calculatePatch(afterObj client.Object, focus patchType) (client return beforeObj, afterObj, nil } -func (h *Helper) shouldPatch(in string) bool { - return h.changes[in] +func (h *Helper) shouldPatch(focus patchType) bool { + if focus == specPatch { + // If we're looking to patch anything other than status, + // return true if the changes map has any fields after removing `status`. + return h.changes.Clone().Delete("status").Len() > 0 + } + return h.changes.Has(string(focus)) } // calculate changes tries to build a patch from the before/after objects we have // and store in a map which top-level fields (e.g. `metadata`, `spec`, `status`, etc.) have changed. -func (h *Helper) calculateChanges(after client.Object) (map[string]bool, error) { +func (h *Helper) calculateChanges(after client.Object) (sets.Set[string], error) { // Calculate patch data. patch := client.MergeFrom(h.beforeObject) diff, err := patch.Data(after) @@ -306,9 +312,9 @@ func (h *Helper) calculateChanges(after client.Object) (map[string]bool, error) } // Return the map. - res := make(map[string]bool, len(patchDiff)) + res := sets.New[string]() for key := range patchDiff { - res[key] = true + res.Insert(key) } return res, nil } diff --git a/util/patch/patch_test.go b/util/patch/patch_test.go index 269167a18b91..7f517fcd2163 100644 --- a/util/patch/patch_test.go +++ b/util/patch/patch_test.go @@ -32,6 +32,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" ) @@ -723,6 +724,56 @@ func TestPatchHelper(t *testing.T) { }) }) + t.Run("should patch a corev1.ConfigMap object", func(t *testing.T) { + g := NewWithT(t) + + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "node-patch-test-", + Namespace: ns.Name, + Annotations: map[string]string{ + "test": "1", + }, + }, + Data: map[string]string{ + "1": "value", + }, + } + + t.Log("Creating a ConfigMap object") + g.Expect(env.Create(ctx, obj)).To(Succeed()) + defer func() { + g.Expect(env.Delete(ctx, obj)).To(Succeed()) + }() + key := util.ObjectKey(obj) + + t.Log("Checking that the object has been created") + g.Eventually(func() error { + obj := obj.DeepCopy() + return env.Get(ctx, key, obj) + }).Should(Succeed()) + + t.Log("Creating a new patch helper") + patcher, err := NewHelper(obj, env) + g.Expect(err).ToNot(HaveOccurred()) + + t.Log("Adding a new Data value") + obj.Data["1"] = "value1" + obj.Data["2"] = "value2" + + t.Log("Patching the ConfigMap") + g.Expect(patcher.Patch(ctx, obj)).To(Succeed()) + + t.Log("Validating the object has been updated") + objAfter := &corev1.ConfigMap{} + g.Eventually(func() bool { + g.Expect(env.Get(ctx, key, objAfter)).To(Succeed()) + return len(objAfter.Data) == 2 + }, timeout).Should(BeTrue()) + g.Expect(objAfter.Data["1"]).To(Equal("value1")) + g.Expect(objAfter.Data["2"]).To(Equal("value2")) + }) + t.Run("Should update Status.ObservedGeneration when using WithStatusObservedGeneration option", func(t *testing.T) { obj := &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ diff --git a/util/patch/utils.go b/util/patch/utils.go index ef047f8d36a7..1a35291c5f49 100644 --- a/util/patch/utils.go +++ b/util/patch/utils.go @@ -17,8 +17,6 @@ limitations under the License. package patch import ( - "strings" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -26,10 +24,6 @@ import ( type patchType string -func (p patchType) Key() string { - return strings.Split(string(p), ".")[0] -} - const ( specPatch patchType = "spec" statusPatch patchType = "status" @@ -81,8 +75,20 @@ func unsafeUnstructuredCopy(obj *unstructured.Unstructured, focus patchType, isC for key := range obj.Object { value := obj.Object[key] - // Perform a shallow copy only for the keys we're interested in, or the ones that should be always preserved. - if key == focus.Key() || preserveUnstructuredKeys[key] { + preserve := false + switch focus { + case specPatch: + // For what we define as `spec` fields, we should preserve everything + // that's not `status`. + preserve = key != string(statusPatch) + case statusPatch: + // For status, only preserve the status fields. + preserve = key == string(focus) + } + + // Perform a shallow copy only for the keys we're interested in, + // or the ones that should be always preserved (like metadata). + if preserve || preserveUnstructuredKeys[key] { res.Object[key] = value }