[alpha] Create mirror QUIC listeners for HTTPS servers on gateways #33817
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/L
|
Experiment apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
metadata:
name: istio-with-quic
namespace: istio-system
spec:
meshConfig:
accessLogFile: /dev/stdout
hub: localhost:5000
tag: istio-quic
components:
ingressGateways:
- name: bookinfo-ingress
namespace: bookinfo
enabled: true
label:
gateway: bookinfo
quic: enabled
k8s:
service:
type: NodePort
ports:
- name: https-bookinfo
port: 443
targetPort: 8443
protocol: TCP
# Same service and target port with
# the only difference being the protocol
- name: http-quic-bookinfo
port: 8443
targetPort: 8443
protocol: UDP
values:
global:
imagePullPolicy: Always
logging:
level: "default:debug"
pilot:
env:
PILOT_ENABLE_QUIC_LISTENERS: "true"Then deployed the bookinfo application and setup routing (notice that there is no QUIC related stuffs in configs. QUIC mirror listener happens behind the scenes apiVersion: networking.istio.io/v1beta1
kind: Gateway
metadata:
name: bookinfo-gw
namespace: bookinfo
spec:
selector:
gateway: bookinfo
quic: enabled
servers:
- port:
number: 443
protocol: HTTPS
name: https-bookinfo
hosts:
- bookinfo.com
tls:
mode: SIMPLE
credentialName: bookinfo-secret
---
apiVersion: networking.istio.io/v1beta1
kind: VirtualService
metadata:
name: bookinfo-vs
namespace: bookinfo
spec:
hosts:
- bookinfo.com
gateways:
- bookinfo-gw
http:
- name: productpage-route
route:
- destination:
host: productpage.bookinfo.svc.cluster.local
port: { number: 9080 }Finally test it with QUIC enabled cURL (it is not enabled by default. Instructions here - https://github.com/curl/curl/blob/master/docs/HTTP3.md). In my computer it is called And Eureka! Access logs from the gateway Then I wanted to make sure that it is indeed sending UDP packets (it should). So tested with Corresponding filter chain config in Envoy (it is long. So I have omitted many details. With the experiment you can inspect with - address:
socketAddress:
address: 0.0.0.0
portValue: 8443
protocol: UDP
filterChains:
- filterChainMatch:
serverNames:
- bookinfo.com
filters:
- name: envoy.filters.network.http_connection_manager
typedConfig:
'@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
codecType: HTTP3
http3ProtocolOptions: {}
httpFilters:
- name: istio.metadata_exchange
# Rest of HTTP filters go here
- name: envoy.filters.http.router
typedConfig:
'@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
transportSocket:
name: envoy.transport_sockets.quic
typedConfig:
'@type': type.googleapis.com/envoy.extensions.transport_sockets.quic.v3.QuicDownstreamTransport
downstreamTlsContext:
commonTlsContext:
alpnProtocols:
- h2
- http/1.1
tlsCertificateSdsSecretConfigs:
- name: kubernetes://bookinfo-secret
sdsConfig:
ads: {}
resourceApiVersion: V3
requireClientCertificate: false
name: quic_0.0.0.0_8443
reusePort: true
trafficDirection: OUTBOUND
udpListenerConfig:
quicOptions: {}
|
|
Nice! I haven't had a chance to review this yet but wanted to point you to https://github.com/howardjohn/istio/tree/pilot/http3-gateway where I have done similar experiments |
|
Thanks @howardjohn - Your implementation is much simpler and understandable :) Do you mind if I borrow from your implementation and continue from there? With some modification, it would be possible to support HTTP3 only ports, although the dominant use case is running both TCP+TLS and QUIC on the same port. Plus, I want to fix some stuffs like proper ALPN (h3 instead of h2 although from my yesterday's experiment it looked fine). However, some changes though (not major ones) - I wouldn't call them From RFC 9000
There seem to be some problems at Kubernetes layer though. Looks like it does not allow both TCP and UDP for the same port when service type is LoadBalancer (but NodePort works though). I just tried an experiment with the following spec and it fails
Service YAML apiVersion: v1
kind: Service
metadata:
name: nginx-https
namespace: test
spec:
selector:
app: nginx
type: LoadBalancer
ports:
- name: https-tcp
port: 443
targetPort: 8443
protocol: TCP
- name: https-quic
port: 443
targetPort: 8443
protocol: UDPBut it accepts same port, different protocol in Deployment spec though. So I have raised kubernetes/kubernetes#103464 |
|
UPDATE: Turns out that Kubernetes has an alpha feature gate called |
istio-testing
added
size/XL
|
@howardjohn - I re-started with your code, fixed some issues and refactored the code. I also added some basic unit tests. I observed that Envoy is buggy for QUIC and not all settings worked. In fact, they mention that it is still alpha. So this definitely needs integration tests. Unfortunately, this is blocked by #33930 . For my experiments, I built Notice that there are no NodePort hacks this time. It is the service of type For QUIC on port 443 For HTTP-over-TCP on port 443 |
|
Oh! I just noticed they have an issue already - envoyproxy/envoy#15953 |
|
UPDATE: When HTTPS over TCP is used, it sends Trying the same with H3/QUIC |
istio-testing
added
the
needs-rebase
istio-testing
removed
the
needs-rebase
su225
changed the title
istio-testing
removed
the
do-not-merge/work-in-progress
pilot/pkg/model/gateway.go
Outdated
| if features.EnableQUICListeners && gateway.IsEligibleForHTTP3Upgrade(s) && | ||
| udpSupportedPort(s.GetPort().GetNumber(), gwAndInstance.instances) { | ||
| log.Debugf("Server at port %d eligible for HTTP3 upgrade. Add QUIC listener", serverPort.Number) | ||
| h3MirrorRouteName := "h3-mirror." + routeName |
There was a problem hiding this comment.
why can we not use the same route? (I might answer myself after reading more)
There was a problem hiding this comment.
Oh for alt-svc... 2x routes is fairly expensive. We may consider adding support directly to envoy (in HCM) to add this - I think there was high level support for that
There was a problem hiding this comment.
If we would like to reduce the number of routes, how about alt-svc in both cases? I get it - it is not needed when the client talks H3 and it is redundant. According to the RFC-7838 that is something the client can ignore.
From https://datatracker.ietf.org/doc/html/rfc7838#section-2.4
By their nature, alternative services are OPTIONAL: clients do not need to use them.
Therefore, if a client supporting this specification becomes aware of an alternative service, the client SHOULD use that alternative service for all requests to the associated origin as soon as it is available
There was a problem hiding this comment.
Just noticed that cloudflare sends alt-svc even for H3. So I assume this shouldn't be a problem in general! (EDIT: So does google!)
| server.GetPort().GetNumber(), server.GetPort().GetName(), server.GetPort().GetProtocol()) | ||
| routeName := mergedGateway.TLSServerInfo[server].RouteName | ||
| if mergedGateway.TLSToQUICServerRouteMap[routeName] != "" { | ||
| routeName = mergedGateway.TLSToQUICServerRouteMap[routeName] |
There was a problem hiding this comment.
is mergedGateway.TLSToQUICServerRouteMap[routeName] ever "" here?
There was a problem hiding this comment.
I originally wrote something and this remained. I changed the name and type to set. It is the set of routes on which H3 has to be advertised (by setting alt-svc header)
| // Not doing so is a security hole as would allow bypassing auth. | ||
| ListenerProtocol: istionetworking.ListenerProtocolHTTP, | ||
| TransportProtocol: istionetworking.TransportProtocolQUIC, | ||
| IstioMutualGateway: false, // Currently, we don't support this. Revisit later |
There was a problem hiding this comment.
Simple answer - I didn't know much about that. I vaguely knew it is about mTLS. TBH searching for its usages, I still don't understand why it is there. So I set it to false and moved on so that I can focus on getting simple TLS to work.
| case istionetworking.TransportProtocolTCP: | ||
| return bind + "_" + strconv.Itoa(port) | ||
| case istionetworking.TransportProtocolQUIC: | ||
| return "quic_" + bind + "_" + strconv.Itoa(port) |
There was a problem hiding this comment.
we may consider here, and many other places replacing quic with udp. The rationale is that in the future we may add non-quic UDP and it would be on the same listener?
Unless quic + non-quic UDP on the same listener will never be supported by envoy
There was a problem hiding this comment.
If I understand it correctly RFC-9000 gives a hint on QUIC co-existing with other UDP based protocols on the same port.
Fixed Bit: The next bit (0x40) of byte 0 is set to 1. Packets
containing a zero value for this bit are not valid packets in this
version and MUST be discarded. A value of 1 for this bit allows
QUIC to coexist with other protocols; see [RFC7983].
So I agree with you that although QUIC is connection-oriented protocol, it is still based on UDP now and does not have its own format like TCP and UDP. So I changed it to udp_ as you suggested. I know that these names show up on istioctl proxy-config listeners. Question - Is this something that needs to be backwards-compatible?
There was a problem hiding this comment.
Question - Is this something that needs to be backwards-compatible?
No, but if you change it it would probably make upgrades not work because Envoy would complain it has two listeners on the same port. But aside from that users are not supposed to depend on XDS details
There was a problem hiding this comment.
Ok. I thought we send the entire state of the world on each LDS push - meaning, if the new update received does not have the name then envoy would clean up the listener. Hmm...That does not seem to be the case here? (I don't know the details of Envoy XDS implementation). Anyways, I changed it to udp_ as suggested.
There was a problem hiding this comment.
We do but envoy doesn't actually atomically apply them like that IIUC. So the new ones still can conflict with old ones
There was a problem hiding this comment.
Oh! Ok. Learned something new :) I didn't know "does not atomically apply them" part. I guess even if it applies atomically, Envoy has to put old listeners to draining mode for deletion (socket still not closed at that time). For modification, simple way is delete+add - in this case we will run into port-conflict.
But still - while upgrading don't we restart pods for newer version of sidecar to be injected and it connects to newer revision of istiod. Due to restart, envoy should have forgotten configs and starts with clean slate. Right? I see that the problem occurs when someone upgrades it in-place (which I think is not recommended) and sidecar reconnects. Another possibility is somebody changing Istiod tag (due to say, hot fix) resulting in newer Istiod sending something (again - hot-fix should be avoided).
Anyways, I have changed it to udp_ as you suggested already. Still not fully convinced with "upgrades does not work" part. I have outlined scenarios that I could think of and yes - I agree that it would be a problem in case of in-case upgrades as for the sidecar it would be "same istiod address and reconnect" resulting in mixing up of envoy config state. But for canary upgrade scenario it should not cause issues as the pod would be restarted to point to the new revision.
|
/test integ-security-multicluster-tests_istio |
istio-testing
added
the
needs-rebase
…nal traffic Signed-off-by: su225 <suchithjn22@gmail.com>
istio-testing
removed
the
needs-rebase
| istionetworking.TransportProtocolQUIC: mergedGateway.MergedQUICTransportServers, | ||
| } | ||
|
|
||
| for transport, gwServers := range transportToServers { |
There was a problem hiding this comment.
nit: it seems TransportProtocolTCP and TransportProtocolQUIC have no reusable code below, why not handle it separately
There was a problem hiding this comment.
This will add difficulty to review, could not distinguish whether it is changed
There was a problem hiding this comment.
Much of the code - like building listener options, calling plugins to create Auth filters, code to generate listeners and filter chains are common to both. The difference is in building HTTP related filter chains. I have moved TCP and QUIC sections of the switch case to its own functions for clarity.
| @@ -536,9 +540,34 @@ func translateRoute(node *model.Proxy, in *networking.HTTPRoute, | |||
| out.TypedPerFilterConfig[wellknown.Fault] = util.MessageToAny(translateFault(in.Fault)) | |||
| } | |||
|
|
|||
| if isH3DiscoveryNeeded { | |||
| h3DiscoveryHeader := buildH3AltSvcHeader(port, util.ALPNHttp3OverQUIC) | |||
There was a problem hiding this comment.
Does it need add alt-svc? Doesn't envoy do it automatically?
There was a problem hiding this comment.
I'm not sure if Envoy does that. I have to check.
There was a problem hiding this comment.
I am not sure, just a question here
There was a problem hiding this comment.
suggest adding note to envoyproxy/envoy#15953
There was a problem hiding this comment.
I added a note about our usage.
What RFC is it not complying?
IMO its not needed. Just like you don't need to declare a port "HTTP2" for it to accept HTTP2 traffic at Gateway. If http3 is possible on the port (ie they have tls settings and UDP defined in service) we can transparently add it |
|
Is it safe when the client does not support http3? Is it a must the gateway will require http3 for the second request? |
|
@howardjohn I mean the proposal, it added |
Ah, got it. Thought you meant IETF rfc. I agree it doesn't match that design but IMO this is better. Declaring "HTTP3" doesn't really make much sense IMO, since you almost never want just h3 - you want h1+h2+h3 all together |
|
make sense, it is safe AFAIK |
@hzxuzhonghu - Yes. It is safe. This creates a QUIC mirror listener for every HTTPS port on the gateway provided there is a UDP port opened on the gateway with the same number. (Like 443/TCP and 443/UDP). Existing listeners are not removed. With In integration tests, both TCP and QUIC are tested as part of the same test to make sure both of them work correctly. |
Signed-off-by: su225 <suchithjn22@gmail.com>
|
lgtm, @howardjohn @ramaraochavali to put more eyes on it. |
| // We have TLS settings defined and we have already taken care of unique route names | ||
| // if it is HTTPS. So we can construct a QUIC server on the same port. It is okay as | ||
| // QUIC listens on UDP port, not TCP | ||
| if features.EnableQUICListeners && gateway.IsEligibleForHTTP3Upgrade(s) && |
There was a problem hiding this comment.
nit: IsEligibleForHTTP3Upgrade already checks EnableQUICListeners, no need to replace
There was a problem hiding this comment.
Yeah. I see it is redundant. Guess it is fine for readability?
pilot/pkg/model/gateway.go
Outdated
| if mergedQUICServers[serverPort] == nil { | ||
| mergedQUICServers[serverPort] = &MergedServers{ | ||
| Servers: []*networking.Server{s}, | ||
| RouteName: routeName, |
There was a problem hiding this comment.
what if routeName changes between serverPort? Is it possible?
There was a problem hiding this comment.
Something I need to go through once again.
There was a problem hiding this comment.
It is possible because different HTTPS servers would have different route names. I went through the code again and found that RouteName is unnecessary for HTTPS and HTTP/3 cases. This is because we take it from TLSServerInfo. The field is used only for plaintext HTTP. Hence I removed this in both places.
| @@ -536,9 +540,34 @@ func translateRoute(node *model.Proxy, in *networking.HTTPRoute, | |||
| out.TypedPerFilterConfig[wellknown.Fault] = util.MessageToAny(translateFault(in.Fault)) | |||
| } | |||
|
|
|||
| if isH3DiscoveryNeeded { | |||
| h3DiscoveryHeader := buildH3AltSvcHeader(port, util.ALPNHttp3OverQUIC) | |||
There was a problem hiding this comment.
suggest adding note to envoyproxy/envoy#15953
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package quic |
There was a problem hiding this comment.
IMO we don't need this one. k8sca is a variant on sds_ingress. We don't need all combinatoric explosion of possible options - k8sca and quic have no relation and are extremely unlikely to conflict
There was a problem hiding this comment.
Removed this. Looking at it, I guess only the PKI stuff changes. Correct? As long as we have the right CA certs for verification, it does not affect QUIC-TLS handshake.
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
This PR demonstrates using QUIC in Istio. This is an experimental implementation where I have added a feature which creates mirror listener for an HTTPS listener at the gateway. At the moment, it is not ideal and there are many issues and caveats. I hope to iterate and refine it based on the feedback (relevant comment over the direction - #23018 (comment))
Caveats, open questions, tasks to-do
This is experimental even in upstream Envoy.
Come up with a cleaner approach for this - maybe we can support QUIC protocol on the gateway for the users instead of doing shady things like this? API stuffs are not yet figured out- Running H3/QUIC and H2/TLS is the common case now. So nothing shady here.h2andhttp 1.1as it is just copied over from the filter chain config generated for TCPIf the user wants both TCP+HTTP2 and UDP/QUIC+HTTP3 on the same port, then Kubernetes does not allow having two entries with same service->target port (even when protocols are different) for service of type- It does. In K8s >= 1.20, turn onLoadBalancer. (tested this in Kubernetes 1.21)MixedProtocolLBServicefeature gate.If we have to create UDP listener to a different port specified by the user, then all things like authn, authz policies including ports should be translated as well- Not doing this.Reference on generating QUIC config - https://github.com/envoyproxy/envoy/blob/main/configs/envoyproxy_io_proxy_http3_downstream.yaml
Istio doc - https://docs.google.com/document/d/1Yqx_tCpMlCRDyBqHTyAZfJhjYF9B2BYHyXkHE2xXCho/edit#
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[X] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure