Merge pull request #10824 from vincepri/patch-helper-fix

🐛 Patch helper should be able to patch non-spec objects

Author: k8s-ci-robot

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
 }

util/patch/patch_test.go

@@ -31,6 +31,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"
 )
 
@@ -722,6 +723,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{

util/patch/utils.go

@@ -17,19 +17,13 @@ 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"
 )
 
 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
 		}