Panic in fake client when updating status with Unstructured resource

[scothis]

There is a regression in 0.16.2 in the fake client when attempting to update a resource's status with an unstructured object.

In this case, the GVK has a type registered in the scheme, but the client is invoked using an unstructured object. The initial object was also defined as unstructured and converted to the typed resource inside the tracker.

The panic is triggered at
https://github.com/kubernetes-sigs/controller-runtime/blob/v0.16.2/pkg/client/fake/client.go#L407

• obj is the unstructured value passed to client.Status().Update()
• oldObject is a typed value managed by the tracker

The two types are incompatible and cannot be reflectively assigned, at least not naively.

panic: reflect.Set: value of type v1.MyResource is not assignable to type unstructured.Unstructured [recovered]
	panic: reflect.Set: value of type v1.MyResource is not assignable to type unstructured.Unstructured

goroutine 12 [running]:
testing.tRunner.func1.2({0x105d91e20, 0x140004cc010})
	/opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1545 +0x370
testing.tRunner.func1()
	/opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1548 +0x528
panic({0x105d91e20?, 0x140004cc010?})
	/opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/panic.go:920 +0x254
reflect.Value.assignTo({0x105f00420, 0x14000100900, 0x199}, {0x105b0d7e6, 0xb}, 0x105e6d040, 0x0)
	/opt/homebrew/Cellar/go/1.21.1/libexec/src/reflect/value.go:3307 +0x340
reflect.Value.Set({0x105e6d040, 0x14000683588, 0x199}, {0x105f00420, 0x14000100900, 0x199})
	/opt/homebrew/Cellar/go/1.21.1/libexec/src/reflect/value.go:2260 +0xa8
sigs.k8s.io/controller-runtime/pkg/client/fake.versionedTracker.update({{0x1060581a0, 0x140000a0370}, 0x140002885b0, 0x1400007fe30}, {{0x1400069dde0, 0x1a}, {0x1400069ddfb, 0x2}, {0x140001ae220, 0xd}}, ...)
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.16.2/pkg/client/fake/client.go:407 +0x938
sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).update(0x140000a03c0, {0x10605dd18, 0x14000683588}, 0x1, {0x14000356dc0, 0x1, 0x1})
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.16.2/pkg/client/fake/client.go:789 +0x46c
sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Update(0x14000516f18, {0x106054c50, 0x14000024c30}, {0x10605dd18, 0x14000683588}, {0x0, 0x0, 0x0})

[troy0820]

/kind support bug

[troy0820]

Interesting, I'm trying to see if this is a change with reflect in go1.21. I thought we had a test with this case.
Looking here shows the difference and not sure if using 1.21 is causing this issue:

// Before Go 1.21, ValueOf always escapes and a Value's content
// is always heap allocated.
// Set go121noForceValueEscape to true to avoid the forced escape,
// allowing Value content to be on the stack.
// Set go121noForceValueEscape to false for the legacy behavior
// (for debugging).
const go121noForceValueEscape = true

// ValueOf returns a new Value initialized to the concrete value
// stored in the interface i. ValueOf(nil) returns the zero Value.
func ValueOf(i any) Value {
	if i == nil {
		return Value{}
	}

	if !go121noForceValueEscape {
		escapes(i)
	}

	return unpackEface(i)
}

Thoughts @alvaroaleman ?

I don't believe we are seeing this with go1.20 where controller-runtime is currently at.

[scothis]

I don't believe we are seeing this with go1.20

I also see the panic with go 1.20.8 and 1.19.13. The otherwise same environment works with controller-runtime 0.16.0 and 0.16.1. This is a regression in 0.16.2.

[scothis]

The particular test case I have that is panicking is from reconciler-runtime.

https://github.com/vmware-labs/reconciler-runtime/blob/v0.15.0/reconcilers/resource_test.go#L311-L339

go test -run '^TestResourceReconciler_Unstructured/status_update$' github.com/vmware-labs/reconciler-runtime/reconcilers

Dependabot trying to bump the version https://github.com/vmware-labs/reconciler-runtime/actions/runs/6174150936/job/16758095601?pr=435#step:7:738

[troy0820]

I also see the panic with go 1.20.8 and 1.19.13. The otherwise same environment works with controller-runtime 0.16.0 and 0.16.1. This is a regression in 0.16.2.

Gotcha, (not trying to blame it on the reflect but that changed and looking through the trace led me there).

Also ^^ it wouldn't matter because that wasn't there before (what we added with reflect) to make that issue be the problem.

I don't disagree with you that this may be a regression, just trying to understand the case where this test should've failed in my opinion which does the update on the unstructured.Unstructured status.
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/fake/client_test.go#L1657

I'm trying to reproduce this to better triage this issue but all feedback is welcomed.

[scothis]

Took a stab at fixing the bug in #2495, along with a failing test.

[troy0820]

Thanks for reporting this and pushing up a fix so quickly. I'll look over it but the maintainers are probably gonna be the one to approve/lgtm, etc.