Skip to content

fake client: SSA issues: opts propagation and dryPatch bug #3267

@ezeriver94

Description

@ezeriver94

I'm trying to test new SSA support for FakeClient (after this PR) and I'm facing some conflict errors returned by apimachinery; I found out if using FieldManager without setting the FieldOwner option, the manager name will end up being unknown and an error similar to Apply failed with 1 conflict: conflict with "unknown" using {APIVERSION}: {FIELDPATH} .
After this, I updated my call to fake client's update methods (Patch/Update) to set client.FieldOwner("something") but realised that it was not being propagated within Patch method (while it was fine for Update).
pkg/client/fake/client.go

// 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
}

I believe action should be created like this:

	action := testing.NewPatchActionWithOptions(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data, *patchOptions.AsPatchOptions())

Something similar happened to CREATE:

if err := c.tracker.Create(gvr, obj, accessor.GetNamespace()); err != nil {
// The managed fields tracker sets gvk even on errors
_ = ensureTypeMeta(obj, gvk)
return err
}

expected:

	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
	}

Another thing, dryPatch function has the following logic in switch branch for SSA Apply:

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())

What is the reason for the deferred function? At least for me this is not working properly, as it tries to add to the tracker the obj, which is the zero instance for the received type after this:

// reset the object in preparation to unmarshal, since unmarshal does not guarantee that fields
// in obj that are removed by patch are cleared
value := reflect.ValueOf(obj)
value.Elem().Set(reflect.New(value.Type().Elem()).Elem())

This leads to the creation of an object with Name: "", Namespace: "" in the tracker; this is not failing but in case the SSA PATCH gets executed again for a different instance of the same type, tracker.Add will fail as it will try to re-add the same empty instance, triggering the panic in line 1272

I was able to reproduce this last issue by extending existing test It("supports server-side apply of a client-go resource via Apply method", func(ctx SpecContext) { to:

	It("supports server-side apply of a client-go resource via Apply method", func(ctx SpecContext) {
		cl := NewClientBuilder().Build()
		obj := corev1applyconfigurations.
			ConfigMap("foo", "default").
			WithData(map[string]string{"some": "data"})

		obj2 := corev1applyconfigurations.
			ConfigMap("foo2", "default").
			WithData(map[string]string{"some": "data"})

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

		cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
		cm2 := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo2", Namespace: "default"}}

		Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), cm)).To(Succeed())
		Expect(cm.Data).To(BeComparableTo(map[string]string{"some": "data"}))

		obj.Data = map[string]string{"other": "data"}
		Expect(cl.Apply(ctx, obj, &client.ApplyOptions{FieldManager: "test-manager"})).To(Succeed())

		Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), cm)).To(Succeed())
		Expect(cm.Data).To(BeComparableTo(map[string]string{"other": "data"}))

		obj.Data = map[string]string{"other": "data"}
		Expect(cl.Apply(ctx, obj2, &client.ApplyOptions{FieldManager: "test-manager"})).To(Succeed())
		Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm2), cm2)).To(Succeed())
		Expect(cm2.Data).To(BeComparableTo(map[string]string{"other": "data"}))
	})

where basically I add a second configmap and fails in the Apply call:

• [PANICKED] [10.264 seconds]
Fake client [It] supports server-side apply of a client-go resource via Apply method
REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client_test.go:2673

  [PANICKED] Test Panicked
  In [It] at: REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client.go:1272 @ 08/04/25 15:29:44.302

  configmaps "" already exists

  Full Stack Trace
    sigs.k8s.io/controller-runtime/pkg/client/fake.dryPatch.func1()
    	REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client.go:1272 +0x1d8
    sigs.k8s.io/controller-runtime/pkg/client/fake.dryPatch({{{0x140005a49c9, 0x7}, {0x1019e34ee, 0x5}, {{0x0, 0x0}, {0x140005a497a, 0x2}, {0x140005a4a20, 0xa}}, ...}, ...}, ...)
    	REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client.go:1278 +0xb84
    sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).patch(0x14000a4e090, {0x101fe6740, 0x14001e78350}, {0x101fc69f8, 0x102cd7ce0}, {0x1400064b680, 0x1, 0x1})
    	REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client.go:1143 +0xd14
    sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Apply(0x14000a4e090, {0x12a30d038, 0x140008ca150}, {0x101fc0720, 0x14001c13170}, {0x14000af3450, 0x1, 0x1})
    	REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client.go:1069 +0x4c4
    sigs.k8s.io/controller-runtime/pkg/client/fake.init.func2.57({0x101fde6a0, 0x140008ca150})
    	REDACTED/Documents/git/external/controller-runtime/pkg/client/fake/client_test.go:2699 +0x1288

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions