[v3.0] add downstream support for HTTP/3 #4246

LanceEa · 2022-05-25T00:03:13Z

Description

This PR adds downstream support for HTTP/3 and is able to take advantage of the current developer experience by using the protocolStack field of the Listener CRD.

HTTP/2 and HTTP/1 traffic will continue to be served using a TCP Listener configured for HTTP and now a second Listener configured for HTTP/3 can be done by setting the protocalStack: [ "TLS", "HTTP", "UDP"] on a Listener and binding to the same address:port as the TCP Listener

# Listener that leverages TCP and will be used to server HTTP/2 and HTTP/1.1 traffic
apiVersion: getambassador.io/v3alpha1
kind: Listener
metadata:
  name: emissary-ingress-listener-8443
  namespace: emissary
spec:
  port: 8443
  protocol: HTTPS
  securityModel: XFP
  hostBinding:
    namespace:
      from: ALL
---
# Listener that leverages HTTP & UDP, will be used to serve HTTP/3 traffic
# NOTE: this does not add support for raw UDP traffic
apiVersion: getambassador.io/v3alpha1
kind: Listener
metadata:
  name: emissary-ingress-listener-udp-8443
  namespace: emissary
spec:
  port: 8443
  # order matters here with UDP required to be the last item
  protocolStack:
    - TLS
    - HTTP
    - UDP
  securityModel: XFP
  hostBinding:
    namespace:
      from: ALL

When a TCP Listener binds to the same address and port as the UDP Listener then it will inject the alt-svc header into the responses returned on the TCP Listener. This header advertises HTTP/3 support to the client and tells the client how to upgrade itself to http/3.

NOTE - the alt-svc header is not configurable for the initial release but in the future we should make it available to the user

More info here on the header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Alt-Svc.

Related Issues

None

Testing

additional unit tests were added to capture http/3 behavior
Existing Kat tests were updated to handle and changes introduced

Deployed to multiple clusters and tested setting up in AKS, EKS and GKE.

Checklist

I made sure to update CHANGELOG.md.
This is unlikely to impact how Ambassador performs at scale.
It is a new feature and most of the heavy lifting is done in Envoy. Also, fallback to http/2 and http/1.1 is still possible. One potential thing we will need to keep an eye is that the routes are duplicated on both the TCP and UDP listeners. The UDP listener was able to exclude the http (non-tls) redirect routes due to QUIC requiring TLS so its not an exact duplication.
My change is adequately tested.
I updated DEVELOPING.md
The changes in this PR have been reviewed for security concerns and adherence to security best practices.

LanceEa · 2022-05-31T02:24:19Z

@AliceProxy @LukeShu - This PR is based off from #4241 so you can ignore the first commits because they are being reviewed as part of that PR.

You can start your review at commit: 2c1bcd3

I will rebase on the upgrade once merged, update the changelog and squash and commits that are unnecessary once you have had a chance to review and we feel its in a good place.

LukeShu · 2022-06-01T17:33:56Z

This PR is based off from #4241 so you can ignore the first commits because they are being reviewed as part of that PR.

Tip: If you target this PR at laustin/envoy-1.22 instead of master, then this PR will be clearer, and it will automatically re-target at master when #4241 lands.

LanceEa · 2022-06-14T18:33:18Z

This PR is based off from #4241 so you can ignore the first commits because they are being reviewed as part of that PR.

Tip: If you target this PR at laustin/envoy-1.22 instead of master, then this PR will be clearer, and it will automatically re-target at master when #4241 lands.

Whoops, good catch. I was rebasing it off from laustin/envoy-1.22 and thought the PR was targeting that branch as well but apparently not.

python/tests/unit/test_listener.py

pkg/api/getambassador.io/v3alpha1/crd_listener.go

python/ambassador/envoy/v3/v3listener.py

python/ambassador/ir/irlistener.py

python/ambassador/envoy/v3/v3listener.py

LukeShu · 2022-06-22T16:57:54Z

HTTP/2 and HTTP/1 traffic will continue to be served using a TCP Listener configured for HTTP and now a second Listener configured for HTTP/3 can be done by setting the protocalStack: [ "TLS", "HTTP", "UDP"] on a Listener and binding to the same address:port as the TCP Listener

That protocol stack doesn't make sense to me. Shouldn't that be protocolStack: ["HTTP", "TLS", "UDP"] (or better: protocolStack: ["HTTP", "QUIC", "UDP"])?

