Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sketch of an extension manifest #1221
sketch of an extension manifest #1221
Changes from all commits
2d9e2cd
5300000
a9ca37e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming nit: maybe "ExtensionConfig"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we consider making this semver instead of regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more context - back when we made this in EnvoyFilter we had versions like master-latest-daily. We now call these
1.5-dev.SHA
for example (technically its not semver without 3 numbers but close enough to work) so it should workThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a requests in the original envoyfilter API as well, but we did not have correct proxy version that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semver in UDPA is just a numeric version. Don't we need to handle ASM qualifier in addition to semver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that may be an issue, may want to select on
1.4.2-custom
vs1.4.2
. Or we could say that is overloading the version and should just set another metadata field and use that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Envoy
BuildVersion
in envoyproxy/envoy#9301 includes metadata capable of expressing these labels.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISTIO_VERSION is part of node metadata, so we would be regex matching on a field in metadata. We could express it as a map from a field to a string matcher, but that feels too level in istio, since it does not convey the purpose of using the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially you need the version to check capabilities of the proxy itself (will it accept the extension or not). What is the reason not to rely on BuildVersion, which would make Istio more UDPA compliant, vs Istio-specific API such as using ISTIO_VERSION metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have any reason other than that build version doesn't exist yet as far as I can tell? We would likely need to fallback to istio_version for old proxies even when it does though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with the match criteria enabled by envoyproxy/envoy#9301? Specifically the ability to match on the presence of a given filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need that match criterion in istio. We generally know what filters are packaged, and don't select on presence or absence of compiled filters. In the future, we expect filters to be dynamically loadable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a gateway can have many Servers with its own hosts. Each of which ends up in its own filter chain if using TLS. So you need a way to specify the hostnames here as well, so that you can match a filter chain based on the sni.
The match as it stands is insufficient as people have most commonly wanted to add a lua filter only for certain paths and not for all routes (like /user/.. and not for /index.html). I have seen this way too many times to keep track. This then requires route.PerFilterconfig in envoy. You cant do that with the way you have expressed the matches as it stand today. You would also now need to match on a specific route by name so that you can add this config to the perFilterConfig.
Finally, since we are in the land of designing for future, we also need to take into account the passthrough listeners. People will want to write loggers/trackers/enforces for the traffic that is passthrough while using standard rbac for the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't per filter config be part of virtual service or other service/route-specific resource? This API is written from an administrator point of view (MUST enable telemetry and authorization) so opting out should be an override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between NETWORK_FILTER and UPSTREAM_NETWORK_FILTER?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NETWORK_FILTER is a downstream network filter, UPSTREAM_NETWORK_FILTER is an upstream network filter a.k.a cluster filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the mechanism for two independent providers of filters to coordinate relative order?
I.e. I have filters A and B, is there a way to specify that B should go before A?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to allow specific total order of filters. A partial order constraint like this enum is all we want to support. That is we can express that authorization filters should come before post-processing filters, but no specific order between authorization filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that. Having at least a relative ordering within each category might help with inter-dependant filters. I know it sounds like a long shot at this point but limitations like this also cause problems with more complex setups of e.g. MutatingWebhooks or CNI plugins - as for CNI, we've hit this problem in Istio even.
Could be solved with a simple
priority
integer, where if you have filters 1,2 withtype==Authorization
andpriority(1) > priority(2)
, filter 1 is guaranteed to be injected earlier in the filter chain.priority(1) == priority(2)
could be undefined order, caught using our ValidatingWebhook.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no impl under way yet. We are still in the process of getting agreement.
Re: ordering, integers priority helps, but what is easier is to preserve the manifest order in the actual output within a filter type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, yeah I remember now that this was talked about in the last meeting. I'm not sure that's very user-friendly- it's non-obvious that this implicit order matters, as the filters could be matching on completely separate sidecars. It also leaves out how this would behave if different
ExtensionManifests
add filters to the same Envoy config. Which one wins?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch : I'm curious to hear your thoughts on this API sketch to globally apply a filter across xDS configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would also be valuable to hear from @alexburnos here, as he has experience with similar systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it is not a UDPA Filter name field (as it has uniqueness requirement)? In this case, where is UDPA filter name will be specified, in the Extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working to change UDPA filter name to be free-form in envoy. UDPA filter type URL is sufficient to determine an extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supporting both ref and inline means that k8s rbac cannot be used. We need to use a more sophisitcated admission control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an Istio API expert, but this strikes me as providing an imperative vs. a declarative API. I was under the impression we tend to describe what the system should be like, rather than how to get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is debatable. We have a choice here.
We can forgo these operations, and then leave “how to get there” to CI/CD
In that case you install a whole new named manifest in your namespace, so someone else must do copy and update operation outside the istio API.
Please take a look at the linked doc where we have listed the use cases this needs to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch We really only need the removal operation since there is no other way to provide an override for a filter that disables it. Think of a per-namespace declaration that disables a root default. .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner (and very simple) to add a per-filter disable override in Envoy. We've discussed this before and it just needs O(1 day) of effort to add if someone is inclined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a bool inside the filter proto, it would mean merging the default proto with the override. We want to avoid proto merges.
a “conditional” dispatch of filters is a useful feature, and we have an open feature request in envoy for that.
However wouldn’t it be better to not send a filter configuration at all instead of sending it with a disabled flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch Are you thinking of adding a bool field to filter config protos to disable it? I think that might be useful with filter chain discovery, to be able to dynamically disable filters. If so, we could mirror it here with a bool field instead of operation enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name:
filter_phase