api: support customized UpgradeConfigs

[zirain]

xref: #4859 #3663 #5276

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.09%. Comparing base (c4bc5b2) to head (a82d567).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5300      +/-   ##
==========================================
- Coverage   65.10%   65.09%   -0.02%     
==========================================
  Files         213      213              
  Lines       33586    33586              
==========================================
- Hits        21867    21862       -5     
- Misses      10393    10397       +4     
- Partials     1326     1327       +1     

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

[arkodg]

the other option is calling it backendProtocols, which relates to https://gateway-api.sigs.k8s.io/guides/backend-protocol/

[arkodg]

hey @zirain lets also make sure we address #3663 here
cc @denniskniep

[denniskniep]

Hi @zirain, thanks for tackling that topic!

You added the new config property to the BackendTrafficPolicy. The docs are defining it as:

BackendTrafficPolicy allows the user to configure the behavior of the connection between the Envoy Proxy listener and the backend service.

Isn´t http upgrade_type an HttpRoute specific thing? Just wondering if it makes sense to integrate it as implementation specific HTTPRouteFilter within ExtensionRef
There is already a complex filter defined in the spec: requestMirror

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: upgrade-example-com
spec:
  parentRefs:
  - name: eg
  hostnames:
  - "www.example.com"    
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /test
    filters:
    - type: ExtensionRef
      extensionRef:
        group: gateway.envoyproxy.io
        kind: HTTPRouteFilter
        name: upgrade-websockets
        
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: HTTPRouteFilter
metadata:
  name: upgrade-websockets
spec:
  upgrade:
    protocol: websocket
    disabled: false

Can we also tackle CONNECT_UDP ?

allows HTTP clients to create UDP tunnels through an HTTP proxy server. Unlike CONNECT, which is limited to tunneling TCP, CONNECT-UDP can be used to proxy UDP-based protocols such as HTTP/3.

Furthermore adding ConnectConfig as props would be nice. This can be specified if protocol is CONNECT or CONNECT-UDP

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: HTTPRouteFilter
metadata:
  name: upgrade-connect
spec:
  upgrade:
    disabled: false
    protocol: CONNECT
    connect:
      proxyProtocol:
        version: V2

[zirain]

IMO, upgrade is a thing per backend/port, not per route(even we will put the configuration on route, it's because we share the listener).

Is there a user case, we do the upgrade in one route and not in another route for the same backend?

[denniskniep]

Is there a user case, we do the upgrade in one route and not in another route for the same backend?

tbh, not absolutely sure about that.

The property wouldn't have an effect if we use a tcp-route, right? So therefore I thought properties are added where the effect takes place.

Furthermore there is a related property per route that might be necessary to set in certain scenarios: connect_matcher

see here: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-routematch

[zirain]

The property wouldn't have an effect if we use a tcp-route, right?

Right, we named it as HTTPUpgrade.

[zirain]

Furthermore there is a related property per route that might be necessary to set in certain scenarios: connect_matcher

that's another feature, which is related more than protocol upgrade, in the future, we may need both to support connect matcher.

[arkodg]

unsure here if we should be using Protocol or Type (for type of upgrade)

[arkodg]

we could add a Connect field in this API in the future

connect:
  forwardTCP: false | true // Supports TCP tunnels
  allowHTTP2: false | true
  allowHTTP3: false | true

[arkodg]

LGTM thanks !

[rudrakhp]

Should this be an enum?

[zirain]

the problem is that envoy didn't list all the protocol types it supported in doc.

[zirain]

prefer to leave it as string.

[rudrakhp]

As a control plane we could be strict about what we support, but since proxy allows arbitrary string, so it's fine I guess.

[arkodg]

hey @zirain rethinking think one, as @denniskniep suggested HTTPRouteFilter may be a better fit here especially if we add upgrade.connect to this struct in the future
wdyt @envoyproxy/gateway-maintainers @envoyproxy/gateway-reviewers

[zirain]

Is there a user case, we do the upgrade in one route and not in another route for the same backend?

I replied this before, and we may need both in the future.

[denniskniep]

@zirain @arkodg

Is there a user case, we do the upgrade in one route and not in another route for the same backend?

I personally don´t have currently such a use case. But I guess that such use case can happen. If we just think that the upstream target is also kind of complex ReverseProxy, which offers different services behind one ip+port combination...

My point is why we are adding this feature/property to a resource where it is not as flexible as it would with the other choice/resource. I can´t see any further downsides shifting that property to HttpRoute

Furthermore the upgrade config is something which is going on between downstream and envoy. Maybe upgrade config has also influence to upstream, but that must not be the case. Looking especially at the http-connect config, this has nothing to do with the upstream, meaning BackendRef in our case.

Example:
downstream --Http upgrade:Connect--> envoy --plain tcp--> upstream

In this example the upgrade config is purely a setting between downstream and envoy. It would be counterintuitive to configure it on the upstream (backendRef)

At least my point of view. Am I missing something?

[zirain]

From my point of view, a gateway may have more than 1 httproute.

Thinking of I have N httproute in , before EG support #5351, you need to ref the route filter in every http route for that backend.

I'm not against copy this struct into HTTPRouteFilter so that we can customize for specified route.

ATM, you can ref BTP to Gateway or HTTPRoute, which sounds good to me.

I think this API solves #4859, but for #3663 we need more work on connect_match.