Clarify support for dry-run in fake client write calls (SSA/Patch/Update/...)

[sbueringer]

I think we should either implement dry-run or return errors instead of just nil if someone tryes to use dry-run with the fake client.

xref:

controller-runtime/pkg/client/fake/client.go

Line 913 in 9893d5b

return nil

Returning nil seems very misleading as that is very different than a real dry-run.

Independent of that the Apply func does not detect dry-run correctly today as fakeClient.patch only checks for patchOptions.DryRun, not for patchOptions.Raw.DryRun

I'm not entirely sure what the issue here is:

• that we don't check Raw (which could be something that we are also missing in other places), xref:

  controller-runtime/pkg/client/fake/client.go

  Line 911 in 9893d5b

  for _, dryRunOpt := range patchOptions.DryRun {

• that the field is only set in Raw but not top-level (which is probably specific to Apply), xref:

  controller-runtime/pkg/client/fake/client.go

  Line 858 in 9893d5b

  patchOpts.Raw = applyOpts.AsPatchOptions()

[alvaroaleman]

I think we should either implement dry-run or return errors instead of just nil if someone tryes to use dry-run with the fake client.

What does it mean to "implement dry-run"?

[sbueringer]

The missing piece today is to update the passed in object like it would have been if the call was actually executed.

The way we are using dry-run in CAPI, this is the main feature of dry-run. That you can apply an object and figure out how it would change if you would actually do the call.

I guess in fake client we could only actually "execute the call" and then rollback the state of the object in the fake client?

[alvaroaleman]

I guess in fake client we could only actually "execute the call" and then rollback the state of the object in the fake client?

We can try. I don't know how easy this is though, I think for SSA I removed some dry-patch functionality that did exactly that because it had side-effects

[sbueringer]

Yup. I think it would be also okay to not implement it and clearly error out. This is definitely an area where folks should really consider using envtest or a real Kubernetes cluster for tests anyway.
(in reality you often also have mutating webhooks that are called as part of dry-run)

I mostly just don't like silently doing nothing.

[alvaroaleman]

Yup. I think it would be also okay to not implement it and clearly error out.

I disagree, the silently doing nothing is a minimal version of dry-run, there are existing tests that will break if we start erroring out there

[sbueringer]

Yup. But to make a comparison. fake client has for a long time been setting TypeMeta. When we stopped setting TypeMeta this broke a lot of tests everywhere. Similarly we had a broken SSA implementation for a long time and then started erroring out until it was properly implemented.

I think here we are also silently doing the wrong thing, which similarly can also mask production code bugs. I think this change here would have a much much lower impact.

But of course I'm also fine with just implementing dry-run correctly

[alvaroaleman]

Yup. But to make a comparison. fake client has for a long time been setting TypeMeta. When we stopped setting TypeMeta this broke a lot of tests everywhere. Similarly we had a broken SSA implementation for a long time and then started erroring out until it was properly implemented.

It is not that we can not do a breaking change, the problem is that we have something that is good enough for some cases. I don't think we should remove that just because its not good enough for other cases. The fake client is generally just an approximation of the apiserver and not doing exactly what the apiserver does doesn't mean it does the wrong thing.

[sbueringer]

Okay for me