-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add HttpCache version 2 #41148
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
base: main
Are you sure you want to change the base?
Add HttpCache version 2 #41148
Conversation
Signed-off-by: Raven Black <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
For anyone who wasn't following along, #37990 became embroiled in an impossible conflict of "we can't have this because it adds some ugly API that we should remove later but we can't remove API fields from a non-WIP API" and "it can't work without either the ugly API or a huge risky core change to envoy". The agreed compromise was to make it a new filter, still tagged WIP, so the ugly API can be removed if the roadblocks are ever addressed. The old cache filter at this point has no maintainer sponsor (though I'm still on there for now), meaning it will eventually need to move into contrib or be removed entirely. |
Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
/coverage |
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/41148/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
… header validation consistent. Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Raven Black <[email protected]>
/retest |
The original #37990 was reviwed by @toddmgreer . I'll do a second pass . |
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.
LGTM for deps
Commit Message: Add HttpCache version 2
Additional Description: This is essentially just moving #37990 into a new namespace. In addition to simple renames, some factory functions were changed from using throw to returning StatusOr,
allowed_vary_headers
was hidden as not implemented, and the whole API was tagged WIP as the old version always should have been. There was also a side-effect that the protobuf stableHashKey value for a given cache entry changed because the proto message full-names changed; I bumped the file system cache version so that pre-existing cache entries will be invalidated.From #37990 the improved features of this new cache implementation are:
vary
headers are unsupported temporarily, rather than broken-supported as they were before.And the big dubious feature is:
Risk Level: Very low since it doesn't change existing APIs and only adds a WIP no-security-posture filter, don't use it!
Testing: Loads.
Docs Changes: Mostly-duplicate docs for the new version.
Release Notes: Follow shortly.
Platform Specific Features: No windows support for file system cache, same as before.