router/stats: handle invalid regexes in config#2420
router/stats: handle invalid regexes in config#2420zuercher merged 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
mattklein123
left a comment
There was a problem hiding this comment.
Awesome thanks, tiny nit.
source/common/common/utility.cc
Outdated
| std::regex RegexUtil::parseRegex(const std::string& regex, std::regex::flag_type flags) { | ||
| try { | ||
| return std::regex(regex, flags); | ||
| } catch (std::regex_error& e) { |
| } | ||
|
|
||
| std::regex RegexUtil::parseRegex(const std::string& regex, std::regex::flag_type flags) { | ||
| try { |
There was a problem hiding this comment.
Nit: I think ideally this would be caught by PGV (https://github.com/lyft/protoc-gen-validate) annotations. The language allows one to express regex validity (e.g. https://github.com/lyft/protoc-gen-validate/blob/6cd364ac742e088e22c2d04e90358ce48e1e33e5/validate/validate.proto#L478).
However, this is not yet implemented for C++, and what form the regexes checking should take is unclear (do we really want RE2?).
There was a problem hiding this comment.
It's not clear to me that one can write a regular expression to validate that a string is a well-formed regular expression (given arbitrarily nested groups and depending on the library in use). But perhaps PGV will add a well-known rule for regular expressions in the future. I'll add a TODO.
There was a problem hiding this comment.
@zuercher I like that idea. Right now (since it's written in Go), it expects RE2 syntax, would need to do some work to support other formats.
There was a problem hiding this comment.
Umm, yes, I think I may have linked to the wrong thing. The idea is not to validate a regex with a regex, but to add support in PGV to validate that REs are valid.
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo nit. Maybe add a TODO explaining the PGV connection if you are making any further edit.
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
@zuercher FWIW I've seen OSX flaking more often lately. Not sure if it's worth taking a look into. |
|
Filed #2428 |
Fix isCleartextTrafficPermitted by finding a static method (instead of an object method) and calling the static method of the AndroidNetworkLibrary class instead of calling the instance method on a string. Signed-off-by: Ryan Hamilton <rch@google.com> Signed-off-by: JP Simard <jp@jpsim.com>
Fix isCleartextTrafficPermitted by finding a static method (instead of an object method) and calling the static method of the AndroidNetworkLibrary class instead of calling the instance method on a string. Signed-off-by: Ryan Hamilton <rch@google.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description:
The Envoy API accepts regular expressions for several configuration options. If an invalid regular expression is provided, STL throws
std::regex_error, which is uncaught and causes the sudden termination of Envoy. Replace use of thestd::regex(std::string)constructor with calls to a new utilityRegexUtil::parseRegex()which converts thestd::regex_errorinto anEnvoyException, allowing the invalid configuration to be handled gracefully.Risk Level: Low
Testing:
Unit tests for behavior or new utility function and testing of invalid regular expressions in configuration.
Release Notes: N/A
Fixes: #2406
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io