feat(gateway) allow user-defined Listeners

[rainest]

What this PR does / why we need it:
Rework managed Listener handling. The Gateway controller no longer
replaces user-provided Listeners with a set of Listeners derived from
the Kong proxy Service and listen configuration. It only checks to
ensure a Kong listen with the correct protocol is available for the
requested Listener. If there is none, it marks the Listener detached
because its protocol is unsupported, or because no protocol is
configured for the specific port.

Remove allowedRoutes merger. This is unnecessary with user-defined
Listeners preserved.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): prereq for #2474
fixes #2557

Special notes for your reviewer:

• I ran across something that looked odd with Gateway addresses in the course of this, and think we should consider only using the LoadBalancer address for them. Should we? Some earlier discussion at Implement Gateway certificateRef #2474 (comment)
• The new getListenerStatus() function charged with determining both spec-based Listener compatibility and Listener attachability based on Kong configuration is LARGE. I believe it's functionally correct, but it's hard to read. I need to put it down for a bit and come back to figure out how to break it down into more digestible bits. This is now revised a bit to break it down more into functions that hopefully flow more cleanly together. Still large but a lot of that is composing Condition objects over several lines.
• I realized while writing this that my attempts to avoid taking existing Listeners offline because some new Listener introduced a conflict had a bit of a snag: the fields of a Listener that can conflict are not immutable, and changes to them can result in conflict. More on that below.
• E2E test updated because we can't test absent listens in the integration tests. We create all listen types in integration.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[rainest]

Changing a Listener's Protocol, Hostname, or Port (all permitted) invalidates that Listener's previous status calculation. The modified Listener may conflict with some other existing Listener, and essentially needs to be treated as new. This in itself is fine, but it introduces a nasty problem: since Listener status has no information about those fields, we have no idea whether a change occurred unless we keep a copy of the old Gateway around.

Tracking state like this is generally bad practice in Reconcilers, and I agree that it looks kinda ugly, even if it works (I still need to write tests for it to be sure). I briefly discussed this with the SIG and there's no standard around how to handle it, though others have encountered the same problem. There are roughly two approaches in other implementations:

• Let the user footgun and suffer the consequences. Conflicted Listeners are removed from active configuration until fixed, even if they were previously active.
• Keep state and do something like what I've attempted here. In that case, the other implementation does not track state in the controller itself, though I haven't explored what options we have to do the same. It's possible that we could figure out some way to detect the "both broken" state and make it block parser applies, which is annoying but less dangerous than abruptly dropping config.

Consensus was that it was too late in the API design phase to make these fields immutable and enforced by the API webhooks or similar.

[shaneutt]

I went back and forth on this since the meeting last night. The lack of upstream support from Gateway API to solve the problem is a serious issue that we're going to just have to pay for it would seem. I'm starting to lean towards going for the footgun approach for now, the reason being we can easily iterate on top of as we build some experience up with it. If we try to build a lot of custom logic to cleanly handle the problem early, we may end up having a harder time iterating on it.

[rainest]

https://github.com/Kong/kubernetes-ingress-controller/actions/runs/2498691049 is the test E2E run that includes this.

Be careful what you wish for 🙂 making everything conflicted is arguably more complicated because you have to walk through the list again to re-mark anything you'd previously marked okay (which was fine when one, but only one Listener could be ready for a given hostname/port).

This goes ahead and strips Cluster IPs from address status--there wasn't really a clear decision on that, but the community seemed to be leaning towards that one, and the commit is essentially atomic if we want to strip it.

I am utterly confused as to what is going wrong with lint. How is running verify-scripts.sh detecting a diff (in something that it shouldn't inspect at all) on CI and not remotely? https://gist.github.com/rainest/4da9e62249460575f5bc0b39e1c25d94

[pmalek]

I am utterly confused as to what is going wrong with lint. How is running verify-scripts.sh detecting a diff (in something that it shouldn't inspect at all) on CI and not remotely? https://gist.github.com/rainest/4da9e62249460575f5bc0b39e1c25d94

I believe in internal/controllers/gateway/utils.go it strips the //nolint:unparam because all the parameters are being used. Actually if you rebase on main you should get my change that introduced that removal of said comment.

[rainest]

Weird. Different golangci-lint version issues? That appears to be the one tool we don't download to and run from local bin. On mine, I do get:

$ golangci-lint --version
golangci-lint has version v1.45.0 built from (unknown, mod sum: "h1:T2oCVkYoeckBxcNS6DTYiSXN2QcTNuAWaHyLGfqzMlU=") on (unknown)

$ golangci-lint run                                                                                                                       
internal/dataplane/kongstate/upstream.go:87:2: `(*Upstream).override` - `log` is unused (unparam)
	log logrus.FieldLogger,
	^
internal/controllers/gateway/utils.go:24:59: `info` - `keysAndValues` always receives `nil` (unparam)
func info(log logr.Logger, obj client.Object, msg string, keysAndValues ...interface{}) {
                                                          ^

Though I'm also slightly confused since I didn't think that verify tool was capable of touching anything in non-generated code, which is where that lives.

[pmalek]

Weird. Different golangci-lint version issues? That appears to be the one tool we don't download to and run from local bin. On mine, I do get:

$ golangci-lint --version
golangci-lint has version v1.45.0 built from (unknown, mod sum: "h1:T2oCVkYoeckBxcNS6DTYiSXN2QcTNuAWaHyLGfqzMlU=") on (unknown)

$ golangci-lint run                                                                                                                       
internal/dataplane/kongstate/upstream.go:87:2: `(*Upstream).override` - `log` is unused (unparam)
	log logrus.FieldLogger,
	^
internal/controllers/gateway/utils.go:24:59: `info` - `keysAndValues` always receives `nil` (unparam)
func info(log logr.Logger, obj client.Object, msg string, keysAndValues ...interface{}) {
                                                          ^

Though I'm also slightly confused since I didn't think that verify tool was capable of touching anything in non-generated code, which is where that lives.

Yeah, so there's this other thing which is the fact that go-get-tool (described in: #2560) doesn't download new version/check for the current version of the tool accepted as parameter. This is why you're still running v1.45.0 even though that your branch has v1.46.2.

And indeed that seems to be the culprit (version mismatch). After downloading v1.45.0 I can confirm that I'm getting the same errors.

This is probably due to the fact that the newer versions of golangci-lint are "1.18 aware" but not all linters support it yet.

WARN [linters context] nilerr is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649.
WARN [linters context] structcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649.
WARN [linters context] unparam is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649.
WARN [linters context] wastedassign is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649.

[shaneutt]

It seems that we're looking to accept this as an incomplete implementation, more or less: I noticed we have a lot of TODOs without any links to their follow up issues. We should be sure that we link follow-ups, and in the follow-up issues it would be helpful to make sure there's a reminder to clean up comments which reference the issue as a part of resolution.

[rainest]

It seems that we're looking to accept this as an incomplete implementation, more or less: I noticed we have a lot of TODOs without any links to their follow up issues. We should be sure that we link follow-ups, and in the follow-up issues it would be helpful to make sure there's a reminder to clean up comments which reference the issue as a part of resolution.

@shaneutt which are you seeing unlinked still? They should all have links, though they're mostly at the bottom of the comment blocks instead of the start.

Cleanup links in the issue body are a good idea, though I'd want to link them to a commit in main instead of the temp commits here that'll be squashed away.

[shaneutt]

I think I was mistaken about missing links, let's merge this and then we can iterate on it from here in the follow-up issues.