Downstream QUIC/HTTP3 Support #2111

tanujd11 · 2023-10-29T18:15:45Z

What type of PR is this?

Feature

What this PR does / why we need it:

Adds HTTP3 support in envoy gateway

Which issue(s) this PR fixes:

Fixes #422

codecov · 2023-10-29T18:27:19Z

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (d13c329) 64.54% compared to head (83cbc59) 64.58%.

Files	Patch %	Lines
internal/ir/zz_generated.deepcopy.go	0.00%	20 Missing and 1 partial ⚠️
...frastructure/kubernetes/proxy/resource_provider.go	9.09%	9 Missing and 1 partial ⚠️
internal/xds/translator/listener.go	86.36%	6 Missing and 3 partials ⚠️
internal/xds/translator/translator.go	70.00%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2111      +/-   ##
==========================================
+ Coverage   64.54%   64.58%   +0.04%     
==========================================
  Files         112      112              
  Lines       15949    16072     +123     
==========================================
+ Hits        10294    10380      +86     
- Misses       5007     5040      +33     
- Partials      648      652       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arkodg · 2023-10-29T22:34:08Z

thanks for building this out @tanujd11 ! , will start reviewing this post the v0.6 release

arkodg · 2023-11-08T14:44:05Z

hey @LanceEa can you help review this one ( since you have experience building this out before :) )

arkodg · 2023-11-14T01:08:34Z

api/v1alpha1/clienttrafficpolicy_types.go

+	// EnableHTTP3 enables HTTP/3 support on the listener.
+	// Disabled by default.
+	// +optional
+	EnableHTTP3 bool `json:"enableHTTP3,omitempty"`


lets make this *bool since its optional

I would recommend making it an object instead of a bool. This will future proof you more to provide addtional overrides.

For example, in the generated config you have AlpnProtocols: []string{"h3"} hard coded. Not all clients will recognize or support that. If you look at a simple google home page you can see their web server is configured to support h3=":443"; ma=2592000,h3-29=":443"; ma=2592000.

Other examples, are setting different ports to listen for the QUIC traffic on because many cloud providers do not support listening on the same incoming address. TBH, this is the tricky part to setting up H3 and the Envoy config is quite straight forward.

Note: you could start with an empty object and check for existence or not if you want to hold off on allow customizations right away but at least an object future proofs you to add going forward without breaking changes.

I would love to just have a enable knob, but agree with @LanceEa, an empty struct maybe a good call for this API because some user may want to modify the listening port, and everyone loves tweaking port values to adhere with corp perimeter policies

LanceEa

I will defer to the maintainers on code coverage and general style but for the most part it looks good. I have left some feedback on the design and recommendations for future proofing it because support for mixedprotocols on the LoadBalancer Service and cloud vendors was limited last I looked.

LanceEa · 2023-11-14T13:40:06Z

api/v1alpha1/clienttrafficpolicy_types.go

+	// EnableHTTP3 enables HTTP/3 support on the listener.
+	// Disabled by default.
+	// +optional
+	EnableHTTP3 bool `json:"enableHTTP3,omitempty"`


I would recommend making it an object instead of a bool. This will future proof you more to provide addtional overrides.

For example, in the generated config you have AlpnProtocols: []string{"h3"} hard coded. Not all clients will recognize or support that. If you look at a simple google home page you can see their web server is configured to support h3=":443"; ma=2592000,h3-29=":443"; ma=2592000.

Other examples, are setting different ports to listen for the QUIC traffic on because many cloud providers do not support listening on the same incoming address. TBH, this is the tricky part to setting up H3 and the Envoy config is quite straight forward.

LanceEa · 2023-11-14T13:41:05Z

api/v1alpha1/clienttrafficpolicy_types.go

+	// EnableHTTP3 enables HTTP/3 support on the listener.
+	// Disabled by default.
+	// +optional
+	EnableHTTP3 bool `json:"enableHTTP3,omitempty"`


Note: you could start with an empty object and check for existence or not if you want to hold off on allow customizations right away but at least an object future proofs you to add going forward without breaking changes.

LanceEa · 2023-11-14T13:45:17Z

internal/gatewayapi/listener.go

