Skip to content

🐛 Fakeclient: Fix some SSA-related bugs #3268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+154 −36
Open

🐛 Fakeclient: Fix some SSA-related bugs #3268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 51 additions & 36 deletions pkg/client/fake/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
https://github.com/kubernetes/kubernetes/pull/120326 (v5.6.0+incompatible
missing a critical fix)
*/

jsonpatch "gopkg.in/evanphx/json-patch.v4"
appsv1 "k8s.io/api/apps/v1"
authenticationv1 "k8s.io/api/authentication/v1"
Expand Down Expand Up @@ -717,6 +718,10 @@ func (c *fakeClient) List(ctx context.Context, obj client.ObjectList, opts ...cl
if err := ensureTypeMeta(o, gvk); err != nil {
return err
}

if !c.returnManagedFields {
o.(metav1.Object).SetManagedFields(nil)
}
}

if listOpts.LabelSelector == nil && listOpts.FieldSelector == nil {
Expand Down Expand Up @@ -870,7 +875,7 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
c.trackerWriteLock.Lock()
defer c.trackerWriteLock.Unlock()

if err := c.tracker.Create(gvr, obj, accessor.GetNamespace()); err != nil {
if err := c.tracker.Create(gvr, obj, accessor.GetNamespace(), *createOptions.AsCreateOptions()); err != nil {
// The managed fields tracker sets gvk even on errors
_ = ensureTypeMeta(obj, gvk)
return err
Expand Down Expand Up @@ -1113,11 +1118,7 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
return err
}

data, err := patch.Data(obj)
if err != nil {
return err
}

var isApplyCreate bool
c.trackerWriteLock.Lock()
defer c.trackerWriteLock.Unlock()
oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName())
Expand All @@ -1126,32 +1127,59 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client
return err
}
oldObj = &unstructured.Unstructured{}
isApplyCreate = true
}
oldAccessor, err := meta.Accessor(oldObj)
if err != nil {
return err
}

