CacheFilter: parses the allowed_vary_headers from the cache config. #12928
Conversation
Signed-off-by: Caio <caiomelo@google.com>
Signed-off-by: Caio <caiomelo@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Caio <caiomelo@google.com>
|
Can you make a C++ class for the allow-list rather than passing around a vector of matchers? Also is this a good time to introduce a config object so we don't have to reconstruct the allow structure on every request? |
|
Could you point me to a configuration example in an existing filter? I'm not sure how to proceed with that one. |
|
/lgtm api |
Signed-off-by: Caio <caiomelo@google.com>
|
@jmarantz thanks for the link! I'll look into how we can change the config of the plugin to be closer to gzip. |
There was a problem hiding this comment.
sorry for the delay -- I thought you might have been planning to do the conversion to the explicit config as part of this PR.
Mostly looks good; just some nits. Can you leave a TODO at least if you are not going to do this conversion to an explicit config in this PR?
/wait
| // Checks if the headers contain an allowed value in the Vary header. | ||
| static bool isAllowed(const std::vector<Matchers::StringMatcherPtr>& allowlist, | ||
| const Http::ResponseHeaderMap& headers); | ||
|
|
||
| // Checks if the headers contain a non-empty value in the Vary header. | ||
| static bool hasVary(const Http::ResponseHeaderMap& headers); |
There was a problem hiding this comment.
Should these static methods still be public? Or will they now be used only internal to the class as helpers?
There was a problem hiding this comment.
hasVary and createVaryKey are used by simple_http_cache, so those should stay public. I'll make the other two methods private.
There was a problem hiding this comment.
I moved the logic of parseAllowlist to the constructor and made parseHeaderValues more generic and moved it into CacheHeaderUtils.
Let me know if that's okay.
|
Sorry for the delay on my end too; I'm starting to look at the config just now because of the long weekend. |
Signed-off-by: Caio <caiomelo@google.com>
|
Hi, from the discussion in #12901, I feel like the explicit config should be a separate PR. What do you think? |
|
I was already working on it :-) |
|
@jmarantz gentle ping after merge and all tests passed. |
…code * upstream/master: lint: add more linters for using absl:: over std:: (envoyproxy#13043) udpa: filesystem list collection support for inline entries. (envoyproxy#13028) filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064) [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016) xds: allow empty delta update (envoyproxy#12699) CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928) router: extend HTTP CONNECT route matching criteria (envoyproxy#13056) docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051) build: shellcheck tools/ (envoyproxy#13007) [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017) Signed-off-by: Lihao Luo <lihao@squareup.com>
Commit Message: CacheFilter: parses the allowed_vary_headers from the cache config.
Signed-off-by: Caio caiomelo@google.com
Additional Description:
Parses the allowlist from the cache config proto; this allows users to define a set of rules to control which headers can be varied in the cache.
Risk Level: Low
Testing: Unit testing
Docs Changes: Updated cache proto's comments regarding
allowed_vary_headersRelease Notes: N/A
Fixes #10131