Subsequent calls to fake.ClientBuilder.Build() causes a panic

[benashz]

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

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:

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

Lines 291 to 310 in a589480

	if f.objectTracker == nil {
	if len(f.typeConverters) == 0 {
	// Use corresponding scheme to ensure the converter error
	// for types it can't handle.
	clientGoScheme := runtime.NewScheme()
	if err := scheme.AddToScheme(clientGoScheme); err != nil {
	panic(fmt.Sprintf("failed to construct client-go scheme: %v", err))
	}
	f.typeConverters = []managedfields.TypeConverter{
	clientgoapplyconfigurations.NewTypeConverter(clientGoScheme),
	managedfields.NewDeducedTypeConverter(),
	}
	}
	f.objectTracker = testing.NewFieldManagedObjectTracker(
	f.scheme,
	serializer.NewCodecFactory(f.scheme).UniversalDecoder(),
	multiTypeConverter{upstream: f.typeConverters},
	)
	usesFieldManagedObjectTracker = true
	}

A subsequent call to Build() will cause a panic, since the following condition will always be met:

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

Lines 286 to 288 in a589480

	if f.objectTracker != nil && len(f.typeConverters) > 0 {
	panic(errors.New("WithObjectTracker and WithTypeConverters are incompatible"))
	}

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!

[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.