support many Gateway Listeners #5160

skriss · 2023-03-09T19:18:21Z

Adds support for programming an arbitrary number
of Gateway listeners in Envoy and the Envoy service.

Closes #4960.

skriss · 2023-03-09T19:41:18Z

internal/provisioner/objects/dataplane/dataplane.go

@@ -212,7 +202,6 @@ func desiredContainers(contour *model.Contour, contourImage, envoyImage string)
 				SuccessThreshold:    int32(1),
 				TimeoutSeconds:      int32(1),
 			},
-			Ports: ports,


In testing this, I realized that every time a new Gateway Listener with a new port is added, it triggers a rollout of Envoy because the container ports change. Here, I've stopped setting container ports on the Envoy workload entirely, since it's not required in order to expose the port anyway and mostly serves as documentation, so that Listener changes don't trigger Envoy rollouts. Definitely interested in folks' thoughts here.

@izturn @wilsonwu @Jean-Daniel I'd be interested in any input you have here. Let me know if you need more context!

internal/gatewayapi/listeners.go

codecov · 2023-03-22T20:10:56Z

Codecov Report

❗ No coverage uploaded for pull request base (main@d42b892). Click here to learn what that means.
The diff coverage is 96.55%.

❗ Current head f8b4bfb differs from pull request most recent head 1d7f72b. Consider uploading reports for the commit 1d7f72b to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5160   +/-   ##
=======================================
  Coverage        ?   78.25%           
=======================================
  Files           ?      138           
  Lines           ?    18585           
  Branches        ?        0           
=======================================
  Hits            ?    14544           
  Misses          ?     3762           
  Partials        ?      279

Impacted Files	Coverage Δ
internal/dag/dag.go	`98.68% <ø> (ø)`
internal/dag/httpproxy_processor.go	`91.63% <69.23%> (ø)`
cmd/contour/shutdownmanager.go	`42.22% <100.00%> (ø)`
internal/dag/accessors.go	`86.69% <100.00%> (ø)`
internal/dag/gatewayapi_processor.go	`94.34% <100.00%> (ø)`
internal/dag/ingress_processor.go	`97.35% <100.00%> (ø)`
internal/dag/listener_processor.go	`100.00% <100.00%> (ø)`
internal/featuretests/v3/envoy.go	`99.03% <100.00%> (ø)`
internal/gatewayapi/listeners.go	`100.00% <100.00%> (ø)`
internal/provisioner/controller/gateway.go	`60.37% <100.00%> (ø)`
... and 1 more

skriss · 2023-03-22T22:19:11Z

Still need to look at the upgrade path here; since the ports used internally are changing, the Envoy service needs to be updated as part of the upgrade and I'm guessing there will be some expected downtime. Also need to check whether upgrading the provisioner does the right thing upgrading existing Gateways, or not.

github-actions · 2023-04-07T00:18:10Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

skriss · 2023-04-07T18:59:09Z

This is now passing the upstream dynamic listener ports conformance PR with no modifications thanks to @sunjayBhatia's work to upgrade to v0.6.2.

github-actions · 2023-04-22T00:19:44Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

skriss · 2023-05-18T14:59:54Z

I've been doing some upgrade testing to test out the different listener port mapping schemes, but because of #5375, I think that actually there's currently some downtime for any provisioner-driven upgrade. I'll see if I can get at least a spike of a fix for #5375 implemented, which then should allow for a better comparison between the port mapping schemes.

sunjayBhatia

🤩

still digesting, some comments so far!

internal/dag/ingress_processor.go

internal/dag/status_test.go

internal/gatewayapi/listeners.go

Signed-off-by: Steve Kriss <[email protected]>

Rycieos · 2023-06-09T17:42:56Z

@skriss this is looking great! Sorry I was never able to give feedback earlier, but I can test now. I'm assuming you are still using the same image tag, so I'll try testing with your steveheptio/contour:many-listeners image.

skriss · 2023-06-09T18:10:06Z

@skriss this is looking great! Sorry I was never able to give feedback earlier, but I can test now. I'm assuming you are still using the same image tag, so I'll try testing with your steveheptio/contour:many-listeners image.

@Rycieos thanks! Let me push the latest image, I'm not sure which version that tag currently refers to. Will update here in a sec.

skriss · 2023-06-09T18:15:01Z

OK, latest versions have been pushed:

docker.io/steveheptio/contour:many-listeners
docker.io/steveheptio/contour:many-listeners-tcproute (this adds experimental TCPRoute support on top of the many listeners work, can't recall if you were interested in this too or not)

Rycieos · 2023-06-09T18:15:53Z

this adds experimental TCPRoute support on top of the many listeners work, can't recall if you were interested in this too or not

I was not; I need TLSRoutes, but not TCP.

internal/gatewayapi/listeners_test.go

internal/dag/builder_test.go

sunjayBhatia · 2023-06-09T19:16:12Z

internal/dag/builder_test.go

+								},
+							},
+							TLS: &gatewayapi_v1beta1.GatewayTLSConfig{
+								Mode: ref.To(gatewayapi_v1beta1.TLSModePassthrough),


i assume this is because we don't have a secret that makes sense in terminate mode since TLS is specified on the HTTPProxy?

now i remember discussing this a bit before but seeing it just now threw me for a second, could be a little counterintuitive for users, still on the fence

Ah, this test case can be switched to use the custom protocol, in which case the whole TLS block can be removed (this is pretty much the point of supporting the custom protocol, so users don't have to specify tls mode: passthrough which as you say is counterintuitive)

Oh, I guess I have a test case below that uses the custom protocol, so this is more just documentation that this is a valid alternate way of configuring things.

The basic problem is that if you specify a protocol of HTTPS, then the Gateway API webhook requires TLS mode to be specified, and if you set it to terminate, then it further requires a secret to be specified. So this is all just to work around the webhook logic.

sunjayBhatia

Just a couple small comments, but overall looks great! nice test coverage and looks like the gateway conformance is passing 👍🏽

Rycieos · 2023-06-09T19:20:47Z

Unfortunately, I am unable to test this; due to this limitation on TLSRoutes I was unaware of:

Listener.TLS.Mode must be "Passthrough" when protocol is "TLS".

I need to Terminate TLS at the Gateway. Is this an intentional limitation? The Gateway spec seems to suggest both must be supported, but it is not clear. I couldn't find any open issues on this project about this. I would be happy to open one.

But since this is a different message than I got last time, I guess you fixed that problem. Sorry I can't report better results.

skriss · 2023-06-09T20:33:17Z

OK, sounds like we have some more work to do to fully support your use case (though this PR is a big step in the right direction).

There's some conflicting information upstream about the exact combos of listener protocol and route type that should be supported:

So will need to track down exactly what should be supported. I think thus far we had implemented on the basis of https://gateway-api.sigs.k8s.io/guides/tls/, which indicates TLSRoute is for Passthrough mode.

From my perspective, it seems like TLS termination should be compatible with both TLSRoute (if you need to make a routing decision on the basis of SNI) and TCPRoute (if there's no routing decision to be made, all traffic arriving on the listener port will be proxied to the service(s) in the TCPRoute).

Since you say you need TLSRoute - does that imply you need to make a routing decision based on SNI? If not, then TLS Termination + TCPRoute should be sufficient.

If you could open a new issue to track this work, that'd be great. Thanks!

Rycieos · 2023-06-09T21:20:16Z

Since you say you need TLSRoute - does that imply you need to make a routing decision based on SNI? If not, then TLS Termination + TCPRoute should be sufficient.

That is correct. Routing by SNI is an extremely useful feature, and as far as I can tell, only Traefik supports that part of the Gateway API currently. Or at least is the only provider that supports TLSRoutes and their Termination.

If you could open a new issue to track this work, that'd be great.

I opened #5461

Signed-off-by: Steve Kriss <[email protected]>

skriss commented Mar 9, 2023

View reviewed changes

internal/gatewayapi/listeners.go Outdated Show resolved Hide resolved

skriss force-pushed the pr-multiple-gateway-listeners branch from 1974a5b to 853c44e Compare March 9, 2023 21:05

skriss force-pushed the pr-multiple-gateway-listeners branch 2 times, most recently from 91c7df5 to d6d35fa Compare March 22, 2023 20:03

skriss added the release-note/major A major change that needs more than a paragraph of explanation in the release notes. label Mar 22, 2023

skriss force-pushed the pr-multiple-gateway-listeners branch from 7863641 to 9d4f56c Compare March 22, 2023 22:58

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2023

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2023

skriss force-pushed the pr-multiple-gateway-listeners branch from 172f84c to 33ffdbe Compare April 7, 2023 16:14

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2023

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2023

This was referenced Apr 27, 2023

Gateway API: support TCPRoute #5321

Closed

Exposing TCP with FQDN for mongodb #2922

Closed

skriss force-pushed the pr-multiple-gateway-listeners branch from 33ffdbe to d68c47d Compare April 27, 2023 17:26

skriss force-pushed the pr-multiple-gateway-listeners branch from d68c47d to 8608387 Compare May 9, 2023 19:02

sunjayBhatia self-requested a review May 15, 2023 17:27

skriss force-pushed the pr-multiple-gateway-listeners branch from 8608387 to 8e9f81b Compare May 23, 2023 16:37

skriss marked this pull request as ready for review May 23, 2023 17:30

skriss requested a review from a team as a code owner May 23, 2023 17:30

skriss requested review from tsaarni and removed request for a team May 23, 2023 17:30

skriss changed the title ~~[WIP] support many Gateway Listeners~~ support many Gateway Listeners May 23, 2023

sunjayBhatia reviewed May 23, 2023

View reviewed changes

internal/dag/ingress_processor.go Outdated Show resolved Hide resolved

internal/dag/status_test.go Show resolved Hide resolved

internal/gatewayapi/listeners.go Outdated Show resolved Hide resolved

skriss added 13 commits June 6, 2023 08:26

switch to +8000 for port mapping

7b700ad

Signed-off-by: Steve Kriss <[email protected]>

support projectcontour.io/https protocol

1be64ce

Signed-off-by: Steve Kriss <[email protected]>

fix error msg for identifying listener

be9f0eb

Signed-off-by: Steve Kriss <[email protected]>

UT for custom HTTPS protocol

0549228

Signed-off-by: Steve Kriss <[email protected]>

listener conflicts: first in list order wins

2bf2743

Signed-off-by: Steve Kriss <[email protected]>

tweak changelog

8c185b1

Signed-off-by: Steve Kriss <[email protected]>

lower-case listener protocol

01e19c9

Signed-off-by: Steve Kriss <[email protected]>

set condition for conflicting ports

963e75a

Signed-off-by: Steve Kriss <[email protected]>

off by one

1efdd42

Signed-off-by: Steve Kriss <[email protected]>

fix dumb code

2317b81

Signed-off-by: Steve Kriss <[email protected]>

add UT

4eca1bb

Signed-off-by: Steve Kriss <[email protected]>

update changelog

270a9ad

Signed-off-by: Steve Kriss <[email protected]>

enable GatewayObserveredGenerationBump test

632a6a1

Signed-off-by: Steve Kriss <[email protected]>

skriss force-pushed the pr-multiple-gateway-listeners branch from 8aa1bf0 to 632a6a1 Compare June 6, 2023 14:27

interface{} -> any

f8b4bfb

Signed-off-by: Steve Kriss <[email protected]>

sunjayBhatia reviewed Jun 9, 2023

View reviewed changes

internal/gatewayapi/listeners_test.go Show resolved Hide resolved

sunjayBhatia reviewed Jun 9, 2023

View reviewed changes

internal/dag/builder_test.go Show resolved Hide resolved

sunjayBhatia reviewed Jun 9, 2023

View reviewed changes

sunjayBhatia approved these changes Jun 9, 2023

View reviewed changes

Rycieos mentioned this pull request Jun 9, 2023

Support Termination mode for TLS listener protocol (TLSRoutes, TCPRoutes) #5461

Closed

add UT

1d7f72b

Signed-off-by: Steve Kriss <[email protected]>

skriss merged commit 42d6f2b into projectcontour:main Jun 12, 2023
25 checks passed

skriss deleted the pr-multiple-gateway-listeners branch July 28, 2023 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support many Gateway Listeners #5160

support many Gateway Listeners #5160

skriss commented Mar 9, 2023 •

edited

Loading

skriss Mar 9, 2023 •

edited

Loading

skriss May 11, 2023

codecov bot commented Mar 22, 2023 •

edited

Loading

skriss commented Mar 22, 2023 •

edited

Loading

github-actions bot commented Apr 7, 2023

skriss commented Apr 7, 2023

github-actions bot commented Apr 22, 2023

skriss commented May 18, 2023

sunjayBhatia left a comment

Rycieos commented Jun 9, 2023

skriss commented Jun 9, 2023

skriss commented Jun 9, 2023

Rycieos commented Jun 9, 2023

sunjayBhatia Jun 9, 2023

skriss Jun 9, 2023

skriss Jun 9, 2023

skriss Jun 9, 2023

sunjayBhatia left a comment

Rycieos commented Jun 9, 2023

skriss commented Jun 9, 2023 •

edited

Loading

Rycieos commented Jun 9, 2023

support many Gateway Listeners #5160

support many Gateway Listeners #5160

Conversation

skriss commented Mar 9, 2023 • edited Loading

skriss Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

skriss May 11, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

skriss commented Mar 22, 2023 • edited Loading

github-actions bot commented Apr 7, 2023

skriss commented Apr 7, 2023

github-actions bot commented Apr 22, 2023

skriss commented May 18, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

Rycieos commented Jun 9, 2023

skriss commented Jun 9, 2023

skriss commented Jun 9, 2023

Rycieos commented Jun 9, 2023

sunjayBhatia Jun 9, 2023

Choose a reason for hiding this comment

skriss Jun 9, 2023

Choose a reason for hiding this comment

skriss Jun 9, 2023

Choose a reason for hiding this comment

skriss Jun 9, 2023

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

Rycieos commented Jun 9, 2023

skriss commented Jun 9, 2023 • edited Loading

Rycieos commented Jun 9, 2023

skriss commented Mar 9, 2023 •

edited

Loading

skriss Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

skriss commented Mar 22, 2023 •

edited

Loading

skriss commented Jun 9, 2023 •

edited

Loading