api: add field to allow clients to ignore unknown HTTP filters #14982
Conversation
Signed-off-by: Mark D. Roth <roth@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
| @@ -816,7 +816,7 @@ message ScopedRds { | |||
| [(validate.rules).message = {required: true}]; | |||
| } | |||
|
|
|||
| // [#next-free-field: 6] | |||
| // [#next-free-field: 7] | |||
| message HttpFilter { | |||
There was a problem hiding this comment.
Not specific to gRPC: Should we also add this "optional" functionality to upstream filters in cluster? Then if one day Envoy decided to implement this feature, they will have all proto changes needed.
There was a problem hiding this comment.
In general, I think it would be useful to do this for all types of filters (also including L4 filters and listener filters). But right now, we are primarily concerned about HTTP filters, so let's focus this PR on that functionality.
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
…tional Signed-off-by: Mark D. Roth <roth@google.com>
| // [#not-implemented-hide:] | ||
| message OptionalFilterConfig { | ||
| google.protobuf.Any config = 1; | ||
| } |
There was a problem hiding this comment.
Is this wrapper needed? Setting is_optional=true ought to disable parsing of config for filters that are optional and not build into the binary. It should be possible to ignore this config without wrapping it in an extra layer of messages.
There was a problem hiding this comment.
The problem is that at the time that we receive the RouteConfiguration from RDS, we don't necessarily know which HCM config it's going to be matched up with, because there could be a new LDS response waiting to be swapped in, so we can't depend on the is_optional=true that was set there. See my comment in #12274 for details.
There was a problem hiding this comment.
Yeah, I think this is unavoidable given the nature of xDS resource independence.
| // filter but otherwise accept the config. | ||
| // Otherwise, clients that do not support this filter must reject the config. | ||
| // [#not-implemented-hide:] | ||
| bool is_optional = 6; |
There was a problem hiding this comment.
What does the rest of the implementation look like? Is there a natural place to ignore the config for optional filters that are not built into the binary?
There was a problem hiding this comment.
@htuch said this probably wouldn't be hard to implement.
I'm guessing that we'll do it when validating the LDS response. Presumably, that's the point at which we're checking that we have a filter registered for the type of the config in the typed_config field, and we'll NACK the config if we don't have one. The purpose of the is_optional field is to have that check ignore the filter instead of NACKing the config.
| // to indicate that the filter is optional. In this case, if the client does not support the filter, | ||
| // it may ignore the map entry rather than rejecting the config. | ||
| // [#not-implemented-hide:] | ||
| message OptionalFilterConfig { |
There was a problem hiding this comment.
A downside of this wrapper is that it adds exactly one feature, and leaves no room for future expansion. If we need another feature in the future, we'll need two levels of wrapping. (Actually, we already do have another level of wrapping in the TypedStruct feature.)
Instead of this limited-purpose wrapper, maybe we should have one that holds all future expandability instead?
message FilterConfig {
oneof config_type {
proto.Any typed_config = 1; // Would never contain a TypedStruct
udpa.type.v1.TypedStruct struct_config = 2;
config.core.v3.ExtensionConfigSource config_discovery = 3; // ? (from HttpFilter)
}
bool optional = 4;
// room for future expansion
}Also, optionally & more invasively, we could update HttpFilter to deprecate config_type and reference this new FilterConfig message to unify things further.
There was a problem hiding this comment.
I like the idea of making this more general-purpose so that we can add additional per-filter options later! Done.
I didn't include the TypedStruct field, because I think the established way of dealing with TypedStruct in xDS is to embed it inside of an Any field, since that makes the JSON representation more natural.
I also didn't include the ExtensionConfigSource field, since I don't see any existing option for using ECDS here in the typed_per_filter_config fields. I suspect the two are mutually exclusive, but @htuch can confirm.
Signed-off-by: Mark D. Roth <roth@google.com>
|
LGTM. @lizan can you provide a non-Google API review here, want to sanity check we're not over-fitting to Google needs? |
Signed-off-by: Mark D. Roth roth@google.com
Commit Message: api: add field to allow clients to ignore unknown HTTP filters
Additional Description: This allows control planes to tell clients that a given HTTP filter is okay to ignore if it is not supported. This is useful in environments where the clients are not centrally controlled and therefore cannot be upgraded at will, where it is desirable to start using a new filter for clients that do support the new filter without breaking older clients. Unlike a client-capability-based approach, where the client tells the server which filters it supports, this avoids cache pollution and resource-name-based complexity on the server. And because the control plane can set this on a per-filter basis, this approach does not impose risk that clients will silently fail to apply filters that provide mandatory functionality (e.g., authz policies).
Risk Level: Low
Testing: N/A (actual functionality will be implemented in a subsequent PR)
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A
CC @htuch @ejona86 @dfawley @lidizheng @dapengzhang0