Envoy filter configurations #447
Conversation
Signed-off-by: Shriram Rajagopalan <[email protected]>
googlebot
added
the
cla: yes
| // **A host name can be defined by only one PluginConfiguration**. | ||
| // | ||
| // *Note for Kubernetes users*: When short names are used (e.g. "reviews" | ||
| // instead of "reviews.default.svc.cluster.local"), Istio will interpret |
There was a problem hiding this comment.
This is an important point but it's probably better situated further up the tree since it applies generally. Suggest putting a reference here.
| // dots in the name). In such a scenario, the FQDN of the host would be | ||
| // derived based on the underlying platform. | ||
| // | ||
| // **A host name can be defined by only one PluginConfiguration**. |
There was a problem hiding this comment.
Suggestion: Host names must be unique among all PluginConfigurations due to...
Why is this the case?
| // rules will apply only to the gateways. To apply the configuration to | ||
| // both gateways and sidecars, specify `mesh` as one of the gateway | ||
| // names. | ||
| repeated string gateways = 2; |
There was a problem hiding this comment.
these two statements appear to be inconsistent or at least confusing:
- The names of gateways and sidecars that should apply these routes.
- If a list of gateway names is provided, the rules will apply only to the gateways.
can statement 2 be dropped since it seems self-evident? unless it means that sidecar names will then be excluded which contradicts 1.
also, suggest making this a oneof with the following choices:
- enum type of ALL_SIDECARS/GATEWAYS or just ALL_MESH
- repeated gateways_and_sidecards
special strings are best avoided. in this case mixing this with normal names makes it hard to reason about where the configs will apply.
There was a problem hiding this comment.
this was litigated when designing VirtualService and we favored 'special' strings for a number of reasons.
- sidecars are really just a special form of gateway, at some point we may configure them using the gateway resource (they're all just proxies after all)
- we may at some point support more than one sidecar on a pod at which point they would need to be named
- the enum + gateways field wouldn't cover the above and was not considered more readable as an API
There was a problem hiding this comment.
In case of mesh would the host be empty?
There was a problem hiding this comment.
ok, then could the description be reworked at least? an example would help.
|
|
||
| message Plugins { | ||
| string name = 1; | ||
| string inbound_config = 2; |
There was a problem hiding this comment.
would it be possible to specify the position of the filter in the list of filter. At least a way to say 1st/last/don't care.
| } | ||
|
|
||
| message Plugins { | ||
| string name = 1; |
There was a problem hiding this comment.
comment that this is required.
There was a problem hiding this comment.
and document what it refers to, ideally with an example referring into the Envoy docs
There was a problem hiding this comment.
is the name here the name of the filter in Envoy?
|
cc @spikecurtis |
| // There can be only one plugin configuration object per host. When the | ||
| // host does not exist in the service registry, the plugin configuration | ||
| // must be accompanied with a virtualservice configuration that uses the | ||
| // same hosts. |
There was a problem hiding this comment.
This design seems to make the implicit assumption that a host will be part of either 0 or 1 services. In reality a host could be part of any number of services (2+ is possible).
We need to either pick an identifier with which a host can be associated with at most one instance (e.g. SPIFFE ID/service account), or develop a mechanism for combining multiple PluginConfigurations when more than one applies.
There was a problem hiding this comment.
This is essentially copying the virtual services spec. and we have certainly taken into account things like a host being part of multiple services, etc. Take a look at the virtual service doc for details..
There was a problem hiding this comment.
Thanks. I've had a look and I think I understand how it works for "inbound" filters, but I'm not clear how this config behaves for "outbound". If I have outbound plugin for host foo.default.svc.cluster.local, does the corresponding filter appear in the listener on clients attempting to connect to foo.default.svc.cluster.local, or does it appear in listeners that are outbound from pods in foo.default.svc.cluster.local when those pods connect to other services?
There was a problem hiding this comment.
Former, from clients to foo.default.. and inbound is from any client to foo.default..
Sorry, this PR is a WIP. I should have mentioned that, as there is a lot of text missing
There was a problem hiding this comment.
Just so I'm clear, we are proposing
- outbound call from pod A to host X, filter applies on pod A's sidecar
- inbound call to pod A implementing host X, filter applies on pod A's sidecar
Only #2 would apply to gateways I assume.
Note that this is pod-oriented in the sense that if we want to filter the things this rule should apply to the only reasonable selection mechanism would be over pods (we've already declared one for gateways)
|
|
||
| message Plugins { | ||
| string name = 1; | ||
| string inbound_config = 2; |
There was a problem hiding this comment.
Does this support Envoy filters that are not lua? How?
| // *Note for Kubernetes users*: When short names are used (e.g. "reviews" | ||
| // instead of "reviews.default.svc.cluster.local"), Istio will interpret | ||
| // the short name based on the namespace of the rule, not the service. A | ||
| // rule in the "default" namespace containing a host "reviews will be |
There was a problem hiding this comment.
s/"reviews will/"reviews" will
| string name = 1; | ||
| string inbound_config = 2; | ||
| string outbound_config = 3; | ||
| } |
There was a problem hiding this comment.
Do we need precedence? e.g. before / after
| } | ||
|
|
||
| message Plugins { | ||
| string name = 1; |
There was a problem hiding this comment.
is the name here the name of the filter in Envoy?
|
|
||
| message Plugins { | ||
| string name = 1; | ||
| string inbound_config = 2; |
There was a problem hiding this comment.
would it be possible to specify the position of the filter in the list of filter. At least a way to say 1st/last/don't care.
| // rules will apply only to the gateways. To apply the configuration to | ||
| // both gateways and sidecars, specify `mesh` as one of the gateway | ||
| // names. | ||
| repeated string gateways = 2; |
There was a problem hiding this comment.
In case of mesh would the host be empty?
|
@saumoh We have explicitly refrained from adding clusters in adhoc manner. Clusters are generated only for services in the registry. So, if your auth service is an external service, add it as an ExternalService and it will appear as a cluster in Envoy (whose name has a specific format). This makes it easy to control all cluster config from one place. |
|
@rshriram the cluster we want to add is another sidecar available over a Unix Domain Socket. |
|
@rshriram The model of ExternalServices could be used as long as we are willing to enhance the https://github.com/istio/api/blob/master/networking/v1alpha3/external_service.proto to be able to specify a UDS. |
|
I would associate a lua script (or any other plugin) with a specific Since Certain plugin may not support certain associations, but we should not rule them out in the model. |
| // the mesh, i.e., those found in the service registry, must always be | ||
| // referred to using their alphanumeric names. IP addresses are allowed | ||
| // only for services defined via the Gateway. | ||
| repeated string hosts = 1; |
There was a problem hiding this comment.
Could we add an example for how to address all the sidecars in the cluster so that the plugin is applied to all the sidecars but not the ingress/egress gateways. thx!
Signed-off-by: Shriram Rajagopalan <[email protected]>
|
Based on prior feedback, reworked the PR again to include ordering. Borrowed some ideas from #479 |
|
|
||
| // The name of the filter to instantiate. The name must match a supported | ||
| // filter _compiled into_ Envoy. | ||
| string filter_name = 3; |
There was a problem hiding this comment.
Just to be clear, there will be pilot plugins to process each filter_name. It is unlikely that we can just place these contents verbatim in the envoy config.
There was a problem hiding this comment.
Also I think this is too close to Evnoy filter architecture, and no attempt is made to abstract it :-), based on the proto names.
How does one do a different proxy ?
There was a problem hiding this comment.
You create a different CRD. This entire CRD is called EnvoyFilter.
There was a problem hiding this comment.
Also, this is not for plugins. This is verbatim filter configuration that people put in here and we simply stick it into our generated config.
|
|
||
| // Inbound vs outbound sidecar listener or gateway listener. If not specified, | ||
| // matches all listeners. | ||
| ListenerType listener_type = 2; |
There was a problem hiding this comment.
We also want to be able to distinguish between HTTP and TCP listeners
There was a problem hiding this comment.
Why? Pilot will know the difference automatically. Filter config structure is same for both.
There was a problem hiding this comment.
Let me rephrase: a cluster operator will want to distinguish between HTTP and TCP listeners.
As an example use case, there are two ext_auth filters, a network filter and an HTTP filter. I want to insert the network filter only on TCP listeners and the HTTP filter only on HTTP listeners.
There was a problem hiding this comment.
Yes. but the HTTP and TCP will be on different ports. The Listener match will allow you to specify different filters for different ports. I could add a protocol field (http or tcp) in the listener match and that allows selecting all http ports or all non-http ports. Would that help?
I see no point in having the exact same struct be called as http Filter or tcp Filter, but contain the exact same semantics. To Pilot, this is very simple: stick it verbatim into the listener filter chain in the position mentioned.
There was a problem hiding this comment.
I could add a protocol field (http or tcp) in the listener match and that allows selecting all http ports or all non-http ports. Would that help?
Yes, that would be great. We want to be able to select all http listeners without having to know the specific port numbers, which will in general vary among services.
| }; | ||
|
|
||
| // Envoy network filters/http filters to be added to matching listeners. | ||
| repeated Filter filters = 2; |
There was a problem hiding this comment.
This needs to distinguish between network filters and HTTP filters, otherwise Pilot won't know which one to try to insert.
There was a problem hiding this comment.
See above. It’s enough to pick a specific listener.
There was a problem hiding this comment.
It's not safe to assume that an HTTP listener implies an HTTP filter. Operators might want to insert a network filter on an HTTP listener. For example, they might have a network filter they want inserted on every listener.
There was a problem hiding this comment.
YEs, but thats not envoy architecture. HTTP and TCP have completely different filter chains and these chains are not shared in any way. In other words, HTTP listener implies HTTP filters only. Other filters are not possible with the envoy architecture we have today.
| }; | ||
|
|
||
| // Indicates the relative index in the filter chain where the filter should be inserted. | ||
| message InsertPosition { |
There was a problem hiding this comment.
Instead of making a custom operation, why not use something well understood like JSON patch?
There was a problem hiding this comment.
Too complicated? Harder to write config?
|
So there appears to be two parts in the spec. One is "where to stick the change". Another one is "what to stick in there". The first one is proxy independent, and should be associated with a virtual service/service. The other one is envoy-specific, but can also be made generate with json patch, I think. |
| // example, the scope includes pods running in all reachable | ||
| // namespaces. Omitting the selector applies the filter to all proxies in | ||
| // the mesh. | ||
| map<string, string> selector = 1; |
There was a problem hiding this comment.
New K8s objects support set-based label selectors and I think we should too.
See the end of https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors
There was a problem hiding this comment.
This is purely istio only and trying to keep this independent of hte platforms.. Our label selection mechanism is quite rudimentary. Something like the above would require equivalent implementation in pilot
|
Also keep in mind that the more fancier we make this logic the more likely it is to skip 1.0 :). So let’s start simple and iterate over time. Also this Api is almost a break glass config. I have paid very little attention to aesthetics here. |
|
We don't want to incur debt unless we have to. Nothing against getting something simple. But this API makes a lot of assumptions about the structure of proxy config, so it needs to be done carefully. |
In fact, both of these are proxy dependent. The concepts of network & http filters and filter chains on listeners are all Envoy specific concepts. I'd prefer we just not kid ourselves about writing something that is portable to multiple dataplanes. Let's just accept that it is Envoy-specific, and get it out there. The filters themselves will be Envoy-specific in any case. |
|
@spikecurtis node selection, listener, inbound and outbound are generic. Otherwise the design goal here is to have smallest possible shim over xds. As for http lua specific use case, This satisfies lua requirement in a very unsatisfactory way, I can’t associate scripts with routes or virtual hosts. Ultimately I would like to associate scripts with virtual hosts/routes which by implication specify a listener. At present we only model outbound virtual hosts as virtual service. Whenever we model an inbound virtual host this association works either way. |
|
@mandarjog I think you already in trouble with lua in the current xDS configuration model. Virtual service == virtual host for HTTP traffic, for all intents and purposes. lua filter sits in the http connection manager, which is shared between many, possibly unrelated virtual hosts. So you cannot easily partition lua config between virtual hosts, unless we implement per_filter config for lua. Should we mandate per_filter_config for HTTP filters? Then we can express the config abstractly, with less coupling with how we chose to model xDS in istio. |
|
If we expose inserting lua scripts in routes then it is an implementation details as to how to make that happen. They way lua filter operate today with http-conn-man, we need to inject route level partitioning as part of deploying the script. Every route will have a unique id injected as per route metadata. The per route lua script will be called by the top level script only if the selected route meta data matches. Once envoy supports proper per route lua, (like Nginx and others) all this can go away. |
|
@mandarjog I think your idea of selecting scripts by route ID requires some understanding of lua. We can synthesize some lua code that checks route metadata first, but that only works for lua. The generic config dispatcher is per_filter_metadata in envoy, and that also requires some filter cooperation. I just don't see how this can be generalized to a random filter. |
|
recommend changing listener_proto to enum (users can still define their own enums). |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ostromart, rshriram The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Allows folks to ship their custom filter configs as part of regular Istio configs.
Example shown using Lua plugin which we would enable.
cc @ZackButcher @ostromart
Signed-off-by: Shriram Rajagopalan [email protected]