caching: Validate stale cache entries and update cached headers #12232
Conversation
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
…stale cache entries if needed Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
…rrectly, not tested yet Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
…that were discovered by tests, and minor refactoring to the CacheFilter Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
…r functions preconditions in the CacheFilter Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
This PR is a little bit lacking in tests, but right now I am working on a separate PR to improve tests and coverage for the whole cache tree. The CacheFilter is still WIP. |
| Http::LowerCaseString key(std::string(cached_header.key().getStringView())); | ||
| absl::string_view value = cached_header.value().getStringView(); | ||
| if (!response_headers->get(key)) { | ||
| response_headers->setCopy(key, value); |
There was a problem hiding this comment.
The cached header key is copied twice here, once when creating Http::LowerCaseString key and once inside setCopy. I am open to suggestions to avoid this.
The get function accepts a LowerCaseString, so I cannot pass the cached_header.key() directly, and there is no setViaMove to move the key instead of copying it.
Perhaps I am wrong in any of this, looking forward to any suggestions.
There was a problem hiding this comment.
It might be as good as it can be, depending on the limitations you have. If you have to use setCopy, then you have to copy into the map. Any optimization would need the single write to be in there. But, you first need to check for the (lowercased) key before you do that, so you're stuck.
However, what's the lifetime of cached_header? Or more broadly lookup_result_->headers_? If it outlives response_headers, then you're free to use setReference().
(Also, although this is a wild tangent and not appropriate to address in this PR, I wonder if LowerCaseString could be changed to accept "I promise this is already lower". Since it's an attempt to never let non-lowercasing slip in, maybe that's not a good idea, but... it seems like you really could be sure that a header you previously cached would be well behaved.)
There was a problem hiding this comment.
cached_header is a member of a member of the CacheFilter, so it only outlives the response if the filter does. However, I think it is dangerous to rely on that.
Another thought I had is to use addViaMove and pass to it cached_header.key() and cached_header.value() as the lookup_result_ will not be used any more. Two problems prevent that:
- The only version of
iterateI found is a constant version. So I cannot pass these parameters tocached_header.key(). - I still have to create a
LowerCaseStringto check if the header already exists.
There was a problem hiding this comment.
This part was moved to processSuccessfulValidation function.
| [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailers(std::move(trailers)); }); | ||
| } | ||
| lookup_result_ = std::make_unique<LookupResult>(std::move(result)); | ||
| encodeCachedResponse(); |
There was a problem hiding this comment.
The code removed from case CacheEntryStatus::Ok: is refactored into encodeCachedResponse() with some modifications so that it works for encoding cached responses in both decoding and encoding scenarios.
decoding scenario -> fresh cache hit found when decoding
encoding scenario -> stale cache hit found when decoding, successfully validated when encoding
| } else if (response_has_trailers_) { | ||
| getTrailers(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function is refactored out of onHeaders, see the comment I left there.
…r of pointers as containers of consts caused an error on Windows Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
cc @toddmgreer (extension co-owner) |
| Http::LowerCaseString key(std::string(cached_header.key().getStringView())); | ||
| absl::string_view value = cached_header.value().getStringView(); | ||
| if (!response_headers->get(key)) { | ||
| response_headers->setCopy(key, value); |
There was a problem hiding this comment.
It might be as good as it can be, depending on the limitations you have. If you have to use setCopy, then you have to copy into the map. Any optimization would need the single write to be in there. But, you first need to check for the (lowercased) key before you do that, so you're stuck.
However, what's the lifetime of cached_header? Or more broadly lookup_result_->headers_? If it outlives response_headers, then you're free to use setReference().
(Also, although this is a wild tangent and not appropriate to address in this PR, I wonder if LowerCaseString could be changed to accept "I promise this is already lower". Since it's an attempt to never let non-lowercasing slip in, maybe that's not a good idea, but... it seems like you really could be sure that a header you previously cached would be well behaved.)
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc
Show resolved
Hide resolved
…ed is invalid or non-existent Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@toddmgreer I think this is ready for you to take a look. Josiah and Fred took passes so hopefully this shouldn't take too long. |
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
…nd cleaner Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@toddmgreer I managed to rewrite the state management in the filter in a simpler and cleaner way. Take a look whenever you can! |
|
@dio I merged master again, hopefully this will fix it |
|
The clang tidy error still exists. All the checks used to pass before merging master and the nit commit (moving the include). I don't know what might be the problem. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@yosrym93, sorry, seems like you need to have #12449 as well. Currently, yours is still: https://github.com/envoyproxy/envoy/blob/634f21e6b3d551a258d669283fa8a94a70681574/ci/run_clang_tidy.sh (#12426). |
|
Hello, sorry for the delay, I was pulled into working on customer case. |
According to: https://tools.ietf.org/html/rfc7234#section-4.3.3 304 responses (not modified) require special handling as they mean we should update and serve the cached response that is being validated. |
|
Still no luck with the clang-tidy problem, @dio thoughts? |
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
There was a problem hiding this comment.
@yosrym93 thank you for your patience! Since you need to merge main, I'd like to take this opportunity to suggest some nitpiciking.
Furthermore, I think @yanavlasov's comment is worth to be addressed by adding comments near to the relevant lines?
Thanks!
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
|
@dio done! |
|
Do I need to do anything about the cherry-picked fix now? |
|
@lizan do you want to sign this as a senior maintainer? Thanks! |
|
@lizan is there anything this PR still needs to be merged? |
| const LowerCaseString IfMatch{"if-match"}; | ||
| const LowerCaseString IfNoneMatch{"if-none-match"}; | ||
| const LowerCaseString IfModifiedSince{"if-modified-since"}; | ||
| const LowerCaseString IfUnmodifiedSince{"if-unmodified-since"}; | ||
| const LowerCaseString IfRange{"if-range"}; | ||
| const LowerCaseString LastModified{"last-modified"}; |
There was a problem hiding this comment.
Shall we register them as static inline headers instead of using get through out the implementation?
There was a problem hiding this comment.
I could add a inline_headers_handles.h file to the cache tree that registers all the used headers in this module (using Http::RegisterCustomInlineHeader), to avoid registering them in every file that uses them. What do you think?
There was a problem hiding this comment.
But I guess this won't work without making the handles static which is against the style guide.
There was a problem hiding this comment.
There is also no setReferenceKeyInline to replace setReferenceKey(Http::CustomHeaders::get().IfModifiedSince, date).
There was a problem hiding this comment.
Can you elaborate further on what you are suggesting as a follow up?
Commit Message: CacheFilter now validates cache stale entries and updates the cached headers.
Additional Description:
When a stale cache hit is found for a request, conditional headers are injected to the request for validation. If the a 304 response is received (not modified), a response is constructed from cache, and cached headers are updated. If a request already includes conditional headers it bypasses the CacheFilter, this is valid behavior and satisfies the minimum of #9855 for production -- however, it can lead to missed caching opportunities.
Signed-off-by: Yosry Ahmed yosryahmed@google.com
Risk Level: Low
Testing: Integration tests
Docs Changes: N/A
Release Notes: N/A
Fixes #9976