Add custom cmp.Options when comparing k8s objects

[neowulf]

This fix allows adding custom comparator options and is especially useful when non standard k8s objects are used and may require custom comparators.

Resolves #484

[neowulf]

I am unable to assign.

Requesting feedback from @squeedee @mamachanko @scothis

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 61.03%. Comparing base (3180680) to head (f061f6d).

Files	Patch %	Lines
testing/subreconciler.go	0.00%	3 Missing ⚠️
testing/reconciler.go	0.00%	1 Missing ⚠️
testing/webhook.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #486   +/-   ##
=======================================
  Coverage   61.03%   61.03%           
=======================================
  Files          27       27           
  Lines        2556     2556           
=======================================
  Hits         1560     1560           
  Misses        909      909           
  Partials       87       87           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mamachanko]

I recognize your need for working with APIs that reconciler-runtime is not yet appreciatively compatible with! iirc you are working with https://pkg.go.dev/istio.io/client-go/pkg/apis/networking/v1beta1#DestinationRule and https://pkg.go.dev/istio.io/client-go/pkg/apis/networking/v1beta1#WorkloadEntry in particular, aren't you?

In general terms, I am not in favour of a global option. Rather I think we should solve this local to the reconciler. I am not saying that we should revive SemanticEquals (see #266), but it's the level of API I have in mind.

Nonetheless, which particular CmpOptions would you plan to set? I am wondering if those would be candidates to added instead. It's not that uncommon to work with APIs that have protobuf integration. Are looking to set different CmpOptions for tests and production?

It's not relevant yet and I don't ask you to provide those, but tests and docs would help understand the wider context.

[scothis]

I generally agree with @mamachanko about avoiding globals. For the non-test code, the diff is only used for log statements. If there is a way to ignore all unexpected fields, we should use that (I'm not sure there is, at least not built-in).

While I think we should open up cmp options to be user configurable, I personally find depending on a third party API to be an anti-pattern. In controllers I've created that use external apis (outside of the built-in k8s types) I'll create a copy of those types that I control that mirrors the API shape. This not only avoids issues with how the types are defined, but also avoids dependency hell by not pulling in the all the dependencies of that API package. The k8s ecosystem has trouble not making breaking changes in point releases that cause the entire ecosystem to be on either one side of that break or the other. The fewer dependencies you have, the easier it is to upgrade past that breaking change.

[neowulf]

Thank you for the feedback. In order to accommodate these types using this fix , I am able to move on by doing the following:

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

and when I need to use SemanticEquals (irrespective of this PR) , I have been doing this:

func init() {
        // this is for DestinationRule only. Need to define for each Istio data type that's in use 
	if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool {
		return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform())
	}); err != nil {
		panic(err)
	}
}

I am happy to provide test cases and additional documentation to get this MR merged provided this is the fix we want. Which at this point, it seems to me we want to go in a different direction. Please advise.

[neowulf]

Hi folks, requesting feedback for next steps

[mamachanko]

Inspired by @scothis's points, how about the following:

• Try to ignore all unexported fields when diffing for the reconciler logs (@mamachanko)
• Triage if maintaining an copy of upstream istio types works for you (@neowulf)
• Open rtesting's diffing/equality to configuration either by making cmp options user-configurable or a VerifyCreate field in the testsuite or ...

[scothis]

I took a crack at applying a cmp.Option to ignore all unexported fields in #487

This should handle @neowulf's immediate concern. I still think we should open up custom opts, but how to approach that needs a bit more thought.