support EnvoyPatchPolicy when enabling mergeGateways

[apjoseph]

Description:
when mergeGateways is true, EnvoyPatchPolicies will not be picked up.

Repro steps:
set mergeGateways to true, create an EnvoyPatchPolicy.

Environment:

0.0.0-latest (12/12/2023)

[shawnh2]

It seems EnvoyPatchPolicy feature is not supported when mergeGateways is enabled.

gateway/internal/gatewayapi/envoypatchpolicy.go

Line 40 in 52946be

gwXdsIR, ok := xdsIR[irKey]

Currently, EnvoyPatchPolicy can be only applied for per Gateway. I am not sure whether MergeGateways should support EnvoyPatchPolicy? cc @arkodg @cnvergence

If we do, it only make sense that the targetRef of EnvoyPatchPolicy should be in the GatewayClass level instead of Gateway level.

Or we may need take a another look at this EnvoyPatchPolicy API design.

[zirain]

@kflynn PTAL

[apjoseph]

As a side comment on patching, I think the generic EnvoyPatchPolicy would benefit from being split up into more targeted CRDs

I'd prefer something like

HttpRoutePatchPolicy
HttpListenerPolicy

with name/namespace/label-based selectors so that you can target specific HttpRoutes and listeners to patch. I think ~95% of users will mostly be patching filters. So you could have the following

EnvoyHTTPRoutePatchPolicy

filterPatches:
  match:
   - type: type.googleapis.com/envoy.extensions.filters.http.cors.v3.CorsPolicy
     replace:
       - op: add 
         path: /typed_per_filter_config/allow_credentials
         value:  true 

EnvoyHttpListenerPatchPolicy

filterPatches:
  match:
   - type: type.googleapis.com/envoy.extensions.filters.http.cors.v3.Cors
      - op: add
         path: /typed_filter_config/bypass_cors_preflight
         value:  true 

The current approach has the RouteConfiguration as the most granular patch level -which is highly problematic since it requires the user to know the order of the routes -and there is no obvious mechanism to enforce/determine the specific order for routes -so that's going to result in people resorting to all kinds of nasty hacks to attempt to apply route-specific functionality.

Just yesterday, I needed to make a patch to specify bypass_cors_preflight: true That's not some edge-case esoteric configuration tweak - it's basically required in order to use an OIDC frontend with a JWT backend API. To do this I needed to apply a listener patch at:
/filter_chains/0/filters/0/typed_config/http_filters/0/typed_config/bypass_cors_preflight Such a patch is extremely fragile and fills me with dread. yet there is no way for me to avoid it.

A targeted selector based approach would be:

1. be much more powerful
2. provide a dramatically improved user experience (I am new to envoy and can attest that it took me a quite a bit of trial and error to correctly apply a patch.)
3. produce dramatically more stable configurations.

