infra: add support for merging gateways on to a single EnvoyProxy by cnvergence · Pull Request #1938 · envoyproxy/gateway

cnvergence · 2023-10-09T21:26:11Z

What type of PR is this?

What this PR does / why we need it:

Setting the mergeGateways field in the EnvoyProxy resource linked to GatewayClass will result in merging
all Gateway Listeners
Ensure that the tuple of port, protocol, and hostname is unique across all Listeners

Which issue(s) this PR fixes:

Fixes:

Merge Gateways on to a single EnvoyProxy fleet #1230

codecov · 2023-10-10T18:28:05Z

Codecov Report

Merging #1938 (c1f808e) into main (23c91a6) will increase coverage by 0.15%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##             main    #1938      +/-   ##
==========================================
+ Coverage   65.55%   65.70%   +0.15%     
==========================================
  Files          92       92              
  Lines       13509    13583      +74     
==========================================
+ Hits         8856     8925      +69     
- Misses       4107     4111       +4     
- Partials      546      547       +1

Files	Coverage Δ
internal/gatewayapi/address.go	`100.00% <100.00%> (ø)`
internal/gatewayapi/helpers.go	`83.78% <100.00%> (+0.25%)`	⬆️
internal/gatewayapi/listener.go	`97.72% <100.00%> (-0.07%)`	⬇️
internal/gatewayapi/route.go	`87.44% <100.00%> (+0.05%)`	⬆️
internal/gatewayapi/translator.go	`98.13% <100.00%> (+0.63%)`	⬆️
internal/gatewayapi/validate.go	`89.65% <100.00%> (+0.34%)`	⬆️
...ternal/infrastructure/kubernetes/proxy/resource.go	`91.39% <100.00%> (ø)`
internal/provider/kubernetes/controller.go	`55.32% <0.00%> (+1.08%)`	⬆️
internal/provider/kubernetes/helpers.go	`79.19% <50.00%> (-2.02%)`	⬇️
...frastructure/kubernetes/proxy/resource_provider.go	`86.54% <33.33%> (+0.07%)`	⬆️
... and 1 more

internal/gatewayapi/listener.go

internal/gatewayapi/testdata/multiple-gateways.in.yaml

internal/gatewayapi/testdata/merge-valid-multiple-gateways.in.yaml

arkodg · 2023-10-13T22:53:54Z

internal/gatewayapi/helpers.go

will irRouteName and irRouteDestinationName collide since now there is nothing unique if the routes attach to multiple gateways resources which now will manifests into a single xds config
or can we reuse ?

and you then want to do something similar to https://github.com/envoyproxy/gateway/blob/2ecc54da663dc10c9257a61843cd14ba1bb77dcc/internal/provider/utils/utils.go#L33C1-L33C1 in the infra layer side

internal/gatewayapi/validate.go

internal/infrastructure/kubernetes/proxy/resource.go

arkodg · 2023-10-13T23:22:44Z

internal/gatewayapi/helpers.go

lets flip the order of concatenation, similar to irHTTPListenerName ? and we cannot use -, we need to use / , an invalid char as a delimiter

alright, I thought I could use - as this is mainly used for container ports, but I will change it to / and add this in the infrastructure helper function

internal/provider/kubernetes/controller.go

internal/provider/kubernetes/helpers.go

internal/provider/kubernetes/predicates.go

arkodg · 2023-10-13T23:47:21Z

this is looking pretty good @cnvergence
added some comments
would be great if you can add some more comments in the code as well so devs can understand why the gatewayclass label is being used instead of the gateway label or why the IR keys are being computed differently

arkodg · 2023-10-16T19:09:00Z

hey @cnvergence guessing now with this change when mergeGateway: true, the envoy proxy data plane fleet is created and deleted based on when a new gatewayclass is accepted and removed ?

cnvergence · 2023-10-17T09:19:53Z

hey @cnvergence guessing now with this change when mergeGateway: true, the envoy proxy data plane fleet is created and deleted based on when a new gatewayclass is accepted and removed ?

hey @arkodg, yes, the envoy proxy fleet is redeployed when gatewayclass is referencing mergeGateways: true

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Xunzhuo

LGTM, hope this can land in v0.6.0-rc1, tested locally and it works well:

Config:

apiVersion: gateway.networking.k8s.io/v1
kind: GatewayClass
metadata:
  name: eg
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
  parametersRef:
    group: gateway.envoyproxy.io
    kind: EnvoyProxy
    name: custom-proxy-config
    namespace: envoy-gateway-system
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
  name: custom-proxy-config
  namespace: envoy-gateway-system
spec:
  mergeGateways: true
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: merged-eg-1
  namespace: default
spec:
  gatewayClassName: eg
  listeners:
  - allowedRoutes:
      namespaces:
        from: Same
    name: http
    port: 8080
    protocol: HTTP
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: merged-eg-2
  namespace: default
spec:
  gatewayClassName: eg
  listeners:
  - allowedRoutes:
      namespaces:
        from: Same
    name: http
    port: 8081
    protocol: HTTP
