Introduce StringMatcher to the compressor - #46458
Conversation
GitHub Actions
Error
|
Hi @Vexali0n, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
d427da2 to
e85c957
Compare
GitHub Actions
Error
GitHub Actions
Error
…cher' Signed-off-by: Vexali0n <adam.richter218@gmail.com>
Signed-off-by: Vexali0n <adam.richter218@gmail.com>
3080c20 to
001674e
Compare
GitHub Actions
Inactive
KBaichoo
left a comment
KBaichoo
left a comment
There was a problem hiding this comment.
Thank you for working on this
/wait
| // When this field is specified, the default content type list is not used. | ||
| // Compression is applied when at least one configured matcher matches the | ||
| // response content-type header. | ||
| repeated type.matcher.v3.StringMatcher content_type_matcher = 4; |
There was a problem hiding this comment.
worth documenting what happens if both of content-type and content_type_matcher is used -- e.g. currently they get merged as per implementation
There was a problem hiding this comment.
proto updated to explain more in detail
| if (!content_type_matchers_.empty()) { | ||
| for (const auto& matcher : content_type_matchers_) { | ||
| if (matcher->match(value)) { | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Media types are case insensitive but the content_type_matcher here are case sensitive which is a bit of a mismatch. Consider lower casing the value and noting this in the proto that matches occur on lowercases values.
There was a problem hiding this comment.
Value is lower cased and mentioned in the proto that users should type in lower cased valuses for the content types.
| uint32_t minimumLength() const { return min_content_length_; } | ||
| bool isMinimumContentLength(const Http::RequestOrResponseHeaderMap& headers) const; | ||
| bool isContentTypeAllowed(const Http::RequestOrResponseHeaderMap& headers) const; | ||
| const std::vector<Matchers::StringMatcherPtr>& contentTypeMatchers() const { |
There was a problem hiding this comment.
seems like this isn't used?
There was a problem hiding this comment.
This is now dead but we end up now re-generating this in every loop below, can we instead normalize this once and pass it in below in an overloaded case?
There was a problem hiding this comment.
Overload has been added to not normalize value on each loop and on top of that I have added normalization content type helper function
| repeated string content_type = 3; | ||
|
|
||
| // Allows specifying which mime-types yield compression using | ||
| // envoy.type.matcher.v3.StringMatcher. |
There was a problem hiding this comment.
High-level API question: would it make sense to add a generic matcher extension here (instead of adding each of them individually)?
…alization helper function Signed-off-by: Vexali0n <adam.richter218@gmail.com>
GitHub Actions
Error
Signed-off-by: Vexali0n <adam.richter218@gmail.com>
GitHub Actions
Waiting
3 participants
Commit Message:
This PR adds new functionality of the compressor module to allow usage of StringMatcher, to allow more than just exact match for the content_type as it is at the moment.
Additional Description:
Current state of compressor allows only to use exact match for content types or it will fallback to the default list. There is no way how to specify wildcards to allow extreme amount of different content types as it is in our case. Generative AI has been used.
Risk Level:
Medium
Testing:
In addition to the written tests in the PR I have compiled the proxy locally, started up the proxy and tested against multiple envoy proxy configurations to prove envoy StringMatcher is supported and response is compressed while original logic of exact match and default list still function correctly.
Docs Changes:
New field content_type_matcher introduced for the compressor
Release Notes:
compressor: added support for
content_type_matcherinCommonDirectionConfig, allowing fine-grained MIME type matching via string matchers.[Optional Fixes #Issue]
Fixes #46344
FYI:
Original PR was #46374 where I was told to introduce StringMatcher into the compressor hence here is the new suggested solution.