Spike: disentangle the store, controllers, and annotations code for class matching

[rainest]

Summary

The existing implementation for filtering resources with the correct ingress class annotation or native class has poor separation between the store and annotations code. Store loads in a lot of annotations code that it ideally shouldn't. We should explore options for making the separation cleaner, which should allow for easier changes to this implementation in the future.

Background

#843 works, but is a mess. Key pain points/oddness items for me were:

• The verification functions are generated and exist as part of the store instance. Side effects here are sending a lot of arguments into store.New() to toggle class behavior per-object.
• Generated function constructors only get the ingress class and aren't otherwise distinguished, as matching behavior is now mostly per-object: we pass it into the generated function along with the object details. This would be useful if we had controllers that did use multiple stores to support different classes simultaneously, but I don't think experience has shown we need that much. We do have demonstrated need for finer-grained namespace permissions within a single controller (e.g. Kong enterprise multiple ingress controllers deployment for multiple workspace support helm/charts#19091), and creating multiple stores would allow us a means around the standard K8S client "you can get resources from one namespace or ALL NAMESPACES" limitation, but in that case we'd probably use the same class throughout while creating different stores for each namespace--essentially, we'd implement support for passing multiple --watch-namespace flags to a single controller instance.
• When testing fake_store, we must choose a specific ingress class (that's maybe fine), but have to pull that default out of annotations, which seems wrong.
• Matching logic currently lives in annotations; it is invoked from the generated functions attached to the store instance. That's somewhat odd now that the matching is no longer entirely about annotations (Ingress V1 has that functionality bundled into the Ingress object alongside legacy annotation support, but trying to place that logic outside annotations code would require exporting or moving validIngress), and was always more related to a single specific annotation (ingress.class) than annotations in general.
• We currently have functions for retrieving class info from object meta (annotations) and objects themselves (currently only v1 Ingresses, but maybe other stuff in the future). We also have a function that claims to handle that generically, but in practice I think it's more old code at this point, that we (probably I) forgot to rip it out after adding the object meta variant, and is now only used in tests that don't test actual in-use code.

[rainest]

Some initial comments from @mflendrich pulled out of Slack convo:

One possible way to solve this would be to make an ingress-class-aware view over store.Storer. Possibly even implementing the same interface.

[shaneutt]

@Kong/team-k8s we recently did a code pairing that was relevant to this one, just wanted to check in on this one do we have existing code or is anyone actively working on this?

[rainest]

The pairing ended up addressing a different issue (testing something related). Not started yet, but I think we should keep this around because of how annoying it is.

The OP is now somewhat out of date, and doesn't cover that there's now an additional filter in the controllers, which duplicate the store's filter functionality and has its own set of helper functions, so the original in-store implementation should be possible to discard entirely.

There's hopefully a simple solution (discard filtering in the store altogether, write/review integration tests that confirm that we don't ingest wrong-class resources) so it could be a good onboarding project to gain some familiarity with the store and its relationship to the controllers. May need more in-depth assistance if it turns out being more difficult (the tests fail after removing the store filters).

[shaneutt]

reviewing this it sounds like we need to spike this to figure out what actually should be done to solve the problem, and how long that solution will take.

[mflendrich]

Closed because it's huge. Likely other, more narrowly targeted, efforts to improve the parser/translate code will likely partially resolve this. Let's reopen if there's a good, promising way to fix this.