+
+				proxyListener := &ir.ProxyListener{
+					Name:  irHTTPListenerName(listener),
+					Ports: []ir.ListenerPort{infraPort},


If I'm understanding this correctly we are only supporting a single port for this listener and then using the HTTP/3 flag to instruct the single ir.ProxyListener to generate a secondary listener for UDP/QUIC. This means the second listener would inherit the port correct?

If so you are more than likely going to want to allow that to be configurable because most cloud vendors were not supporting this last I knew (6+ months ago was the last time I looked so could have changed)

This comment is referring to mixed protocols on a single Service of type Loadbalancer.

LanceEa · 2023-11-14T13:46:46Z

internal/xds/translator/translator.go

+		Append: &wrapperspb.BoolValue{Value: true},
+		Header: &corev3.HeaderValue{
+			Key:   "alt-svc",
+			Value: strings.Join([]string{fmt.Sprintf(`%s=":%d"; ma=86400`, "h3", port)}, ", "),


You are going to want this to be configurable in the future, see my previous comment on the design of the ClientTrafficPolicy.

LanceEa · 2023-11-14T14:12:50Z

internal/xds/translator/testdata/out/xds-ir/http3.routes.yaml

+      - append: true
+        header:
+          key: alt-svc
+          value: h3=":10443"; ma=86400


This port needs to be the port exposed externally outside the cluster to the client. So, in most cases involving a browser it is more than likely going to be :443 and not the listener port. The LoadBalancer of type Service then needs to point traffic to 10443. That is what I was referring to as the tricky part because a single Service doesn't always allow for MixedProtocol. note: that might have change since last time I looked into this but it is the more nuanced part to the setup.

tanujd11 · 2023-11-21T06:22:22Z

Hi @LanceEa @arkodg I have made changes to the API making it as following

http3Settings:
  enabled: true

At the moment only enabled is there. I can raise a follow up PR to expose separate port for QUIC and other alt-svc configs. Till then this feature can be termed as experimental. WDYT?

tanujd11 · 2023-11-21T06:31:47Z

Also it would be great if someone from @envoyproxy/gateway-reviewers / @envoyproxy/gateway-maintainers can try out the feature

api/v1alpha1/clienttrafficpolicy_types.go

tanujd11 · 2023-11-22T04:57:51Z

/retest

tanujd11 · 2023-11-22T05:17:09Z

/retest

api/v1alpha1/clienttrafficpolicy_types.go

arkodg · 2023-11-22T20:42:08Z

internal/gatewayapi/clienttrafficpolicy.go

+		if httpIR.TLS != nil && policySpec.HTTP3 != nil {
+			httpIR.HTTP3 = &ir.HTTP3Settings{}
+			var proxyListenerIR *ir.ProxyListener
+			for _, proxyListener := range infraIR[irKey].Proxy.Listeners {


nit: can we reuse irListenerName and also check if irListenerName.Proxy != nil

Hi, I did not get this point. It seems irListenerName is a string. I need to get infraIR from the corresponding IrListenerName that's why I iterated listeners for a gateway.

arkodg · 2023-11-22T20:46:30Z

internal/ir/infra_test.go

@@ -20,7 +20,7 @@ func TestValidateInfra(t *testing.T) {
 		{
 			name:   "default",
 			infra:  NewInfra(),
-			expect: false,
+			expect: true,


why is this true ?

As we removed NewProxyListener by default and only add it when needed(Since we replaced only 1 listener supported to multiple listener), there is no error generated in this test case, hence false. Before any empty listener was there which failed the validate function.

arkodg · 2023-11-22T20:58:14Z

internal/infrastructure/kubernetes/proxy/resource_provider.go

@@ -80,6 +80,18 @@ func (r *ResourceRender) Service() (*corev1.Service, error) {
 				TargetPort: target,
 			}
 			ports = append(ports, p)
+
+			if port.Protocol == ir.HTTPSProtocolType {


should this live here on in the Gateway API layer ?

This is for opening UDP ports for only those ports which have HTTPS configured, That's a requirement for quic. So this condition is added here.

arkodg · 2023-11-22T21:00:08Z

internal/xds/translator/listener.go

@@ -137,6 +163,10 @@ func (t *Translator) addXdsHTTPFilterChain(xdsListener *listenerv3.Listener, irL
 		}
 	}

+	if http3Listener {
+		mgr.CodecType = hcmv3.HttpConnectionManager_HTTP3


will auto work ?
is there a way for us to support http3 but also support something else in case the client doesnt support it ?

Hi Arko, If HTTP3 is not supported then the other listener will be used. We have created a mirror UDP listener and the connection would be upgraded to HTTP3 according to the alt-svc header. So even if HTTP3 is not supported HTTP/HTTP2 will be used

internal/gatewayapi/testdata/clienttrafficpolicy-status-conditions.out.yaml

tanujd11 · 2023-11-29T10:54:21Z

/retest

tanujd11 · 2023-11-29T11:31:07Z

/retest

tanujd11 · 2023-12-19T13:11:19Z

Hi @arkodg , welcome back from the vacation. Your reviews on this will be greatly appreciated. Thanks.

github-actions · 2023-12-19T13:14:22Z

🚀 Deployed on https://gateway-pr-2111-preview--eg-preview.netlify.app

arkodg

LGTM, thanks for adding this feature !

Signed-off-by: tanujd11 <[email protected]>

Xunzhuo · 2023-12-20T05:22:51Z

/retest

Xunzhuo

Great! Thanks for working on it : )

shawnh2

lgtm

Xunzhuo · 2023-12-20T14:42:19Z

/cc @arkodg

tanujd11 requested a review from a team as a code owner October 29, 2023 18:15

tanujd11 force-pushed the feat/http3-support branch from 81a92c0 to 8a6b240 Compare October 29, 2023 18:21

tanujd11 self-assigned this Oct 29, 2023

zirain added this to the Backlog milestone Oct 30, 2023

arkodg requested a review from LanceEa November 8, 2023 14:44

arkodg reviewed Nov 14, 2023

View reviewed changes

LanceEa reviewed Nov 14, 2023

View reviewed changes

tanujd11 force-pushed the feat/http3-support branch from 8a6b240 to 62082e2 Compare November 21, 2023 06:20

tanujd11 requested review from arkodg and LanceEa November 21, 2023 06:22

tanujd11 force-pushed the feat/http3-support branch from 62082e2 to 4b1c286 Compare November 21, 2023 06:31

arkodg reviewed Nov 22, 2023

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

arkodg reviewed Nov 22, 2023

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

arkodg reviewed Nov 22, 2023

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

arkodg reviewed Nov 22, 2023

View reviewed changes

arkodg reviewed Nov 23, 2023

View reviewed changes

internal/gatewayapi/testdata/clienttrafficpolicy-status-conditions.out.yaml Outdated Show resolved Hide resolved

tanujd11 requested a review from arkodg November 29, 2023 09:36

Xunzhuo added the road-to-ga label Dec 6, 2023

Xunzhuo modified the milestones: Backlog, v1.0.0-rc1 Dec 6, 2023

github-actions bot deployed to pull request December 19, 2023 13:14 View deployment

arkodg previously approved these changes Dec 19, 2023

View reviewed changes

arkodg requested review from a team December 19, 2023 22:22

tanujd11 added 7 commits December 20, 2023 10:19

Add http3 support to EG

39adf44

Signed-off-by: tanujd11 <[email protected]>

addressed review comments

9f3b85e

Signed-off-by: tanujd11 <[email protected]>

address review comments: api changes

4ad3e64

Signed-off-by: tanujd11 <[email protected]>

address review comments: api changes

31b527b

Signed-off-by: tanujd11 <[email protected]>

addressed review comments

7fab82f

Signed-off-by: tanujd11 <[email protected]>

rebase to main and make gen-check

a19c115

Signed-off-by: tanujd11 <[email protected]>

make gen-check

83cbc59

Signed-off-by: tanujd11 <[email protected]>

tanujd11 dismissed arkodg’s stale review via 83cbc59 December 20, 2023 04:53

tanujd11 force-pushed the feat/http3-support branch from 50250c3 to 83cbc59 Compare December 20, 2023 04:53

Xunzhuo approved these changes Dec 20, 2023

View reviewed changes

Xunzhuo requested review from zhaohuabing, zirain and shawnh2 December 20, 2023 06:48

shawnh2 approved these changes Dec 20, 2023

View reviewed changes

arkodg merged commit da40ccb into envoyproxy:main Dec 20, 2023
18 checks passed

arkodg mentioned this pull request Jan 9, 2024

E2E for HTTP3 #2423

Open

arkodg mentioned this pull request Mar 11, 2024

Enabling HTTP3 removes h2 from TLS ALPN #2875

Closed

Downstream QUIC/HTTP3 Support #2111

Downstream QUIC/HTTP3 Support #2111

Conversation

tanujd11 commented Oct 29, 2023

codecov bot commented Oct 29, 2023 • edited Loading

Codecov Report

arkodg commented Oct 29, 2023

arkodg commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

LanceEa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujd11 commented Nov 21, 2023

tanujd11 commented Nov 21, 2023

tanujd11 commented Nov 22, 2023

tanujd11 commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujd11 commented Nov 29, 2023

tanujd11 commented Nov 29, 2023

tanujd11 commented Dec 19, 2023

github-actions bot commented Dec 19, 2023

arkodg left a comment

Choose a reason for hiding this comment

Xunzhuo commented Dec 20, 2023

Xunzhuo left a comment

Choose a reason for hiding this comment

shawnh2 left a comment

Choose a reason for hiding this comment

Xunzhuo commented Dec 20, 2023

codecov bot commented Oct 29, 2023 •

edited

Loading

arkodg Nov 16, 2023 •

edited

Loading