From 8f6a60dd105ec57d2ee5b290a3172dce6b3c232e Mon Sep 17 00:00:00 2001 From: Raven Black Date: Fri, 10 May 2024 16:40:46 +0000 Subject: [PATCH] [cache filter] Add option to ignore cache-control directives from request Signed-off-by: Raven Black --- .../filters/http/cache/v3/cache.proto | 7 ++++- .../filters/http/cache/cache_filter.cc | 23 +++++++++------- .../filters/http/cache/cache_filter.h | 27 ++++++++++++------- .../extensions/filters/http/cache/config.cc | 7 +++-- .../filters/http/cache/http_cache.cc | 7 +++-- .../filters/http/cache/http_cache.h | 3 ++- .../filters/http/cache/cache_filter_test.cc | 5 ++-- .../filters/http/cache/http_cache_test.cc | 24 +++++++++++++++++ 8 files changed, 74 insertions(+), 29 deletions(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index 6c124e10f90f..70687b715084 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -20,7 +20,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // [#protodoc-title: HTTP Cache Filter] // [#extension: envoy.filters.http.cache] -// [#next-free-field: 6] +// [#next-free-field: 7] message CacheConfig { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.http.cache.v2alpha.CacheConfig"; @@ -88,4 +88,9 @@ message CacheConfig { // Max body size the cache filter will insert into a cache. 0 means unlimited (though the cache // storage implementation may have its own limit beyond which it will reject insertions). uint32 max_body_bytes = 4; + + // By default, a ``cache-control: no-cache`` or ``pragma: no-cache`` header in the request + // causes the cache to validate with its upstream even if the lookup is a hit. Setting this + // to true will ignore these headers. + bool ignore_request_cache_control_header = 6; } diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 8628a68e23c9..ddbf95193a6c 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -41,12 +41,15 @@ struct CacheResponseCodeDetailValues { using CacheResponseCodeDetails = ConstSingleton; -CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3::CacheConfig& config, - const std::string&, Stats::Scope&, - Server::Configuration::CommonFactoryContext& context, +CacheFilterConfig::CacheFilterConfig( + const envoy::extensions::filters::http::cache::v3::CacheConfig& config, + Server::Configuration::CommonFactoryContext& context) + : vary_allow_list_(config.allowed_vary_headers(), context), time_source_(context.timeSource()), + ignore_request_cache_control_header_(config.ignore_request_cache_control_header()) {} + +CacheFilter::CacheFilter(std::shared_ptr config, std::shared_ptr http_cache) - : time_source_(context.timeSource()), cache_(http_cache), - vary_allow_list_(config.allowed_vary_headers(), context) {} + : cache_(http_cache), config_(config) {} void CacheFilter::onDestroy() { filter_state_ = FilterState::Destroyed; @@ -99,7 +102,9 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& hea } ASSERT(decoder_callbacks_); - LookupRequest lookup_request(headers, time_source_.systemTime(), vary_allow_list_); + LookupRequest lookup_request(headers, config_->timeSource().systemTime(), + config_->varyAllowList(), + config_->ignoreRequestCacheControlHeader()); request_allows_inserts_ = !lookup_request.requestCacheControl().no_store_; is_head_request_ = headers.getMethodValue() == Http::Headers::get().MethodValues.Head; lookup_ = cache_->makeLookupContext(std::move(lookup_request), *decoder_callbacks_); @@ -151,7 +156,7 @@ Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& he // Either a cache miss or a cache entry that is no longer valid. // Check if the new response can be cached. if (request_allows_inserts_ && !is_head_request_ && - CacheabilityUtils::isCacheableResponse(headers, vary_allow_list_)) { + CacheabilityUtils::isCacheableResponse(headers, config_->varyAllowList())) { ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting headers", *encoder_callbacks_); auto insert_context = cache_->makeInsertContext(std::move(lookup_), *encoder_callbacks_); if (insert_context != nullptr) { @@ -166,7 +171,7 @@ Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& he insert_status_ = InsertStatus::InsertAbortedByCache; }); // Add metadata associated with the cached response. Right now this is only response_time; - const ResponseMetadata metadata = {time_source_.systemTime()}; + const ResponseMetadata metadata = {config_->timeSource().systemTime()}; insert_queue_->insertHeaders(headers, metadata, end_stream); } if (end_stream) { @@ -589,7 +594,7 @@ void CacheFilter::processSuccessfulValidation(Http::ResponseHeaderMap& response_ if (should_update_cached_entry) { // TODO(yosrym93): else the cached entry should be deleted. // Update metadata associated with the cached response. Right now this is only response_time; - const ResponseMetadata metadata = {time_source_.systemTime()}; + const ResponseMetadata metadata = {config_->timeSource().systemTime()}; cache_->updateHeaders(*lookup_, response_headers, metadata, [](bool updated ABSL_ATTRIBUTE_UNUSED) {}); insert_status_ = InsertStatus::HeaderUpdate; diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index 5cac2781fa10..a5fd855aa35f 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -46,6 +46,22 @@ enum class FilterState { Destroyed }; +class CacheFilterConfig { +public: + CacheFilterConfig(const envoy::extensions::filters::http::cache::v3::CacheConfig& config, + Server::Configuration::CommonFactoryContext& context); + + // The allow list rules that decide if a header can be varied upon. + const VaryAllowList& varyAllowList() const { return vary_allow_list_; } + TimeSource& timeSource() const { return time_source_; } + bool ignoreRequestCacheControlHeader() const { return ignore_request_cache_control_header_; } + +private: + const VaryAllowList vary_allow_list_; + TimeSource& time_source_; + const bool ignore_request_cache_control_header_; +}; + /** * A filter that caches responses and attempts to satisfy requests from cache. */ @@ -53,9 +69,7 @@ class CacheFilter : public Http::PassThroughFilter, public Logger::Loggable, public std::enable_shared_from_this { public: - CacheFilter(const envoy::extensions::filters::http::cache::v3::CacheConfig& config, - const std::string& stats_prefix, Stats::Scope& scope, - Server::Configuration::CommonFactoryContext& context, + CacheFilter(std::shared_ptr config, std::shared_ptr http_cache); // Http::StreamFilterBase void onDestroy() override; @@ -132,7 +146,6 @@ class CacheFilter : public Http::PassThroughFilter, // CacheFilter::onDestroy, allowing the insert queue to outlive the filter // while the necessary cache write operations complete. std::unique_ptr insert_queue_; - TimeSource& time_source_; std::shared_ptr cache_; LookupContextPtr lookup_; LookupResultPtr lookup_result_; @@ -142,11 +155,7 @@ class CacheFilter : public Http::PassThroughFilter, // onHeaders for Range Responses, otherwise initialized by encodeCachedResponse. std::vector remaining_ranges_; - // TODO(#12901): The allow list could be constructed only once directly from the config, instead - // of doing it per-request. A good example of such config is found in the gzip filter: - // source/extensions/filters/http/gzip/gzip_filter.h. - // Stores the allow list rules that decide if a header can be varied upon. - VaryAllowList vary_allow_list_; + const std::shared_ptr config_; // True if the response has trailers. // TODO(toddmgreer): cache trailers. diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index e379b4c05d1a..3e33af354307 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -9,7 +9,7 @@ namespace Cache { Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::cache::v3::CacheConfig& config, - const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { + const std::string& /*stats_prefix*/, Server::Configuration::FactoryContext& context) { std::shared_ptr cache; if (!config.disabled().value()) { if (!config.has_typed_config()) { @@ -26,10 +26,9 @@ Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( cache = http_cache_factory->getCache(config, context); } - return [config, stats_prefix, &context, + return [config = std::make_shared(config, context.serverFactoryContext()), cache](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamFilter(std::make_shared(config, stats_prefix, context.scope(), - context.serverFactoryContext(), cache)); + callbacks.addStreamFilter(std::make_shared(config, cache)); }; } diff --git a/source/extensions/filters/http/cache/http_cache.cc b/source/extensions/filters/http/cache/http_cache.cc index 73a2365a5f99..377ef2742562 100644 --- a/source/extensions/filters/http/cache/http_cache.cc +++ b/source/extensions/filters/http/cache/http_cache.cc @@ -24,7 +24,8 @@ namespace HttpFilters { namespace Cache { LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp, - const VaryAllowList& vary_allow_list) + const VaryAllowList& vary_allow_list, + bool ignore_request_cache_control_header) : request_headers_(Http::createHeaderMap(request_headers)), vary_allow_list_(vary_allow_list), timestamp_(timestamp) { // These ASSERTs check prerequisites. A request without these headers can't be looked up in cache; @@ -36,7 +37,9 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst absl::string_view scheme = request_headers.getSchemeValue(); ASSERT(Http::Utility::schemeIsValid(request_headers.getSchemeValue())); - initializeRequestCacheControl(request_headers); + if (!ignore_request_cache_control_header) { + initializeRequestCacheControl(request_headers); + } // TODO(toddmgreer): Let config determine whether to include scheme, host, and // query params. diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 177d12863e33..24533ca19467 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -78,7 +78,8 @@ class LookupRequest { public: // Prereq: request_headers's Path(), Scheme(), and Host() are non-null. LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp, - const VaryAllowList& vary_allow_list); + const VaryAllowList& vary_allow_list, + bool ignore_request_cache_control_header = false); const RequestCacheControl& requestCacheControl() const { return request_cache_control_; } diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 9ddf067b4d58..d481e905799e 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -30,9 +30,8 @@ class CacheFilterTest : public ::testing::Test { // The filter has to be created as a shared_ptr to enable shared_from_this() which is used in the // cache callbacks. CacheFilterSharedPtr makeFilter(std::shared_ptr cache, bool auto_destroy = true) { - std::shared_ptr filter(new CacheFilter(config_, /*stats_prefix=*/"", - context_.scope(), - context_.server_factory_context_, cache), + auto config = std::make_shared(config_, context_.server_factory_context_); + std::shared_ptr filter(new CacheFilter(config, cache), [auto_destroy](CacheFilter* f) { if (auto_destroy) { f->onDestroy(); diff --git a/test/extensions/filters/http/cache/http_cache_test.cc b/test/extensions/filters/http/cache/http_cache_test.cc index fa22525396ca..3d330dc158ca 100644 --- a/test/extensions/filters/http/cache/http_cache_test.cc +++ b/test/extensions/filters/http/cache/http_cache_test.cc @@ -259,6 +259,30 @@ TEST_F(LookupRequestTest, PragmaNoCacheFallback) { EXPECT_EQ(CacheEntryStatus::RequiresValidation, lookup_response.cache_entry_status_); } +// "pragma:no-cache" is ignored if ignoreRequestCacheControlHeader is true. +TEST_F(LookupRequestTest, IgnoreRequestCacheControlHeaderIgnoresPragma) { + request_headers_.setReferenceKey(Http::CustomHeaders::get().Pragma, "no-cache"); + const LookupRequest lookup_request(request_headers_, currentTime(), vary_allow_list_, + /*ignore_request_cache_control_header=*/true); + const Http::TestResponseHeaderMapImpl response_headers( + {{"date", formatter_.fromTime(currentTime())}, {"cache-control", "public, max-age=3600"}}); + const LookupResult lookup_response = makeLookupResult(lookup_request, response_headers); + // Response is not expired and no-cache is ignored. + EXPECT_EQ(CacheEntryStatus::Ok, lookup_response.cache_entry_status_); +} + +// "cache-control:no-cache" is ignored if ignoreRequestCacheControlHeader is true. +TEST_F(LookupRequestTest, IgnoreRequestCacheControlHeaderIgnoresCacheControl) { + request_headers_.setReferenceKey(Http::CustomHeaders::get().CacheControl, "no-cache"); + const LookupRequest lookup_request(request_headers_, currentTime(), vary_allow_list_, + /*ignore_request_cache_control_header=*/true); + const Http::TestResponseHeaderMapImpl response_headers( + {{"date", formatter_.fromTime(currentTime())}, {"cache-control", "public, max-age=3600"}}); + const LookupResult lookup_response = makeLookupResult(lookup_request, response_headers); + // Response is not expired and no-cache is ignored. + EXPECT_EQ(CacheEntryStatus::Ok, lookup_response.cache_entry_status_); +} + TEST_F(LookupRequestTest, PragmaNoCacheFallbackExtraDirectivesIgnored) { request_headers_.setReferenceKey(Http::CustomHeaders::get().Pragma, "no-cache, custom-directive=custom-value");