✨ komega: add EqualObject matcher

[schrej]

This adds an EqualObject matcher to the new komega package which allows to compare objects more conveniently.

It's based on the implementation in cluster-api/internal/test/matchers by @killianmuldoon and @sbueringer but the actual comparison logic is completely rewritten. The reason was the lack of support for arrays/lists, which occur quite frequently (e.g. owner references) and my attempt to add array support wasn't that pretty.
It's now using google's cmp library with a custom reporter. It still allows to ignore and filter the matched paths. The API is mostly the same, with the exception of how paths are specified. Instead of providing the paths as arrays, they can now be written in Go syntax. Both type field names as well as json/yaml names are supported.

[]string{"metadata", "name"}
// turns into
"ObjectMeta.Name"

// arrays
"ObjectMeta.OwnerReferences[0].UID"

// maps, if they do not contain any of .[]/\
"ObjectMeta.Labels.somelabel"

// maps, if they contain any of .[]/\
"ObjectMeta.Labels[kubernetes.io/somelabel]"

CC @fabriziopandini, since you suggested moving it to controller-runtime

/assign @sbueringer

[JoelSpeed]

/lgtm

This looks useful from a testing perspective, code is clean, happy to have this merged if others are

[schrej]

/retest

[sbueringer]

Thank you very much!

At a first glance it looks good.

@killianmuldoon If you have some time, can you please take a look and test if we can use the matcher as is in our tests? (just to make sure there is nothing we're missing by just reviewing)

[schrej]

I've just tested it briefly by replacing the import and it seems like it can be used as a drop-in replacement when no custom IgnorePaths are specified (only once so far, and that's a one line change).

[killianmuldoon]

I think the new way of doing IgnorePaths doesn't work on unstructured.Unstructured objects. When I plug this into our tests as a drop in I get failures across these tests

The issue is that paths for unstructured appear as map[string]interface{} so we get something like

the following fields were expected to match but did not:
        [Object["metadata"]["annotations"] Object["metadata"]["resourceVersion"]]

This is why the library was using json to compare in Cluster API, so as is this doesn't work as a drop in replacement when using unstructured objects.

I think there's a couple of ways to solve this:

1. Go back to generic json comparisons
2. Add handling for filtering paths so that it handles both the e.g. ObjectMeta.ResourceVersion case and the Object[\"metadata\"][\"resourceVersion\"], case

I think the second option fits best with what you're trying to do here and would turn this into a drop in replacement. I'm excited to remove the matchers from Cluster API 😆 so thanks for that @schrej

[schrej]

Then I didn't test thoroughly enough. Should we add another set of default ignores for Unstructured, or do we just add those to the existing one, so it's an actual drop-in replacement?

We could also allow to use single quotes in the path, which would make writing tests that involve maps easier.

[killianmuldoon]

I think it might be better to handle the replicas when doing the filterDiffPaths stage because that would also let any future ignore paths work with Unstructured objects too.

[sbueringer]

I think it might be better to handle the replicas when doing the filterDiffPaths stage because that would also let any future ignore paths work with Unstructured objects too.

I think we can't infer json paths automatically from field names. This will probably work with the builtin metadata fields, but this doesn't work generically.

I think it's even hard to do with metadata:

{"metadata", "uid"}, => "ObjectMeta.UID",
{"metadata", "generation"}, => "ObjectMeta.Generation",
{"metadata", "creationTimestamp"}, => "ObjectMeta.CreationTimestamp",
{"metadata", "resourceVersion"}, => "ObjectMeta.ResourceVersion",
{"metadata", "managedFields"}, => "ObjectMeta.ManagedFields",
{"metadata", "deletionGracePeriodSeconds"}, => "ObjectMeta.DeletionGracePeriodSeconds",
{"metadata", "deletionTimestamp"}, => "ObjectMeta.DeletionTimestamp",
{"metadata", "selfLink"}, => "ObjectMeta.SelfLink",
{"metadata", "generateName"}, => "ObjectMeta.GenerateName",

I guess we can do something by going over JSON tags with reflection but not sure if it's worth the complexity

Might be good enough to just add the metadata paths for unstructured and "real" objects to IgnoreAutogeneratedMetadata. From a user perspective it's just not ideal that depending on if an Unstructured or a real object should be compared different path formats have to be used :/ If somehow possible I would prefer converging on the JSON field names.

[schrej]

I've tried to do so, but wasn't able to find a way quickly. But I've had an idea yesterday evening, so let's see if that works out.

[schrej]

After quite a while I finally got around to working on this again. It's now using a bit of reflection and supports both, go types and yaml. For unstructured only the latter is really useful.
I've changed the ignore syntax a little bit, ptal at the description.

I also tested it against sigs.k8s.io/cluster-api/internal/controllers/topology/cluster and it seems to work. But it requires to use a more generic "metadata.annotations" ignore rule. This is because of unstructured.Unstructured not even containing metadata.annotations at all if there are no annotations, which causes the comparison library not descending deeper in the tree, only adding metadata.annotations to the diverging paths, not metadata.annotations[topology.cluster.x-k8s.io/managed-field-paths] as it should.

[schrej]

I've added all the test cases from the "old" Matcher now. All work, with the exception of three:
As mentioned in the previous comment as well, cmp.Diff doesn't provide the full paths when fields mismatch and one of the compared objects is not nested as deep as the other. This is only the case for unstructured.Unstructured and pointers.

I've looked for a solution, but haven't found any. I'm also not sure if it's worth investing too much time into it. I've kept the tests as comments, maybe we'll be able to improve it in the future.

@killianmuldoon could you take another look?

[sbueringer]

Very nice!

Sorry for the late response.

Just that I understood the limitation correctly. Example:
original has ".spec.parent.child"
modified only has ".spec"

If we want to ignore this diff we have to ignore the entire .spec and not only e.g. .spec.parent.child because the comparison will stop at .spec as modified doesn't have parent/child?

[schrej]

Just that I understood the limitation correctly. Example: original has ".spec.parent.child" modified only has ".spec"

If we want to ignore this diff we have to ignore the entire .spec and not only e.g. .spec.parent.child because the comparison will stop at .spec as modified doesn't have parent/child?

No. You can ignore .spec.parent.
It does notice when a field is present on one object but not the other. It just will not iterate over children of that field. So in your example it would report that .spec.parent does not match, but it will not report .spec.parent.child.

[sbueringer]

No. You can ignore .spec.parent.
It does notice when a field is present on one object but not the other. It just will not iterate over children of that field. So in your example it would report that .spec.parent does not match, but it will not report .spec.parent.child.

Makes sense, thx!

Not sure if it's necessary in this repo, but let's squash and then merge

[schrej]

Doesn't hurt to squash. Was only split in two commits so it's easier to see what changed from your implementation.

@JoelSpeed could you also take another look?

[sbueringer]

Makes sense.

Thank you very much!!
/lgtm

[JoelSpeed]

Looks good thanks

/lgtm

[sbueringer]

/approve

cc @alvaroaleman (we need top-level approval for the go.mod change)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer, schrej

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment