New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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 |
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.
Thank you for this improvement!
@@ -46,16 +46,30 @@ enum class FilterState { | |||
Destroyed | |||
}; | |||
|
|||
class CacheFilterConfig { |
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.
This would be simpler as a struct with public members, since it has no invariants
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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