Compressor content-type wildcard support - #46374
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 |
GitHub Actions
Error
|
/lgtm api |
| for (const auto& type : types) { | ||
| std::string trimmed = std::string(StringUtil::trim(type)); | ||
| if (trimmed.empty()) { | ||
| continue; | ||
| } | ||
| if (trimmed == "*" || trimmed == "*/*") { | ||
| has_global_wildcard = true; | ||
| } else if (absl::EndsWith(trimmed, "/*")) { | ||
| std::string prefix = std::string(absl::StripSuffix(trimmed, "*")); | ||
| prefix_patterns.push_back(absl::AsciiStrToLower(prefix)); | ||
| } else { | ||
| exact_set.insert(absl::AsciiStrToLower(trimmed)); | ||
| } | ||
| } |
There was a problem hiding this comment.
FWIW, this is a behavior change. For example, in the previous implementation, custom/* will be treated as an exact content type and in the new implementation, this will be treated as an prefix.
I will inclined to add a repeated type.matcher.v3.StringMatcher content_type_matcher rather than changing the meaning of existing API.
There was a problem hiding this comment.
Thanks for the feedback. You are right I haven't thought that somebody would actually configure in their configs wildcard while using exact match.
I am gonna rewrite it to still fully support old behavior the way you have mentioned.
GitHub Actions
Error
4 participants
Commit Message:
This PR enhances current functionality of the compressor module to allow wildcards for the content type list.
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 wildcards are supported and response is compressed while original logic of exact match and default list still function correctly.
Docs Changes:
Only 1 comment inside the compressor.proto file
Release Notes:
compressor: added support for wildcards (such as
text/*or*) in the compressor filter'scontent_typeconfiguration to allow matching MIME type prefixes or all content types.[Optional Fixes #Issue]
Fixes #46344