Skip to content

Commit

Permalink
bug: Patch helper should be able to patch non-spec objects
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
vincepri authored and k8s-infra-cherrypick-robot committed Jul 4, 2024
1 parent 4ed135b commit 8d11927
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 16 deletions.
22 changes: 14 additions & 8 deletions util/patch/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
51 changes: 51 additions & 0 deletions util/patch/patch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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{
Expand Down
22 changes: 14 additions & 8 deletions util/patch/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 8d11927

Please sign in to comment.