EnvoyFilter patch does not work as expected

[dansiviter]

Bug description
Applying a HTTP_FILTER via EnvoyFilter is very confusing and requires a lot of trial and error. I would have thought it should be possible to just ADD but it appears it must appear before the predefined envoy.router or before/after envoy.cors or envoy.fault filters to appear.

The 'fix' for this may just be documenting the correct usage.

Expected behaviour
EnvoyFilter of HTTP_FILTER should be able to ADD config.

Steps to reproduce the bug
Use the following config:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: tagger
  namespace: istio-system
spec:
  configPatches:
  - applyTo: HTTP_FILTER
    match:
      context: GATEWAY
      listener:
        filterChain:
          filter:
            name: "envoy.http_connection_manager"
            subFilter:
              name: "envoy.router"
    patch:
      operation: ADD  // this doesn't work, only INSERT_BEFORE with 'envoy.router' or INSERT_BEFORE/INSERT_AFTER with 'envoy.cors' or 'envoy.fault'
      value:
        name: envoy.ip_tagging
        config:
          request_type: INTERNAL
          ip_tags:
          - ip_tag_name: GOTCHA
            ip_list:
            - address_prefix: 0.0.0.1

Then perform: istioctl proxy-config listener istio-ingressgateway-N -n istio-system -o json

Notice the config is not applied. When changing the following operation: INSERT_BEFORE it appears. I've tried also dropping out the subFilter section and I would expect it would just add the config at the end, but that doesn't work either.

There is a working example here: https://discuss.istio.io/t/ip-tagging-configuration/5377/3?u=dansiviter

Version (include the output of istioctl version --remote and kubectl version and helm version if you used Helm)
Istio

client version: 1.4.0
control plane version: 1.4.4
data plane version: 1.4.4 (4 proxies)

Helm

version.BuildInfo{Version:"v3.1.0", GitCommit:"b29d20baf09943e134c2fa5e1e1cab3bf93315fa", GitTreeState:"clean", GoVersion:"go1.13.7"}

Kube

Client Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.5", GitCommit:"20c265fef0741dd71a66480e35bd69f18351daea", GitTreeState:"clean", BuildDate:"2019-10-15T19:16:51Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.5", GitCommit:"20c265fef0741dd71a66480e35bd69f18351daea", GitTreeState:"clean", BuildDate:"2019-10-15T19:07:57Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}

How was Istio installed?
Helm

Environment where bug was observed (cloud vendor, OS, etc)
Docker for Mac

[dansiviter]

I've found the error:

Error: envoy.router must be the terminal http filter.

Which has lead me to envoyproxy/envoy#7767. Therefore ADD will never work with HTTP_FILTER or INSERT_AFTER with envoy.router which is not mentioned in the documentation:

ADD: Add the provided config to an existing list (of listeners, clusters, virtual hosts, network filters, or http filters). This operation will be ignored when applyTo is set to ROUTE_CONFIGURATION, or HTTP_ROUTE.

[esnible]

@istio/wg-networking-maintainers Currently the analyze framework doesn't have any rules for EnvoyFilter. Is there someone on the Networking team who either wants to work with me to produce either an admission webhook or analysis warning/info message for this problem?

[howardjohn]

I really don't think its feasible to attempt to implement the XDS spec as an analyzer. The API is break glass, if you use it wrong that is the users fault. Its not intended to be easy to use or prevent you from shooting yourself in the foot.

That error message is coming directly from Envoy, right? We should just send bad config to envoy (as we do today) and the user can monitor the appropriate metrics (they are already on our dashboards) to investigate envoy errors

[kyessenov]