I realize the EnvoyPatchPolicy is intended more as an escape hatch than as a permanent solution (hence why it's disabled by default), but if even the webdev 101 JWT + OIDC case requires a patch to work right now -then I think its safe most gateway users are going to heavily be heavily reliant on patches in the short to medium term. Thus I think it is better to address that reality head on with a more targeted approach.

[aigcxiaoyi]

I think mergeGateway and PatchPolicy should not conflict. If we register IR to the xdsIR map when mergeGateway, can this solve the problem? There is no need to distinguish between these two situations, and it is not necessary to bind with GatewayClass. It would be better if we could use httproutepatch more clearly. Our current case urgently needs to use mergeGateway and PatchPolicy at the same time.

[Xunzhuo]

@aigcxiaoyi @apjoseph Thanks for rasing this issue. Yes, it is correct, mergeGateway should not be conflicted with PatchPolicy.

Enabling mergeGateways means that EG will only have one envoyproxy instance, so then the targetRef for EnvoyPatchPolicy may be needed to rise to GatewayClass level.

cc @envoyproxy/envoy-maintainers @envoyproxy/gateway-reviewers and also cc @cnvergence

[cnvergence]

I agree that it should be supported, but like you mentioned it needs to be risen to the GatewayClass with current implementation.
I will take a look if we can somehow pass gateway before processing patch policies.

[fanux]

Should the targetRef for EnvoyPatchPolicy be elevated to the GatewayClass level? After all, there is only one gateway proxy instance, right? I understand that specifying that one envoy proxy should be enough? There should be no difference in handling logic between one gateway proxy and multiple ones? @Xunzhuo @cnvergence

[cnvergence]

For the configuration there should be no difference, but from logical point of view, when you apply EnvoyPatchPolicy when MergeGateways is enabled, this will apply to every Gateway there is on the cluster as well.
This can pose some problems imo, even if this always gonna be experimental API, if we apply something that will interfere with other environments.
I will open a draft PR to test it out a bit.

[zzjin]

hello @cnvergence, it seems that #2320 can trigger EnvoyPatchPolicy been parsed, but the spec/jsonPatches/0/name do not match any gateway.
At doc says:

# The listener name is of the form <GatewayNamespace>/<GatewayName>/<GatewayListenerName>

but when apply example EnvoyPatchPolicy with mergeGateways=true been set, the CR's status give error:

message: "unable to find xds resource
        type.googleapis.com/envoy.config.listener.v3.Listener: default/eg/http"
      reason: ResourceNotFound
      status: "False"
      type: Programmed

Change spec/jsonPatches/0/name to eg/http also shown same error:

message: "unable to find xds resource
        type.googleapis.com/envoy.config.listener.v3.Listener: eg/http"
      reason: ResourceNotFound
      status: "False"
      type: Programmed

[Xunzhuo]

/assign @cnvergence

[cnvergence]

hey @zzjin, thanks for catching this!
it should be the same pattern, I will take a look there as well

[cnvergence]

@zzjin it is working for me in merged gateway, under default listener name default/eg/http

envoy-gateway-67654f8cb9-glpqb envoy-gateway   envoyPatchPolicies:
envoy-gateway-67654f8cb9-glpqb envoy-gateway   - jsonPatches:
envoy-gateway-67654f8cb9-glpqb envoy-gateway     - name: default/eg/http
envoy-gateway-67654f8cb9-glpqb envoy-gateway       operation:
envoy-gateway-67654f8cb9-glpqb envoy-gateway         op: add
envoy-gateway-67654f8cb9-glpqb envoy-gateway         path: /default_filter_chain/filters/0/typed_config/local_reply_config
envoy-gateway-67654f8cb9-glpqb envoy-gateway         value:
envoy-gateway-67654f8cb9-glpqb envoy-gateway           mappers:
envoy-gateway-67654f8cb9-glpqb envoy-gateway           - body:
envoy-gateway-67654f8cb9-glpqb envoy-gateway               inline_string: could not find what you are looking for
envoy-gateway-67654f8cb9-glpqb envoy-gateway             filter:
envoy-gateway-67654f8cb9-glpqb envoy-gateway               status_code_filter:
envoy-gateway-67654f8cb9-glpqb envoy-gateway                 comparison:
envoy-gateway-67654f8cb9-glpqb envoy-gateway                   op: EQ
envoy-gateway-67654f8cb9-glpqb envoy-gateway                   value:
envoy-gateway-67654f8cb9-glpqb envoy-gateway                     default_value: 404
envoy-gateway-67654f8cb9-glpqb envoy-gateway                     runtime_key: key_b
envoy-gateway-67654f8cb9-glpqb envoy-gateway             status_code: 406
envoy-gateway-67654f8cb9-glpqb envoy-gateway       type: type.googleapis.com/envoy.config.listener.v3.Listener
envoy-gateway-67654f8cb9-glpqb envoy-gateway     name: custom-response-patch-policy
envoy-gateway-67654f8cb9-glpqb envoy-gateway     namespace: default
envoy-gateway-67654f8cb9-glpqb envoy-gateway     status:
envoy-gateway-67654f8cb9-glpqb envoy-gateway       conditions:
envoy-gateway-67654f8cb9-glpqb envoy-gateway       - lastTransitionTime: "2023-12-21T16:23:16Z"
envoy-gateway-67654f8cb9-glpqb envoy-gateway         message: EnvoyPatchPolicy has been accepted.
envoy-gateway-67654f8cb9-glpqb envoy-gateway         observedGeneration: 1
envoy-gateway-67654f8cb9-glpqb envoy-gateway         reason: Accepted
envoy-gateway-67654f8cb9-glpqb envoy-gateway         status: "True"
envoy-gateway-67654f8cb9-glpqb envoy-gateway         type: Accepted

envoy-gateway-system   envoy-example-class-928fa378-599bb994b-ttljp          1/1     Running   0          85m
envoy-gateway-system   envoy-gateway-67654f8cb9-glpqb                        1/1     Running   0          84m

curl --header "Host: www.example.com" http://172.18.255.200/find   
could not find what you are looking for

[zzjin]

Hi @cnvergence
After using latest main branch's codebase, I still have same error.
Since we're apply gateway under our namespace instead of default, is that the matter?

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: v1
kind: ConfigMap
metadata:
  name: envoy-gateway-config
  namespace: envoy-gateway-system
data:
  envoy-gateway.yaml: |
    apiVersion: gateway.envoyproxy.io/v1alpha1
    kind: EnvoyGateway
    provider:
      type: Kubernetes
    gateway:
      controllerName: gateway.envoyproxy.io/gatewayclass-controller
    extensionApis:
      enableEnvoyPatchPolicy: true

---
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: eg
  namespace: sealos-system
spec:
  gatewayClassName: eg
  listeners:
    - name: http
      protocol: HTTP
      port: 80
      allowedRoutes:
        namespaces:
          from: All
    - name: https
      protocol: HTTPS
      port: 443
      allowedRoutes:
        namespaces:
          from: All
      tls:
        mode: Terminate
        certificateRefs:
          - kind: Secret
            group: ""
            name: wildcard-cert

---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyPatchPolicy
metadata:
  name: test-policy
  namespace: sealos-system
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: GatewayClass
    name: eg
    namespace: sealos-system
  type: JSONPatch
  jsonPatches:
  - type: "type.googleapis.com/envoy.config.listener.v3.Listener"
    name: "sealos-system/eg/http"
    operation:
      op: add
      path: "/default_filter_chain/filters/0/typed_config/local_reply_config"
      value:
        mappers:
        - filter:
            status_code_filter:
              comparison:
                op: EQ
                value:
                  default_value: 404
                  runtime_key: key_b
          status_code: 406
          body:
            inline_string: "not acceptable"

---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: sealos-desktop
  namespace: sealos
spec:
  parentRefs:
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: eg
      sectionName: https
      namespace: sealos-system
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: eg
      sectionName: http
      namespace: sealos-system
  hostnames:
    - "34.84.109.10.nip.io"
  rules:
    - backendRefs:
        - group: ""
          kind: Service
          name: desktop-frontend
          port: 3000
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /

[cnvergence]

Hey @zzjin, might be the case, I have deployed the docs example under default ns
I will take a look when available