Edit: Nope, protocolStack makes even less damn sense than I thought. Which was already pretty low.

LukeShu · 2022-06-22T17:07:25Z

I'm not sure how I feel about magically associating 2 listeners together if they have matching addr:port. In particular, the rules about how Hosts get attached to Listeners (and then Mappings to Hosts) is complex and I'm worried about locking ourselves in to funny behavior if the 2 Listeners disagree about which Hosts should be bound to them.

LukeShu · 2022-06-22T17:08:33Z

PS: e2e tests would be good too, since this is a very integration-y problem space.

LukeShu · 2022-06-22T17:12:01Z

On the other hand, I do think that I like having a 2nd listener better than packing 2 ports in to 1 listener.

LukeShu · 2022-06-22T17:15:17Z

You know what, I'm prepared to call any weird listener-host binding behavior at this point a bug. So fine for v0, and we can break it later because it was always a bug.

LukeShu · 2022-06-22T19:55:36Z

python/ambassador/ir/ir.py

@@ -271,8 +271,6 @@ def __init__(self, aconf: Config,
        self.groups = {}
        self.grpc_services = {}
        self.hosts = {}
-        # self.invalidate_groups_for is handled above.
-        # self.k8s_status_updates is handled below.


LukeShu · 2022-06-22T19:56:00Z

python/ambassador/ir/ir.py

-            self.post_error("Duplicate listener %s on %s:%d; keeping definition from %s" %
-                            (listener.name, listener.bind_address, listener.port, extant_listener.location))
+            err_msg = f"Duplicate listener {listener.name} on {listener.socket_protocol.lower()}://{listener.bind_address}:{listener.port};" \
+                      f"; keeping definition from {extant_listener.location}"


This duplicates the ; in the error message, doesn't it?

LukeShu · 2022-06-22T19:57:44Z

python/ambassador/envoy/v3/v3listener.py

+            "udp_listener_config": {},
            "filter_chains": self._filter_chains,
            "traffic_direction": self.traffic_direction
        }

+        if self.isProtocolUDP():
+            listener['udp_listener_config'] = {
+                'quic_options': {},
+                'downstream_socket_config': { 'prefer_gro': True }
+            }
+        else:
+            del(listener['udp_listener_config'])
+


I'm not sure what's to like with this pattern of "initialize with a bogus value, then either overwrite with a valid value or remove it". Why not drop the initial value and drop the else case?

agree that is cleaner

LukeShu · 2022-06-22T20:17:20Z

Oh! This needs a releaseNotes entry.

The initial implemenation of HTTP/3 support provides downstream connectivity between client (browser, curl, langs) and Emissary-Ingress. - adds default `alt-svc` to TCP connection if bound address and port are shared between TCP and UDP. - ensures UDP Listener is only created when a TLS Context or fallback context is provided Two things that are not supported with this change: - raw UDP support is NOT added and will be an error - upstream (emissary-ingress and service) HTTP/3 support is NOT added Adds unit test coverage for the following scenarios: - valid http3 listener - TCP/UDP listener bound to same address:port and auto-inject http/3 bits into TCP Listener - drop udp listener missing required TLS Context for http/3 Signed-off-by: Lance Austin <[email protected]>

LanceEa force-pushed the laustin/envoy-1.22-http3 branch from 266c19f to 2c1bcd3 Compare May 28, 2022 12:26

alexgervais mentioned this pull request May 30, 2022

donwstream HTTP3/QUIC support #4243

Closed

LanceEa marked this pull request as ready for review May 31, 2022 02:24

LanceEa requested review from LukeShu and Alice-Lilith May 31, 2022 02:24

LukeShu force-pushed the master branch from 3fb6b2f to c32a044 Compare June 6, 2022 19:01

LanceEa force-pushed the laustin/envoy-1.22-http3 branch 5 times, most recently from 1acd6a3 to fbe7848 Compare June 14, 2022 03:52

LanceEa force-pushed the laustin/envoy-1.22-http3 branch from 6dd6f59 to 03a6936 Compare June 16, 2022 04:52

LanceEa changed the base branch from master to laustin/envoy-1.22 June 16, 2022 04:52

Alice-Lilith approved these changes Jun 16, 2022

View reviewed changes

python/tests/unit/test_listener.py Outdated Show resolved Hide resolved

LanceEa force-pushed the laustin/envoy-1.22-http3 branch from 03a6936 to d957785 Compare June 16, 2022 18:01