EnvoyFilter not matching on other EnvoyFilter with lower priority

[breuerfelix]

Bug Description

We have a TCP Listener 0.0.0.0_8443 with a simple TCP Proxy as the default filter_chain.
The following EnvoyFilter creates a new filter_chain on the Listener that matches a certain address_prefix and proxies to a different location:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: define-filter-chain
  namespace: istio-ingress
spec:
  configPatches:
  - applyTo: FILTER_CHAIN
    match:
      context: ANY
      listener:
        name: 0.0.0.0_8443
        portNumber: 8443
    patch:
      operation: ADD
      value:
        filter_chain_match:
          destination_port: 443
          prefix_ranges:
          - address_prefix: 10.10.10.10
            prefix_len: 32
        filters:
        - name: envoy.filters.network.tcp_proxy
          typed_config:
            '@type': type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy
            cluster: outbound|443||kube-apiserver.shoot.svc.cluster.local
            stat_prefix: outbound|443||kube-apiserver.shoot.svc.cluster.local
        name: foobar
  priority: -10
  workloadSelector:
    labels:
      app: istio-ingressgateway
      istio: ingressgateway

We add another EnvoyFilter that has a higher priority and should add an rbac rule into the filter_chain created by the first EnvoyFilter.
However, the second EnvoyFilter is never applied in the resulting Envoy config.
We tried matching by destinationPort, filterchain name and also by specifying the networkfilter name (envoy.filters.network.tcp_proxy) without success.

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: authorization-policies
  namespace: istio-ingress
spec:
  workloadSelector:
    labels:
      app: istio-ingressgateway
      istio: ingressgateway
  priority: 10
  configPatches:
  - applyTo: NETWORK_FILTER
    match:
      context: ANY
      listener:
        filterChain:
          name: foobar
    patch:
      operation: INSERT_FIRST
      value:
        name: authorization-policies
        typed_config:
          "@type": type.googleapis.com/envoy.extensions.filters.network.rbac.v3.RBAC
          stat_prefix: envoyrbac
          rules:
            action: DENY
            policies:
              authorization-policies:
                permissions:
                - any: true
                principals:
                - any: true

See the resulting envoy config in the additional informations.

Version

istioctl version:
client version: 1.15.0
control plane version: 1.14.1
data plane version: 1.14.1 (2 proxies)

kubectl version:
Client Version: v1.24.0
Server Version: v1.21.11

Additional Information

This is the resulting filter_chain from the envoy config_dump:

{
 "filter_chain_match": {
  "prefix_ranges": [
   {
    "address_prefix": "10.10.10.10",
    "prefix_len": 32
   }
  ],
  "destination_port": 443
 },
 "filters": [
  {
   "name": "envoy.filters.network.tcp_proxy",
   "typed_config": {
    "@type": "type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy",
    "stat_prefix": "outbound|443||kube-apiserver.shoot.svc.cluster.local",
    "cluster": "outbound|443||kube-apiserver.shoot.svc.cluster.local"
   }
  }
 ],
 "name": "foobar"
},

This setup is inside the gardener/gardener environment.
The first EnvoyFilter is created by another part of the system which we do not have control over.
Our current workaround is to write a mutating Webhook which mutates the first Envoyfilter in order to add the rbac rules. The ability to just add the second EnvoyFilter which mutates the filter_chain would simplify our setup a lot.

[hzxuzhonghu]

I think this is expected, because when istio applies the second patch, it couldn't match the filterchain as the low priority patch is not applied yet

[SimonKienzler]

@hzxuzhonghu thanks for the reply, I'm working with @breuerfelix on this. We read in the docs:

The default value for priority is 0 and the range is [ min-int32, max-int32 ]. A patch set with a negative priority is processed before the default. A patch set with a positive priority is processed after the default.

Istio EnvoyFilter Docs

So our assumption was that a lower priority EnvoyFilter is applied first, and then our second, higher priority EnvoyFilter is applied after. Funny enough, we also tried it the other way around, too (because we didn't have any other ideas). That also didn't work. So the problem remains, unfortunately.

[hzxuzhonghu]

Thanks, IC. Different type of patches do not respect the priority value here.
Current implementation is: patchFilterChains -> patchFilterChain(-->patchNetworkFilters) -> filterChain add

https://github.com/istio/istio/blob/master/pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch.go#L132-L168
The patching orders for different types is not defined, I am not sure if should we first patch wider scope xds, And for same type like filterChain, should we first apply Add, then Merge, and last Remove. @howardjohn @ramaraochavali WDYT?

[breuerfelix]

@hzxuzhonghu thanks for investigating on how the patches apply. Now it is clear why our patch won't apply.

I would assume that ALL filters are first sorted by priority and than all patches on the same priority are sorted by the algorithm you explained (patchFilterChains -> patchFilterChain(-->patchNetworkFilters) -> filterChain add). That way, the user has the ability to fine tune when which filter gets merged :)

/edit
Nonetheless, i guess filterChain add should apply before patchNetworkfilters, since they are a little higher order in my opinion (even though they are also part of the networkfilters somehow...)

[hzxuzhonghu]

I do have the same kind of feeling for the last point

[ramaraochavali]

I think it makes sense to add filter chains first before we process patches that apply to filter chains. I think that should solve this case?

[breuerfelix]

That should solve issue, but i would suggest solving it on a different basis.
What about respecting the priority first? Doesn't matter in what order you are applying the patches internally. If i have a filter with prio 10 and one with -10, i assume that the one with prio 10 is ALWAYS applied after the one with -10. Does this makes sense?

The priority doesn't really makes sense to me, if you have a "hidden" priority which beats the priority from the customer.

[hzxuzhonghu]

/not stale

[breuerfelix]

/not stale

[ramaraochavali]

Not stale. This is still needed.

[wulianglongrd]

hello @ramaraochavali , do you have any fix ideas? Apply strictly according to the priority specified by priority? Or apply in a fixed patch.operation order and then use priority?

[hzxuzhonghu]

IMO， should first should by priority.

Then for each set of patches of same priority, patch like what we do currently

[ramaraochavali]

IMO， should first should by priority.

I agree. This is what I am thinking. Honour priority fully/

[hzxuzhonghu]

Appreciate anyone willing to take it

[wulianglongrd]

Behavior-breaking changes seem to be difficult to be backwards compatible. Do we need to consider backwards compatibility?

[hzxuzhonghu]

Yes, but we can have a feature env

[howardjohn]

We cannot just add a feature env if there is no path off if it; It adds to the problem, not lessens it. So if we do please propose a path to removing it. (I will also follow up to make this an official policy)

…

On Sat, Oct 7, 2023, 10:50 PM Zhonghu Xu ***@***.***> wrote: Yes, but we can have a feature env — Reply to this email directly, view it on GitHub <#41536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKS2P7HKBW7KY6LN3TX6IPH3AVCNFSM6AAAAAARJ4NPFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJRHEYTAOJQG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[husnialhamdani]

/not stale

[ramaraochavali]

Since Envoy filters is a break glass API, if we decide to make this change, we can introduce a feature flag that can be removed in couple of releases to give time for people to adjust their filters? My guess is not many customers should be impacted