Skip to content
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

api: GoogleRE2 max_program_size should be checked by server, not client. #10971

Merged
merged 12 commits into from
May 3, 2020
Merged

api: GoogleRE2 max_program_size should be checked by server, not client. #10971

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8803614
api: GoogleRE2 max_program_size should be checked by server, not client.
markdroth Apr 27, 2020
5005314
add deprecation to release notes
markdroth Apr 27, 2020
0be0ab1
fix wording
markdroth Apr 28, 2020
2e8e4c0
fix docs
markdroth Apr 28, 2020
7479875
attempt to fix doc link
markdroth Apr 28, 2020
38c74fc
fix_format
markdroth Apr 28, 2020
b9162d8
try to make CI happy
markdroth Apr 28, 2020
2d3dd0e
Merge remote-tracking branch 'upstream/master' into regex_max_program…
markdroth Apr 28, 2020
3fdd681
Merge remote-tracking branch 'upstream/master' into regex_max_program…
markdroth Apr 30, 2020
cf6cb5f
make api CI happy
markdroth Apr 30, 2020
99bc27f
fix_format
markdroth Apr 30, 2020
512d9ea
Merge remote-tracking branch 'upstream/master' into regex_max_program…
markdroth May 1, 2020
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
5 changes: 4 additions & 1 deletion api/envoy/type/matcher/regex.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ message RegexMatcher {
// compiled regex is to evaluate. A regex that has a program size greater than the configured
// value will fail to compile. In this case, the configured max program size can be increased
// or the regex can be simplified. If not specified, the default is 100.
google.protobuf.UInt32Value max_program_size = 1;
//
// This field is deprecated; regexp validation should be performed on the management server
// instead of being done by each individual client.
google.protobuf.UInt32Value max_program_size = 1 [deprecated = true];
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of leaving this in, but providing a reasonable default max_program_size that is bounded, but can be overridden?

Copy link
Contributor

Choose a reason for hiding this comment

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

In what way is that different from the current field? If max_program_size is "not specified, the default is 100."

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm getting at what @markdroth is trying to express: this is an API that is intended to be implementable by proxies other than Envoy.

I think maybe he objects to predefining what the default behavior is, and rely solely on the management plane to enforce limits, which concerns me because I am thinking from the perspective of managing a shared-tenancy proxy, and I want the default scenario to consume bounded CPU.

I think maybe we could change the comment from saying "if not specified, the default is 100." to "if not specified, the default limit is implementation-dependent".

Mark, would that be good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think changing the default here would address my concern. The real issue is that we need to be validating this on the management server, not on the client.

}

oneof engine_type {
Expand Down
3 changes: 3 additions & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,6 @@ Deprecated
* Tracing provider configuration as part of :ref:`bootstrap config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.tracing>`
has been deprecated in favor of configuration as part of :ref:`HTTP connection manager
<envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing.provider>`.
* The * :ref:`GoogleRE2.max_program_size<envoy_api_field_RegexMatcher.GoogleRE2.max_program_size>`
field is now deprecated. Management servers are expected to validate regexp program sizes
instead of asking the client to do it.
Copy link
Contributor

Choose a reason for hiding this comment

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

here 'client' means the proxy, that is, it's a client of the control-plane?

What if the user is deploying Envoy without a control-plane? Maybe that never happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, "client" means the xDS client (i.e., Envoy or gRPC).

If they're deploying Envoy with a static config, they can statically validate the regexps.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/asking/expecting/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.