router: allow headers with multiple variables and static data #2409
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,206 @@ | ||
| #include "common/router/header_parser.h" | ||
|
|
||
| #include <ctype.h> | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/protobuf/utility.h" | ||
|
|
||
| #include "absl/strings/str_join.h" | ||
| #include "absl/strings/str_split.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Router { | ||
|
|
||
| namespace { | ||
|
|
||
| enum class ParserState { | ||
| Literal, // processing literal data | ||
| VariableName, // consuming a %VAR% name | ||
| ExpectArray, // expect starting [ in %VAR([...])% | ||
| ExpectString, // expect starting " in array of strings | ||
| String, // consuming an array element string | ||
| ExpectArrayDelimiterOrEnd, // expect array delimiter (,) or end of array (]) | ||
| ExpectArgsEnd, // expect closing ) in %VAR(...)% | ||
| ExpectVariableEnd // expect closing % in %VAR(...)% | ||
| }; | ||
|
|
||
| std::string unescape(absl::string_view sv) { return absl::StrJoin(absl::StrSplit(sv, "%%"), "%"); } | ||
|
Contributor
There was a problem hiding this comment. It's worth a comment at least why you didn't use absl::StrReplaceAll().
Member
Author
There was a problem hiding this comment. I'll switch to that function. It's not on their docs website (that I saw), so I didn't realize it existed. |
||
|
|
||
| // Implements a state machine to parse custom headers. Each character of the custom header format | ||
|
Contributor
There was a problem hiding this comment. very nice and readable code in general. I like this better than the regex. Also I think it can potentially be made more general, as described below. However I think this parser does deserve some rigorous unit tests :)
Member
Author
There was a problem hiding this comment. I added enough tests to exercise each case in the switch statement and the branches within them. Is there a particular part you'd like more test cases for? |
||
| // is either literal text (with % escaped as %%) or part of a %VAR% or %VAR(["args"])% expression. | ||
| // The statement machine does minimal validation of the arguments (if any) and does not know the | ||
| // names of valid variables. Interpretation of the variable name and arguments is delegated to | ||
| // RequestInfoHeaderFormatter. | ||
| HeaderFormatterPtr parseInternal(const envoy::api::v2::HeaderValueOption& header_value_option) { | ||
| const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(header_value_option, append, true); | ||
|
|
||
| absl::string_view format(header_value_option.header().value()); | ||
| if (format.empty()) { | ||
| return HeaderFormatterPtr{new PlainHeaderFormatter("", append)}; | ||
|
|
||
| } | ||
|
|
||
| std::vector<HeaderFormatterPtr> formatters; | ||
|
|
||
| size_t pos = 0, start = 0; | ||
| ParserState state = ParserState::Literal; | ||
| do { | ||
| const char ch = format[pos]; | ||
| const bool has_next_ch = (pos + 1) < format.size(); | ||
|
|
||
| switch (state) { | ||
| case ParserState::Literal: | ||
| // Searching for start of %VARIABLE% expression. | ||
| if (ch != '%') { | ||
| break; | ||
| } | ||
|
|
||
| if (!has_next_ch) { | ||
| throw EnvoyException( | ||
| fmt::format("Invalid header configuration. Un-escaped % at position {}", pos)); | ||
| } | ||
|
|
||
| if (format[pos + 1] == '%') { | ||
| // Escaped %, skip next character. | ||
| pos++; | ||
| break; | ||
| } | ||
|
|
||
| // Un-escaped %: start of variable name. Create a formatter for preceding characters, if | ||
| // any. | ||
| state = ParserState::VariableName; | ||
| if (pos > start) { | ||
| absl::string_view literal = format.substr(start, pos - start); | ||
| formatters.emplace_back(new PlainHeaderFormatter(unescape(literal), append)); | ||
| } | ||
| start = pos + 1; | ||
| break; | ||
|
|
||
| case ParserState::VariableName: | ||
| // Consume "VAR" from "%VAR%" or "%VAR(...)%" | ||
| if (ch == '%') { | ||
| // Found complete variable name, add formatter. | ||
| formatters.emplace_back( | ||
| new RequestInfoHeaderFormatter(format.substr(start, pos - start), append)); | ||
| start = pos + 1; | ||
| state = ParserState::Literal; | ||
| break; | ||
| } | ||
|
|
||
| if (ch == '(') { | ||
| // Variable with arguments, search for start of arg array. | ||
| state = ParserState::ExpectArray; | ||
|
Contributor
There was a problem hiding this comment. I was expecting you to bump a depth-count here. Scanning forward a bit, it looks like the algorithm is very sensitive to a particular json structure. I think it might actually be just as easy (and maybe very few if any additional lines of code) to have your scanner track the paren depth for Perhaps this is speculative generality but I also wonder if the json structure you expect might change over time requiring the next person to then update this routine.
Member
Author
There was a problem hiding this comment. The history of using JSON here is that when custom headers only supported the entire header value being custom (vs. what this PR is doing) we thought it would be easier to just use the JSON parser to handle arguments rather than writing an argument parser. In retrospect, I wish I had just handled arguments. FWIW, I'd be willing to toss the JSON business and rewrite this parser to just take quoted arguments (e.g. The JSON spec doesn't seem to allow single-quoted strings, so I don't think we need to handle them in any event.
Contributor
There was a problem hiding this comment. This comment spawned a bunch of thought from me, but not that much typing. As I'm relatively new to Envoy I still don't have a great sense of what needs to be fast (e.g. runs per request) and what can't be changed (because it's already released and documented). If this runs per request, I suspect it'd be worth considering dumping JSON parsing and just scan using string_view-related routines. If this only runs at startup, I'm a little less concerned, however I'm also looking at istio/istio#2554 (comment) and #2373 which are all around startup speed. That doesn't mean this would be a problem but I think it's worth understanding it. The second part of this: is the syntax you need to parse set in stone? If so, and JSON generality is not needed, it'd be nice to drop it in favor of something that could be more easily and quickly parsed. For the moment I'll continue the review assuming the answers are "the speed does not matter" and "the format cannot be changed".
Member
Author
There was a problem hiding this comment. Parsing these strings only happens only at configuration time. The syntax needn't be set in stone, but there's also some user friction related to changing this stuff. It looks to me that all this was added after 1.5 and 1.6 isn't out yet, so if we were going to change it, now is the time. @mattklein123 what do you think?
Member
There was a problem hiding this comment. I don't have a strong opinion. This seems fine to me. What's the proposed alternative?
Member
Author
There was a problem hiding this comment. Right now the parser does just enough parsing to find the start/end of % expressions, but needs to know something about JSON to do it. We could just make the parser smart enough to parse the params and ditch the JSON parser. While we're there we could drop the square brackets. If you want to do this I'll put in another PR. |
||
| } | ||
| break; | ||
|
|
||
| case ParserState::ExpectArray: | ||
| // Skip over whitespace searching for the start of JSON array args. | ||
| if (ch == '[') { | ||
| // Search for first argument string | ||
| state = ParserState::ExpectString; | ||
| } else if (!isspace(ch)) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Expecting JSON array of arguments after '{}', but " | ||
| "found '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)), ch)); | ||
| } | ||
| break; | ||
|
|
||
| case ParserState::ExpectArrayDelimiterOrEnd: | ||
| // Skip over whitespace searching for a comma or close bracket. | ||
| if (ch == ',') { | ||
| state = ParserState::ExpectString; | ||
| } else if (ch == ']') { | ||
| state = ParserState::ExpectArgsEnd; | ||
| } else if (!isspace(ch)) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Expecting ',', ']', or whitespace after '{}', but " | ||
| "found '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)), ch)); | ||
| } | ||
| break; | ||
|
|
||
| case ParserState::ExpectString: | ||
| // Skip over whitespace looking for the starting quote of a JSON string. | ||
| if (ch == '"') { | ||
| state = ParserState::String; | ||
| } else if (!isspace(ch)) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Expecting '\"' or whitespace after '{}', but found '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)), ch)); | ||
| } | ||
| break; | ||
|
|
||
| case ParserState::String: | ||
| // Consume a JSON string (ignoring backslash-escaped chars). | ||
| if (ch == '\\') { | ||
| if (!has_next_ch) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Un-terminated backslash in JSON string after '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)))); | ||
| } | ||
|
|
||
| // Skip escaped char. | ||
| pos++; | ||
| } else if (ch == '"') { | ||
| state = ParserState::ExpectArrayDelimiterOrEnd; | ||
| } | ||
| break; | ||
|
|
||
| case ParserState::ExpectArgsEnd: | ||
| // Search for the closing paren of a %VAR(...)% expression. | ||
| if (ch == ')') { | ||
| state = ParserState::ExpectVariableEnd; | ||
| } else if (!isspace(ch)) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Expecting ')' or whitespace after '{}', but found '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)), ch)); | ||
| } | ||
| break; | ||
|
|
||
| case ParserState::ExpectVariableEnd: | ||
| // Search for closing % of a %VAR(...)% expression | ||
| if (ch == '%') { | ||
| formatters.emplace_back( | ||
| new RequestInfoHeaderFormatter(format.substr(start, pos - start), append)); | ||
| start = pos + 1; | ||
| state = ParserState::Literal; | ||
| break; | ||
| } | ||
|
|
||
| if (!isspace(ch)) { | ||
| throw EnvoyException(fmt::format( | ||
| "Invalid header configuration. Expecting '%' or whitespace after '{}', but found '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)), ch)); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| NOT_REACHED; | ||
| } | ||
| } while (++pos < format.size()); | ||
|
|
||
| if (state != ParserState::Literal) { | ||
| // Parsing terminated mid-variable. | ||
| throw EnvoyException( | ||
| fmt::format("Invalid header configuration. Un-terminated variable expression '{}'", | ||
| absl::StrCat(format.substr(start, pos - start)))); | ||
| } | ||
|
|
||
| if (pos > start) { | ||
| // Trailing constant data. | ||
| absl::string_view literal = format.substr(start, pos - start); | ||
| formatters.emplace_back(new PlainHeaderFormatter(unescape(literal), append)); | ||
| } | ||
|
|
||
| ASSERT(formatters.size() > 0); | ||
|
|
||
| if (formatters.size() == 1) { | ||
| return std::move(formatters[0]); | ||
| } | ||
|
|
||
| return HeaderFormatterPtr{new CompoundHeaderFormatter(std::move(formatters), append)}; | ||
|
|
||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
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.
You are just trying to convert a string_view to a std::string? Use std::string(params)