Rework handling of ingress.class annotation

[rainest]

What this PR does / why we need it:
This PR expands the controller's logic for handling the ingress.class annotation, introducing three handling modes:

• "Lazy", the existing behavior for all resources. we load resources if they match the ingress class or if they have no class and we're using the default ingress class ("kong").
• "Require", used for resources that must have a matching ingress.class annotation (KongClusterPlugin and TCPIngress).
• "Ignore", used when we do not care about the resource class, because the resource can only be loaded if referenced by something else that meets one of the other two handlers' requirements. This means that a KongPlugin loaded by an Ingress or a Secret loaded by a KongPlugin do not require ingress.class, which fixes the bug that prevented the controller from following their updates.

Which issue this PR fixes
Fix #733
Fix #734
Fix #735

Special notes for your reviewer:

This is a draft in part because it needs #751 and #764 merged first, and in part because I have some questions, primarily around what tests we need to add. Draft questions/comments go!

Implementation

• The "Ignore" handler currently allows empty-class resources when using non-standard classes. It does not permit mismatched class resources. Should it? We essentially assume that the class annotation doesn't matter for these resources, since the controller only loads them if they're referenced by some non-empty-class resource via the parser. However, pulling in actual mismatches seems a bit wrong, much more so than empty-class resources.

Tests
I checked for existing similar coverage based on an unscientific run of find ./ -name "*_test.go" | xargs grep -ni class.

• main_test.go and flags_test.go don't need additions since they're just testing the class annotation itself, not its interaction with resources.
• status_test.go shouldn't need changes since it only interacts with Ingresses afaik, and passes them through lazy handling (the old behavior for all resources).
• parser_test.go has some existing test with KongClusterPlugins, but those should continue to have them. Everything else presumably omits them and relies on lazy handling, which is fine for the parser, since it's only concerned with resources that it was already able to track updates for (global KongPlugins excepted, but those are going away). Will see after docs(readme) add go report and license badges, spelling GitHub #175 for sure.
• annotations_test.go is updated, with some caveats about one of the tests, see ---
• not clear if store_test.go or fake_store_test.go. This is where I'd expect to add tests for tracking resource changes, but we don't have any such existing tests. we may require them, or may not--we could consider the annotations_test.go additions sufficient, to show that we can now find emptyMatch/IgnoreClassHandling resources independent of their attachment to something in the parser.

Naming

I'm not super-fond of the naming I chose for the class handling system, and it's something that ultimately matters more how others interpret it, so please bikeshed with abandon as to whether the Require/Ignore/Lazy naming scheme (and some of its minor variants) makes sense for describing what those modes actually do.

Comments

Some of these still refer to the old system and need to be replaced. I'd like to handle that once the actual code/naming is finalized rather than trying to rewrite them in advance.

[rainest]

Current failure is

--- FAIL: TestKongSkipClasslessIngress (0.00s)
    parser_test.go:1714: 
                Error Trace:    parser_test.go:1714
                Error:          Not equal: 
                                expected: 0
                                actual  : 1
                Test:           TestKongSkipClasslessIngress
                Messages:       expected zero service to be rendered
FAIL

From

kubernetes-ingress-controller/internal/ingress/controller/parser/parser_test.go

Lines 1675 to 1727 in 4bf2405

	t.Run("Kong classless ingress evaluated (false)", func(t *testing.T) {
	ingresses := []*networking.Ingress{
	{
	ObjectMeta: metav1.ObjectMeta{
	Name: "bar",
	Namespace: "default",
	Annotations: map[string]string{},
	},
	Spec: networking.IngressSpec{
	Rules: []networking.IngressRule{
	{
	Host: "example.com",
	IngressRuleValue: networking.IngressRuleValue{
	HTTP: &networking.HTTPIngressRuleValue{
	Paths: []networking.HTTPIngressPath{
	{
	Path: "/",
	Backend: networking.IngressBackend{
	ServiceName: "foo-svc",
	ServicePort: intstr.FromInt(80),
	},
	},
	},
	},
	},
	},
	},
	},
	},
	}
	services := []*corev1.Service{
	{
	ObjectMeta: metav1.ObjectMeta{
	Name: "foo-svc",
	Namespace: "default",
	},
	},
	}
	store, err := store.NewFakeStore(store.FakeObjects{
	Ingresses: ingresses,
	Services: services,
	SkipClasslessIngress: true,
	})
	assert.Nil(err)
	parser := New(store)
	state, err := parser.Build()
	assert.Nil(err)
	assert.NotNil(state)
	assert.Equal(0, len(state.Services),
	"expected zero service to be rendered")
	})

Need to think over whether that's something the new system actually does wrong or a contrived test that shouldn't run that way in practice and needs adjusting.

[rainest]

Tests now passing with the --skip-empty-annotation semantics from #664 preserved in the new system. IMO this is ready for review by others, with the caveat that it probably needs work after my trying to address several different concerns at once and getting them confused in my head.

Left un-squashed for now because, assuming that there will be change requests, I want to keep the messy history for the benefit of my own memory when I address them. Apologies for the lack of commit atomicity for the PR; please rely on the file diff and assume it will be wrapped up into a single big "revamp class annotation" commit at the end.

[rainest]

Went ahead and added several additional tasks around changing the default class handling to this by means of renaming and flipping the flag default, along with the new consumer class flag. The latter replaces some placeholder code here, and IMO that's good albeit at the cost of increasing the PR scope.

That should also bring the to cover #731 and clear most of the major changes to class handling/matching--rest should be either relatively minor changes, effectively done by virtue of other changes, or expansion of docs&tests. Remaining tests related to that, I think those should probably be in a new PR before closing out those cards, as I'd like to get the lengthy history in this one squashed.

[rainest]

03fefa7 adds varied class tests for annotations, but not for the parser. fakestore (which the parser tests rely on) currently instantiates its store with a hard-coded class (which is the current default, "kong").

It shouldn't be necessary to test a non-default class there, however, as any special behavior was either in the annotations code or REH filters. The former is tested independently. The latter no longer has any class filter, so I don't see much point in trying to test for special behavior there.

CA Certificates currently have (non-special) behavior in the store, but it will go away as part of #739. Knative stuff is more of an issue: #731 (comment)

[hbagdi]

There are skipClassless and processClassless in this patch.
This can lead to boolean logic bugs in the codebase and maintainers pay an additional tax on understanding the code.

Let's simplify. Please update to ensure that the code has only one construct, my preference is processClassless.

[hbagdi]

This table is bad. We should always use named struct parameters. Not in scope for this PR but leaving a note.

[rainest]

Remaining skipClassless was missed in the rework, but obsoleted by 1bac620 for #767 (comment)