[release-0.17] ⚠ 🐛 Fakeclient: Do not consider an apply patch to be a strategic merge patch

[k8s-infra-cherrypick-robot]

This is an automated cherry-pick of #2679

/assign alvaroaleman

[alvaroaleman]

/hold

/assign @vincepri @sbueringer

WDYT? This could technically be breaking, but if anyone relied on the previous behavior it means that their tests have nothing to do with reality

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, k8s-infra-cherrypick-robot

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

Sounds good to me! Thx for catching this

/lgtm

[sbueringer]

/hold cancel

[kannon92]

We got hit by this. kubernetes-sigs/kueue#1800. I tried the workaround of creating a fake interceptor for SSA but that doesn't seem to work now.

func NewFakeClient(objs ...client.Object) client.Client {
	return NewClientBuilder().WithObjects(objs...).WithStatusSubresource(objs...).
		WithInterceptorFuncs(interceptor.Funcs{Patch: func(ctx context.Context, clnt client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
			// Apply patches are supposed to upsert, but fake client fails if the object doesn't exist,
			// if an apply patch occurs for an object that doesn't yet exist, create it.
			if patch.Type() != types.ApplyPatchType {
				return clnt.Patch(ctx, obj, patch, opts...)
			}
			check, ok := obj.DeepCopyObject().(client.Object)
			if !ok {
				return errors.New("could not check for object in fake client")
			}
			if err := clnt.Get(ctx, client.ObjectKeyFromObject(obj), check); apierrors.IsNotFound(err) {
				if err := clnt.Create(ctx, check); err != nil {
					return fmt.Errorf("could not inject object creation for fake: %w", err)
				}
			}
			return clnt.Patch(ctx, obj, patch, opts...)
		}}).Build()
}

[alculquicondor]

We were aware of the limitations of the strategic merge patch to represent Apply, but it was working for our use case, when we are not testing against multiple managers.

This shouldn't have been included in a patch release for sure.

[alculquicondor]

Would you accept a PR that allows to create a fake client where Apply is treated as strategic merge patch? Not the default, of course.

[alvaroaleman]

Would you accept a PR that allows to create a fake client where Apply is treated as strategic merge patch? Not the default, of course.

No. That is straight up wrong. An apply patch is not a strategic merge patch. It was completely wrong that this was ever done that way and I apologize for that. The fake client shouldn't claim it does something when it does something else. That gives an incorrect signal in unit tests. If you are confident you know how apply behaves in your context, you can use WithInterceptor to fake that behavior.

[alculquicondor]

If you are confident you know how apply behaves in your context, you can use WithInterceptor to fake that behavior.

Thanks, I wasn't aware of that mechanism. That should be enough for us.