Panics when comparing objects with unexported fields #484

neowulf · 2024-02-22T00:45:05Z

Describe the bug

reconciler-runtime: v0.18.0

Panic occurs when Istio types are used in the Reconciler tests.

panic: cannot handle unexported field at {*v1beta1.DestinationRule}.Spec.state:
	"istio.io/api/networking/v1beta1".DestinationRule
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {*v1beta1.DestinationRule}.Spec.state:
	"istio.io/api/networking/v1beta1".DestinationRule
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported

goroutine 109 [running]:
testing.tRunner.func1.2({0x21987c0, 0xc000703d90})
	/usr/local/Cellar/go/1.21.7/libexec/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
	/usr/local/Cellar/go/1.21.7/libexec/src/testing/testing.go:1548 +0x397
panic({0x21987c0?, 0xc000703d90?})
	/usr/local/Cellar/go/1.21.7/libexec/src/runtime/panic.go:914 +0x21f
github.com/google/go-cmp/cmp.validator.apply({{}}, 0xc0001315e0, {0x22f27a0?, 0xc000261788?, 0x32f6c00?}, {0x22f27a0?, 0xc000260c48?, 0xc000261788?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/options.go:248 +0x39d
github.com/google/go-cmp/cmp.(*state).tryOptions(0xc0001315e0, {0x26ce268, 0x22f27a0}, {0x22f27a0?, 0xc000261788?, 0x2525f78?}, {0x22f27a0?, 0xc000260c48?, 0xc00063edb8?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:306 +0x109
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0001315e0, {0x26af500, 0xc000708100?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:261 +0x4e7
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc0001315e0, {0x26ce268, 0x2344b20}, {0x2344b20?, 0xc000261788?, 0x2525f78?}, {0x2344b20?, 0xc000260c48?, 0xc00063f150?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:414 +0x586
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0001315e0, {0x26af500, 0xc0004fd700?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:289 +0xcee
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc0001315e0, {0x26ce268, 0x22ee1e0}, {0x22ee1e0?, 0xc000261680?, 0xc000261680?}, {0x22ee1e0?, 0xc000260b40?, 0xc00063f4f8?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:414 +0x586
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0001315e0, {0x26af590, 0xc0005981c0?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:289 +0xcee
github.com/google/go-cmp/cmp.(*state).comparePtr(0xc0001315e0, {0x26ce268, 0x23eda40}, {0x23eda40?, 0xc000261680?, 0x100e2ba?}, {0x23eda40?, 0xc000260b40?, 0x106d801?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:566 +0x391
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0001315e0, {0x26af620, 0xc000598180?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:295 +0xb92
github.com/google/go-cmp/cmp.Diff({0x23eda40, 0xc000261680}, {0x23eda40, 0xc000260b40}, {0xc0005636e0?, 0x0?, 0x26a3e40?})
	/Users/siva/go/pkg/mod/github.com/google/go-cmp@v0.6.0/cmp/compare.go:122 +0x75
github.com/vmware-labs/reconciler-runtime/testing.(*ExpectConfig).compareActions(0xc0003d7dc0, 0xc0002c11e0, {0x2430ae6, 0x6}, {0xc00061c460, 0x1, 0x1125760?}, {0xc0004dde70, 0x1, 0x1}, ...)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/config.go:382 +0x30b
github.com/vmware-labs/reconciler-runtime/testing.(*ExpectConfig).AssertClientCreateExpectations(0xc0003d7dc0, 0xc0002c11e0)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/config.go:194 +0x1bc
github.com/vmware-labs/reconciler-runtime/testing.(*ExpectConfig).AssertClientExpectations(0xc0002c11e0?, 0xc0002c11e0)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/config.go:178 +0x46
github.com/vmware-labs/reconciler-runtime/testing.(*ExpectConfig).AssertExpectations(0x2290100?, 0xc0002c11e0)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/config.go:166 +0x3e
github.com/vmware-labs/reconciler-runtime/testing.(*ReconcilerTestCase).Run(0xc000183e00, 0xc0002c11e0, 0xc000272150, 0x2524fe8)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/reconciler.go:218 +0xf05
github.com/vmware-labs/reconciler-runtime/testing.ReconcilerTestSuite.Run.func1(0xc0002c11e0)
	/Users/siva/go/pkg/mod/github.com/vmware-labs/reconciler-runtime@v0.18.0/testing/reconciler.go:249 +0x4e
testing.tRunner(0xc0002c11e0, 0xc0003f02e0)
	/usr/local/Cellar/go/1.21.7/libexec/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 82
	/usr/local/Cellar/go/1.21.7/libexec/src/testing/testing.go:1648 +0x3ad

Reproduction steps

Provide an Istio data type in the ExpectCreates"

import (
   "istio.io/client-go/pkg/apis/networking/v1beta1"
   networkingv1beta1 "istio.io/api/networking/v1beta1"
)
.....
.....
                              ExpectCreates: []client.Object{
				&v1beta1.DestinationRule{
					Spec: networkingv1beta1.DestinationRule{},
		              },

Expected behavior

Panic should not occur.

Additional context

Open to alternative ways of normalizing the istio data types to something the library can understand.

Passing in the unstructured struct doesn't seem to help because of the differing data types:

    config.go:154: ExpectCreates[0] differs (-expected, +actual):
          any(
        - 	&unstructured.Unstructured{
        - ....
        + 	&v1beta1.DestinationRule{
        + 		ObjectMeta: v1.ObjectMeta{
        + 			GenerateName:    "sample-",
        + 			Namespace:       "test-namespace",
        + ....
....

Background

Istio datatypes internally wrap protobuf messages in their k8s datatypes:

The text was updated successfully, but these errors were encountered:

dprotaso · 2024-02-22T02:47:27Z

when using cmp package with protobuf you'll need to use protocmp.Transform() so it doesn't error out on these unexported fields.

https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp

Unsure how to wire that option into reconciler-runtime testing

neowulf · 2024-02-22T04:33:47Z

I have the following snippet in my reconciler code which works when the comparator used in the code is from the k8s.io/apimachinery:

func init() {
	if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool {
		return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform())
	}); err != nil {
		panic(err)
	}
}

As @dprotaso points out - I also don't see how I can add this comparator when the lib is directly using github.com/google/go-cmp.

Can the library be updated to provide custom comparators through init function?

neowulf · 2024-02-22T04:54:36Z

I am thinking we can add a property called Comparators in the testing and reconcilers package:

Comparators = make([]cmp.Option, 0)

And update the existing cmp.Diff functions to do one of the following:

        cmp.Diff(exp, actual, Comparators...)

// or when existing comparators exist:
        cmp.Diff(exp, actual, append(Comparators, NormalizeLabelSelector, NormalizeFieldSelector)...)

The following would be the usage in the test file:

func init() {
	rtesting.Comparators = append(rtesting.Comparators, protocmp.Transform())
}

mamachanko · 2024-02-22T09:19:45Z

@neowulf when you say (emphasis mine)

I have the following snippet in my reconciler code which works when the comparator used in the code is from the k8s.io/apimachinery:
func init() {
	if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool {
		return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform())
	}); err != nil {
		panic(err)
	}
}

do you mean that your tests perform as expected or your reconciler works as expected? I assume the latter but want to double-check.

As for a solution, I appreciate your idea concerning comparators. However, I am thinking more of something similar to VerifyStashedValue (See #311, testing#VerifyStashedValueFunc and readme#stash). Instead of exposing the underlying assertion machinery, i.e. cmp, VerifyStashedValueFunc provides an escape hatch for a fully-customizable assertion function. It can use cmp but is not limited to it.

We have a reconciler that stashes x509.Certificate and this type has un-exported fields as well. We are providing this custom func to VerifyStashedValue:

func verifyStashedValue(t *testing.T, key reconcilers.StashKey, expected, actual interface{}) {
	if diff := cmp.Diff(expected, actual, cmp.AllowUnexported(big.Int{})); diff != "" {
		t.Errorf("Unexpected stash value %q (-expected, +actual): %s", key, diff)
	}
}

A similar approach could be extended to Expect*. The challenge is finding the right level of assertion. While there's one stash to assert against, there's an array of objects/statuses that are CRUD'd. 🤔

The naive

type VerifyExpectCreatesFunc func(t *testing.T, expected, actual []client.Object)
// or
type VerifyExpectCreatesFunc func(expected, actual []client.Object) error

might require the user to be very diligent in reimplementing what https://github.com/vmware-labs/reconciler-runtime/blob/main/testing/config.go#L369-L391 does for them. But it could work.

This should then be replicated for all the CRUD ops. Unless there's a simpler way.

I might be able to draft this for you, but it might take a moment.

mamachanko · 2024-02-22T09:41:42Z

type VerifyExpectCreatesFunc func(expected, actual client.Object) error

might be simpler. reconciler-runtime would continue to assert the cardinality of expected and actual creates. It would call the user-provided VerifyExpectCreates with the actual and expected objects.

neowulf · 2024-02-22T16:41:39Z

Sorry for the confusion. Yes, that snippet is for the reconcilers to run. This bug is regarding the tests.

My test setup is failing when I define the expected as part of the ExpectCreates as part of the ReconcilerTests.

scothis · 2024-03-07T12:40:40Z

Should be resolved by #487

neowulf added the bug Something isn't working label Feb 22, 2024

This was referenced Feb 23, 2024

Add custom cmp.Options when comparing k8s objects #485

Closed

Add custom cmp.Options when comparing k8s objects #486

Closed

scothis mentioned this issue Feb 29, 2024

Allow resources to contain unexported fields #487

Merged

mamachanko closed this as completed Mar 7, 2024

scothis mentioned this issue Mar 26, 2024

Proxy vmware-labs/reconciler-runtime reconcilerio/runtime#484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panics when comparing objects with unexported fields #484

Panics when comparing objects with unexported fields #484

neowulf commented Feb 22, 2024 •

edited

dprotaso commented Feb 22, 2024

neowulf commented Feb 22, 2024 •

edited

neowulf commented Feb 22, 2024 •

edited

mamachanko commented Feb 22, 2024

mamachanko commented Feb 22, 2024

neowulf commented Feb 22, 2024 •

edited

scothis commented Mar 7, 2024

Panics when comparing objects with unexported fields #484

Panics when comparing objects with unexported fields #484

Comments

neowulf commented Feb 22, 2024 • edited

Describe the bug

Reproduction steps

Expected behavior

Additional context

Background

dprotaso commented Feb 22, 2024

neowulf commented Feb 22, 2024 • edited

neowulf commented Feb 22, 2024 • edited

mamachanko commented Feb 22, 2024

mamachanko commented Feb 22, 2024

neowulf commented Feb 22, 2024 • edited

scothis commented Mar 7, 2024

neowulf commented Feb 22, 2024 •

edited

neowulf commented Feb 22, 2024 •

edited

neowulf commented Feb 22, 2024 •

edited

neowulf commented Feb 22, 2024 •

edited