It's not clear whether we should promote this API to beta or stable.
There are proposals to make it more usable (e.g. istio/api#1221)

[esnible]

@kyessenov There is nothing in https://preliminary.istio.io/docs/reference/config/networking/envoy-filter/ suggesting that EnvoyFilter is experimental, other than v1alpha3 in the apiVersion in the YAML example. If EnvoyFilter is not on the path to becoming beta should we include a warning in the docs?

@howardjohn We created istioctl analyze because users were not figuring out how to monitor complaints coming from Envoy as metrics. If users are trying to use EnvoyFilter and struggling we could warn them. Are you worried implementing a few warnings would give users a false sense of security because not all possible problems would have warnings? I was thinking to tag this as help wanted but I don't want to encourage someone to write the analyzer if it would be problematic to merge it.

[howardjohn]

@esnible you can generate literally any Envoy config with this API. Its not feasible to capture even 1% of all possible ways this could go wrong. and I don't think its a useful investment in time/effort.

And we do have a note about the stability:

NOTE 1: Some aspects of this API is deeply tied to the internal implementation in Istio networking subsystem as well as Envoy’s XDS API. While the EnvoyFilter API by itself will maintain backward compatibility, any envoy configuration provided through this mechanism should be carefully monitored across Istio proxy version upgrades, to ensure that deprecated fields are removed and replaced appropriately.

[dansiviter]

From a user's perspective, I get that it's not possible to capture all the possible permutations, especially as it's effectively a 3rd party tool. However, if was made easier to find the root cause then we (users) wouldn't waste two man days trying to figure out why Envoy config just disappears. Therefore, exposing the underlying error would be a helpful.

[howardjohn]

Better exposing Envoy errors is a great idea, I just don't think its feasible to try to look at EnvoyFilters and try to determine if they are valid or not.

[kyessenov]

@esnible Yes, a warning is needed I think, but I am not an owner of the networking APIs.

[rshriram]

I think the problem here is insufficient documentation in the EnvoyFilter API for things like ADD. the OP had clearly read through the currently documented options and tried to follow what we suggested. @dansiviter would you like to shoot a PR to istio/api to document these things? I can do it as well but let me know.

[esnible]

I encourage a Traffic Management documentation Task that used EnvoyFilter and provided a valid one. Many people would prefer to start with an example and modify it from their use rather than construct one based on reading the reference info.

I don't have the skill to write such a Task. Anyone?

[dansiviter]

I'm probably not a good candidate as I neither know the code or how it's supposed to work. Sorry!

[howardjohn]

https://github.com/istio/istio/wiki/EnvoyFilter-Samples may help

…

On Fri, Feb 21, 2020, 7:12 AM dansiviter ***@***.***> wrote: I'm probably not a good candidate as I neither know the code or how it's supposed to work. Sorry! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21265?email_source=notifications&email_token=AAEYGXMMV3C7VOZWY2MWXHLRD7VNTA5CNFSM4KXXUNYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMTAPKA#issuecomment-589694888>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXLPISGBPA543DF56GDRD7VNTANCNFSM4KXXUNYA> .

[esnible]

@howardjohn I checked the samples on the Wiki by applying them and using istioctl ps to see if the sidecars went STALE.

trace-sampling makes the sidecars go LDS/STALE.

I am not saying we should write a general purpose XDS validator. I am suggesting that analyze would be improved by adding INFO-level checking things we did in previous version official Istio examples that break Envoy in the current version.

[howardjohn]

I don't think writing that code is something that is feasible without literally applying it and checking against 2 envoys. We don't know what breaks between versions in many cases

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-07-31. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

[DiamondJoseph]

This appears to still be an issue, and is still shown on the documentation for Istio as a valid EnvoyFilter configuration (and the documentation for FilterClass says it is the preferred way of injecting filters).

Example 5 on https://istio.io/latest/docs/reference/config/networking/envoy-filter/
https://istio.io/latest/docs/reference/config/networking/envoy-filter/#EnvoyFilter-Patch-FilterClass

Would it be possible to have a terminal filter on the AuthN/AuthZ (presumably also an issue for Stats?) by default?

[GregHanson]

@DiamondJoseph can you open a separate issue with the exact EnvoyFIlter you tried and your istio version? The above EnvoyFilter will not work work with latest version since there are some naming changes to various fields when Istio switched to Envoy v3 API's in 1.8

[howardjohn]

We should definitely update the samples to work

[DiamondJoseph]

@GregHanson please let me know if you require any further information, I have converted the Helm Library Chart we're using internally to be as close to deployable as possible without including Secrets.
#39882