Repository: kubernetes-sigs/controller-runtime

Issue:
Subsequent calls to fake.ClientBuilder.Build() causes a panic
Hi,

While attempting to upgrade to  sigs.k8s.io/controller-runtime v0.22.0 from  sigs.k8s.io/controller-runtime v0.21.0, we encountered the following panic in previously passing tests:

```panic: WithObjectTracker and WithTypeConverters are incompatible [recovered]
	panic: WithObjectTracker and WithTypeConverters are incompatible
```

trace:
```
[...]
sigs.k8s.io/controller-runtime/pkg/client/fake.(*ClientBuilder).Build(0x14001178000)
	/Users/bash/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.22.0/pkg/client/fake/client.go:287 +0xbb8
[...]
```

Steps to reproduce:
- Upgrade  to sigs.k8s.io/controller-runtime v0.22.0
- Create a new client builder instance with `fake.NewFakeClientBuilder()`
- Call `Build()` twice on the same instance

Investigation:

It seems that calling `Build()` on an instance of `fake.ClientBuilder` initializes the instance's `objectTracker` and  its `typeConverters`, which are meant to be mutually exclusive.

Ref:
https://github.com/kubernetes-sigs/controller-runtime/blob/a5894805dce56522d851fdbe5b97cf20dfcdcbbd/pkg/client/fake/client.go#L291-L310

A subsequent call to `Build()` will cause a panic, since the following condition will always be met:
https://github.com/kubernetes-sigs/controller-runtime/blob/a5894805dce56522d851fdbe5b97cf20dfcdcbbd/pkg/client/fake/client.go#L286-L288

Test code:
https://github.com/hashicorp/vault-secrets-operator/blob/336f6ca2973424c1df5fec2b87701c06d8fcc193/helpers/template_test.go#L1254

Questions:
- Is the above referenced code expected to result in an invalid ClientBuilder i.e.: cause a panic on subsequent calls to `Build()`?
- Aside from adding our own `ObjectTracker` before calling `Build()` what is the best way forward here?


Thanks!

Discussion:
@alvaroaleman: Can you change your code to not build the same builder multiple times? That this ever worked is a coincidence, completely untested and may have unexpected sideeffect. We can add something to make that more explicit.@sbueringer: Agree, would be probably good to have a panic that just says "Build must not be called multiple times)@benashz: @alvaroaleman thanks for the quick reply. The test has only one ClientBuilder set here: https://github.com/hashicorp/vault-secrets-operator/blob/336f6ca2973424c1df5fec2b87701c06d8fcc193/helpers/template_test.go#L579 - is that what you are referring to?
@benashz: @alvaroaleman is the `ClientBuilder.Build()` meant to only ever be called once? I was under the impression that the builder was acting as a factory for creating new client.WithWatch instances?@troy0820: > Agree, would be probably good to have a panic that just says "Build must not be called multiple times)

We do this for `For` so we can change this so that the `Build` is not done many times.  

Solution patch:
diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go
index 4109b5c5..1d4af89a 100644
--- a/pkg/client/fake/client.go
+++ b/pkg/client/fake/client.go
@@ -141,6 +141,7 @@ type ClientBuilder struct {
 	interceptorFuncs      *interceptor.Funcs
 	typeConverters        []managedfields.TypeConverter
 	returnManagedFields   bool
+	isBuilt               bool
 
 	// indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK.
 	// The inner map maps from index name to IndexerFunc.
@@ -267,6 +268,9 @@ func (f *ClientBuilder) WithReturnManagedFields() *ClientBuilder {
 
 // Build builds and returns a new fake client.
 func (f *ClientBuilder) Build() client.WithWatch {
+	if f.isBuilt {
+		panic("Build() must not be called multiple times when creating a ClientBuilder")
+	}
 	if f.scheme == nil {
 		f.scheme = scheme.Scheme
 	}
@@ -344,6 +348,7 @@ func (f *ClientBuilder) Build() client.WithWatch {
 		result = interceptor.NewClient(result, *f.interceptorFuncs)
 	}
 
+	f.isBuilt = true
 	return result
 }
 



Test patch:
diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go
index 328dfeec..46e2b712 100644
--- a/pkg/client/fake/client_test.go
+++ b/pkg/client/fake/client_test.go
@@ -3182,4 +3182,13 @@ var _ = Describe("Fake client builder", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(called).To(BeTrue())
 	})
+
+	It("should panic when calling build more than once", func() {
+		cb := NewClientBuilder()
+		anotherCb := cb
+		cb.Build()
+		Expect(func() {
+			anotherCb.Build()
+		}).To(Panic())
+	})
 })