Skip to content

api: add field to allow clients to ignore unknown HTTP filters #14982

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Feb 11, 2021
Merged

api: add field to allow clients to ignore unknown HTTP filters #14982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28bae07
api: add field to allow clients to ignore unknown HTTP filters
markdroth Feb 8, 2021
de2a1b0
fix_format
markdroth Feb 8, 2021
3c580fc
comment out comments that are causing doc reference failures
markdroth Feb 8, 2021
a59a3f8
fix_format (again)
markdroth Feb 8, 2021
d20c8a5
Merge remote-tracking branch 'upstream/main' into xds_http_filters_op…
markdroth Feb 8, 2021
ad5e787
switch to more general-purpose wrapper for typed_per_filter_config
markdroth Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions api/envoy/config/route/v3/route_components.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ message VirtualHost {
// *envoy.filters.http.buffer* for the HTTP buffer filter. Use of this field is filter
// specific; see the :ref:`HTTP filter documentation <config_http_filters>`
// for if and how it is utilized.
// [#comment: An entry's value may be wrapped in a
// :ref:`FilterConfig<envoy_api_msg_config.route.v3.FilterConfig>`
// message to specify additional options.]
map<string, google.protobuf.Any> typed_per_filter_config = 15;

// Decides whether the :ref:`x-envoy-attempt-count
Expand Down Expand Up @@ -248,6 +251,9 @@ message Route {
// *envoy.filters.http.buffer* for the HTTP buffer filter. Use of this field is filter
// specific; see the :ref:`HTTP filter documentation <config_http_filters>` for
// if and how it is utilized.
// [#comment: An entry's value may be wrapped in a
// :ref:`FilterConfig<envoy_api_msg_config.route.v3.FilterConfig>`
// message to specify additional options.]
map<string, google.protobuf.Any> typed_per_filter_config = 13;

// Specifies a set of headers that will be added to requests matching this
Expand Down Expand Up @@ -362,6 +368,9 @@ message WeightedCluster {
// *envoy.filters.http.buffer* for the HTTP buffer filter. Use of this field is filter
// specific; see the :ref:`HTTP filter documentation <config_http_filters>`
// for if and how it is utilized.
// [#comment: An entry's value may be wrapped in a
// :ref:`FilterConfig<envoy_api_msg_config.route.v3.FilterConfig>`
// message to specify additional options.]
map<string, google.protobuf.Any> typed_per_filter_config = 10;
}

Expand Down Expand Up @@ -1944,3 +1953,20 @@ message InternalRedirectPolicy {
// x-forwarded-proto. The default is false.
bool allow_cross_scheme_redirect = 4;
}

// A simple wrapper for an HTTP filter config. This is intended to be used as a wrapper for the
// map value in
// :ref:`VirtualHost.typed_per_filter_config<envoy_api_field_config.route.v3.VirtualHost.typed_per_filter_config>`,
// :ref:`Route.typed_per_filter_config<envoy_api_field_config.route.v3.Route.typed_per_filter_config>`,
// or :ref:`WeightedCluster.ClusterWeight.typed_per_filter_config<envoy_api_field_config.route.v3.WeightedCluster.ClusterWeight.typed_per_filter_config>`
// to add additional flags to the filter.
// [#not-implemented-hide:]
message FilterConfig {
// The filter config.
google.protobuf.Any config = 1;

// If true, the filter is optional, meaning that if the client does
// not support the specified filter, it may ignore the map entry rather
// than rejecting the config.
bool is_optional = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is unavoidable given the nature of xDS resource independence.

28 changes: 28 additions & 0 deletions api/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion ...envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ message ScopedRds {
[(validate.rules).message = {required: true}];
}

// [#next-free-field: 6]
// [#next-free-field: 7]
message HttpFilter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpFilter";
Expand All @@ -840,6 +840,12 @@ message HttpFilter {
// Extension configs delivered through this mechanism are not expected to require warming (see https://github.com/envoyproxy/envoy/issues/12061).
config.core.v3.ExtensionConfigSource config_discovery = 5;
}

// If true, clients that do not support this filter may ignore the
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

message RequestIDExtension {
Expand Down
8 changes: 7 additions & 1 deletion .../extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions generated_api_shadow/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ proto_library(
"//envoy/extensions/common/matching/v3:pkg",
"//envoy/extensions/common/ratelimit/v3:pkg",
"//envoy/extensions/common/tap/v3:pkg",
"//envoy/extensions/compression/brotli/compressor/v3:pkg",
"//envoy/extensions/compression/brotli/decompressor/v3:pkg",
"//envoy/extensions/compression/gzip/compressor/v3:pkg",
"//envoy/extensions/compression/gzip/decompressor/v3:pkg",
"//envoy/extensions/filters/common/dependency/v3:pkg",
Expand Down
26 changes: 26 additions & 0 deletions generated_api_shadow/envoy/config/route/v3/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions generated_api_shadow/envoy/config/route/v4alpha/route_components.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion ...envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion .../extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.