---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: merged-eg-3
  namespace: default
spec:
  gatewayClassName: eg
  listeners:
  - allowedRoutes:
      namespaces:
        from: Same
    name: http
    port: 8082
    protocol: HTTP
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: hostname1-route
spec:
  parentRefs:
    - name: merged-eg-1
  hostnames:
    - "www.example.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /example
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: hostname2-route
spec:
  parentRefs:
    - name: merged-eg-2
  hostnames:
    - "www.example2.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /example2
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: hostname3-route
spec:
  parentRefs:
    - name: merged-eg-3
  hostnames:
    - "www.example3.com"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: backend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /example3

Result:

❯ kg gateway -A
NAMESPACE   NAME          CLASS   ADDRESS          PROGRAMMED   AGE
default     merged-eg-1   eg      172.18.255.200   True         12m
default     merged-eg-2   eg      172.18.255.200   True         12m
default     merged-eg-3   eg      172.18.255.200   True         12m
❯ kg httproute -A
NAMESPACE   NAME              HOSTNAMES              AGE
default     hostname1-route   ["www.example.com"]    27s
default     hostname2-route   ["www.example2.com"]   27s
default     hostname3-route   ["www.example3.com"]   27s
❯ kg svc -n envoy-gateway-system
NAME                            TYPE           CLUSTER-IP      EXTERNAL-IP      PORT(S)                                        AGE
envoy-eg-d8c59e83               LoadBalancer   10.96.161.114   172.18.255.200   8080:31539/TCP,8081:30783/TCP,8082:31013/TCP   12m
envoy-gateway                   ClusterIP      10.96.116.219   <none>           18000/TCP,18001/TCP                            14m
envoy-gateway-metrics-service   ClusterIP      10.96.69.160    <none>           8443/TCP                                       14m
❯ kg pod -n envoy-gateway-system
NAME                                 READY   STATUS    RESTARTS   AGE
envoy-eg-d8c59e83-68fdf67bc9-pgb2v   1/1     Running   0          95s
envoy-gateway-6f557cc687-h8nxh       2/2     Running   0          14m

Maybe we need an e2e test for this as a follow-up, and also a user-guide docs.

arkodg · 2023-10-20T17:52:08Z

internal/provider/utils/utils.go

+		resourceName := strings.ReplaceAll(name, "/", "-")
+		listenerName := string(resourceName[2])
+
+		return fmt.Sprintf("%s-%s", listenerName, hashedName[0:14-len(listenerName)])


this is going to be prone to collisions, id recommend hardcoding this to 7 chars of the listener and 8 chars of the hash.

Yeah, probably it makes more sense, I don't like it either
Initially didn't want to make the listener name that short, I will change it like you proposed in the follow-up

arkodg · 2023-10-20T17:56:19Z

internal/infrastructure/kubernetes/proxy/resource_provider.go

 			if port.Protocol == ir.UDPProtocolType {
 				protocol = corev1.ProtocolUDP
 			}
+			// Listeners on merged gateways will have a port name {GatewayNamespace}/{GatewayName}/{ListenerName}.


doesnt this need to be hashed to ensure length is under 15 ?

AFAIK, Service port names apply under standard DNS label names, then it should be fine

that means the length is 63 ? but im guessing the individual max length of GatewayNamespace , GatewayName & ListenerName is 63 so this need to be trimmed

That should be trimmed as well, let's just reuse ExpectedResourceHashedName func for it

arkodg

LGTM @cnvergence, thanks for adding this feature

there are a couple of comments mainly around hashed names of container ports that can be addressed in a follow up PR

@tamalsaha can you also help test this feature out

tamalsaha · 2023-10-21T09:33:11Z

Thanks! Does this support cross namespace gateway merge?

arkodg · 2023-10-21T15:11:32Z

@tamalsaha yes it should

cnvergence force-pushed the merge-gateways branch from 5ce28ba to 152eae3 Compare October 9, 2023 21:26

cnvergence changed the title ~~merge gateways support in translator~~ infra: merge gateways on to a single EnvoyProxy Oct 9, 2023

cnvergence force-pushed the merge-gateways branch 2 times, most recently from 4874209 to 1cb2866 Compare October 10, 2023 12:55

cnvergence changed the title ~~infra: merge gateways on to a single EnvoyProxy~~ infra: support merging gateways on to a single EnvoyProxy Oct 10, 2023

cnvergence force-pushed the merge-gateways branch 3 times, most recently from 7de9e4c to ecfb164 Compare October 11, 2023 16:01

cnvergence changed the title ~~infra: support merging gateways on to a single EnvoyProxy~~ infra: add support for merging gateways on to a single EnvoyProxy Oct 11, 2023

cnvergence force-pushed the merge-gateways branch 2 times, most recently from 6e6d2e1 to 1f0e697 Compare October 13, 2023 16:24

cnvergence marked this pull request as ready for review October 13, 2023 16:26

cnvergence requested a review from a team as a code owner October 13, 2023 16:26