[cache filter] Add option to ignore cache-control directives from request #34090
Conversation
…uest Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/retest |
|
/lgtm api |
| @@ -46,16 +46,30 @@ enum class FilterState { | |||
| Destroyed | |||
| }; | |||
|
|
|||
| class CacheFilterConfig { | |||
There was a problem hiding this comment.
This would be simpler as a struct with public members, since it has no invariants
There was a problem hiding this comment.
It's probably going to end up with other things later, and also this way is more similar to the common filter config format.
There was a problem hiding this comment.
Specific example, I have a PR in the works that's going to add a shared pointer to a thundering herd manager structure in here.
There was a problem hiding this comment.
(Which probably could still just be a public struct member, and if I was the style maven here I would absolutely do that, but I'm trying to be conformant with how everything else is, and classes with accessors seems to be the standard for preprocessed filter configs.)
There was a problem hiding this comment.
"class" sounds good. (I looked at envoy classes named "*Config", and the ratio of "class" to "struct" is about 10:1, and consistency is a useful property.)
Commit Message: [cache filter] Add option to ignore cache-control directives from request
Additional Description: We noticed that, versus nginx cache, upstream was issuing many 304 responses. After some delving into what was happening, I tried commenting out the line that this change makes controllable, and the cache behaved more like our expectations. This PR makes it so we can have that sort of control via an option rather than via a patch.
Also addresses #12901 to make configuration more shared rather than initialized per-filter.
Risk Level: Low, it's a WIP filter and the behavior should be the same before and after unless the new option is set.
Testing: Added some.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a