// Apply patch without updating object.
// To remain in accordance with the behavior of k8s api behavior,
// a patch must not allow for changes to the deletionTimestamp of an object.
// The reaction() function applies the patch to the object and calls Update(),
// whereas dryPatch() replicates this behavior but skips the call to Update().
// This ensures that the patch may be rejected if a deletionTimestamp is modified, prior
// to updating the object.
action := testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data)
o, err := dryPatch(action, c.tracker, obj)
if err != nil {
return err
// SSA deletionTimestamp updates are silently ignored
if patch.Type() == types.ApplyPatchType && !isApplyCreate {
obj.SetDeletionTimestamp(oldAccessor.GetDeletionTimestamp())
}
newObj, err := meta.Accessor(o)

data, err := patch.Data(obj)
if err != nil {
return err
}

// Validate that deletionTimestamp has not been changed
if !deletionTimestampEqual(newObj, oldAccessor) {
return fmt.Errorf("rejected patch, metadata.deletionTimestamp immutable")
action := testing.NewPatchActionWithOptions(
gvr,
accessor.GetNamespace(),
accessor.GetName(),
patch.Type(),
data,
*patchOptions.AsPatchOptions(),
)

// Apply is implemented in the tracker and calling it has side-effects
// such as bumping RV and updating managedFields timestamps, hence we
// can not dry-run it. Luckily, the only validation we use it for
// doesn't apply to SSA - Creating objects with non-nil deletionTimestamp
// through SSA is possible and updating the deletionTimestamp is valid,
// but has no effect.
if patch.Type() != types.ApplyPatchType {
// Apply patch without updating object.
// To remain in accordance with the behavior of k8s api behavior,
// a patch must not allow for changes to the deletionTimestamp of an object.
// The reaction() function applies the patch to the object and calls Update(),
// whereas dryPatch() replicates this behavior but skips the call to Update().
// This ensures that the patch may be rejected if a deletionTimestamp is modified, prior
// to updating the object.
o, err := dryPatch(action, c.tracker)
if err != nil {
return err
}
newObj, err := meta.Accessor(o)
if err != nil {
return err
}

// Validate that deletionTimestamp has not been changed
if !deletionTimestampEqual(newObj, oldAccessor) {
return fmt.Errorf("rejected patch, metadata.deletionTimestamp immutable")
}
}

reaction := testing.ObjectReaction(c.tracker)
Expand Down Expand Up @@ -1206,7 +1234,7 @@ func deletionTimestampEqual(newObj metav1.Object, obj metav1.Object) bool {
// This results in some code duplication, but was found to be a cleaner alternative than unmarshalling and introspecting the patch data
// and easier than refactoring the k8s client-go method upstream.
// Duplicate of upstream: https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194
func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, newObj runtime.Object) (runtime.Object, error) {
func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (runtime.Object, error) {
ns := action.GetNamespace()
gvr := action.GetResource()

Expand Down Expand Up @@ -1262,20 +1290,7 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, new
case types.ApplyCBORPatchType:
return nil, errors.New("apply CBOR patches are not supported in the fake client")
case types.ApplyPatchType:
// There doesn't seem to be a way to test this without actually applying it as apply is implemented in the tracker.
// We have to make sure no reader sees this and we can not handle errors resetting the obj to the original state.
defer func() {
if unstructured, isUnstructured := obj.(*unstructured.Unstructured); isUnstructured && unstructured.GetKind() == "" {
unstructured.SetGroupVersionKind(newObj.GetObjectKind().GroupVersionKind())
}
if err := tracker.Add(obj); err != nil {
panic(err)
}
}()
if err := tracker.Apply(gvr, newObj, ns, action.PatchOptions); err != nil {
return nil, err
}
return tracker.Get(gvr, ns, action.GetName())
return nil, errors.New("bug in controller-runtime: should not end up in dryPatch for SSA")
default:
return nil, fmt.Errorf("%s PatchType is not supported", action.GetPatchType())
}
Expand Down
103 changes: 103 additions & 0 deletions pkg/client/fake/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2668,6 +2668,27 @@ var _ = Describe("Fake client", func() {
_, exists, err := unstructured.NestedFieldNoCopy(u.Object, "metadata", "managedFields")
Expect(err).NotTo(HaveOccurred())
Expect(exists).To(BeTrue())

var list corev1.ConfigMapList
Expect(cl.List(ctx, &list)).NotTo(HaveOccurred())
for _, item := range list.Items {
Expect(item.ManagedFields).NotTo(BeNil())
}
})

It("clears managedFields from objects in a list", func(ctx SpecContext) {
cl := NewClientBuilder().Build()
obj := corev1applyconfigurations.
ConfigMap("foo", "default").
WithData(map[string]string{"some": "data"})

Expect(cl.Apply(ctx, obj, &client.ApplyOptions{FieldManager: "test-manager"})).To(Succeed())

var list corev1.ConfigMapList
Expect(cl.List(ctx, &list)).NotTo(HaveOccurred())
for _, item := range list.Items {
Expect(item.ManagedFields).To(BeNil())
}
})

It("supports server-side apply of a client-go resource via Apply method", func(ctx SpecContext) {
Expand Down Expand Up @@ -2736,6 +2757,88 @@ var _ = Describe("Fake client", func() {
Expect(result.Object["spec"]).To(Equal(map[string]any{"other": "data"}))
})

It("sets managed fields through all methods", func(ctx SpecContext) {
owner := "test-owner"
cl := client.WithFieldOwner(
NewClientBuilder().WithReturnManagedFields().Build(),
owner,
)

obj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo"},
Data: map[string]string{"method": "create"},
}
Expect(cl.Create(ctx, obj)).NotTo(HaveOccurred())

Expect(obj.ManagedFields).NotTo(BeEmpty())
for _, f := range obj.ManagedFields {
Expect(f.Manager).To(BeEquivalentTo(owner))
}

originalObj := obj.DeepCopy()
obj.Data["method"] = "patch"
Expect(cl.Patch(ctx, obj, client.MergeFrom(originalObj))).NotTo(HaveOccurred())
Expect(obj.ManagedFields).NotTo(BeEmpty())
for _, f := range obj.ManagedFields {
Expect(f.Manager).To(BeEquivalentTo(owner))
}

obj.Data["method"] = "update"
Expect(cl.Update(ctx, obj)).NotTo(HaveOccurred())
Expect(obj.ManagedFields).NotTo(BeEmpty())
for _, f := range obj.ManagedFields {
Expect(f.Manager).To(BeEquivalentTo(owner))
}
})

// GH-3267
It("Doesn't leave stale data when updating an object through SSA", func(ctx SpecContext) {
obj := corev1applyconfigurations.
ConfigMap("foo", "default").
WithData(map[string]string{"some": "data"})

cl := NewClientBuilder().Build()
Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred())

obj.WithData(map[string]string{"bar": "baz"})
Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred())
var cms corev1.ConfigMapList
Expect(cl.List(ctx, &cms)).NotTo(HaveOccurred())
Expect(len(cms.Items)).To(BeEquivalentTo(1))
})

It("allows to set deletionTimestamp on an object during SSA create", func(ctx SpecContext) {
now := metav1.Now()
obj := corev1applyconfigurations.
ConfigMap("foo", "default").
WithDeletionTimestamp(now).
WithData(map[string]string{"some": "data"})

cl := NewClientBuilder().Build()
Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred())

Expect(obj.DeletionTimestamp).To(BeEquivalentTo(&now))
})

It("will silently ignore a deletionTimestamp update through SSA", func(ctx SpecContext) {
Skip("the apply logic in the managedFieldObjectTracker seems to override this")
now := metav1.Now()
obj := corev1applyconfigurations.
ConfigMap("foo", "default").
WithDeletionTimestamp(now).
WithFinalizers("foo.bar").
WithData(map[string]string{"some": "data"})

cl := NewClientBuilder().Build()
Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred())
Expect(obj.DeletionTimestamp).To(BeEquivalentTo(&now))

later := metav1.Time{Time: now.Add(time.Second)}
obj.DeletionTimestamp = &later
Expect(cl.Apply(ctx, obj, client.FieldOwner("foo"))).NotTo(HaveOccurred())
Expect(*obj.DeletionTimestamp).To(BeEquivalentTo(now))
})

scalableObjs := []client.Object{
&appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Expand Down