Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[cache filter] Add option to ignore cache-control directives from request #34090

Merged
merged 1 commit into from
May 16, 2024
Merged

[cache filter] Add option to ignore cache-control directives from request #34090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion api/envoy/extensions/filters/http/cache/v3/cache.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
}
23 changes: 14 additions & 9 deletions source/extensions/filters/http/cache/cache_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,15 @@ struct CacheResponseCodeDetailValues {

using CacheResponseCodeDetails = ConstSingleton<CacheResponseCodeDetailValues>;

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<const CacheFilterConfig> config,
std::shared_ptr<HttpCache> 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;
Expand Down Expand Up @@ -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_);
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
27 changes: 18 additions & 9 deletions source/extensions/filters/http/cache/cache_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,30 @@ enum class FilterState {
Destroyed
};

class CacheFilterConfig {
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.)

Copy link
Contributor

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.)

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.
*/
class CacheFilter : public Http::PassThroughFilter,
public Logger::Loggable<Logger::Id::cache_filter>,
public std::enable_shared_from_this<CacheFilter> {
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<const CacheFilterConfig> config,
std::shared_ptr<HttpCache> http_cache);
// Http::StreamFilterBase
void onDestroy() override;
Expand Down Expand Up @@ -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<CacheInsertQueue> insert_queue_;
TimeSource& time_source_;
std::shared_ptr<HttpCache> cache_;
LookupContextPtr lookup_;
LookupResultPtr lookup_result_;
Expand All @@ -142,11 +155,7 @@ class CacheFilter : public Http::PassThroughFilter,
// onHeaders for Range Responses, otherwise initialized by encodeCachedResponse.
std::vector<AdjustedByteRange> 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<const CacheFilterConfig> config_;

// True if the response has trailers.
// TODO(toddmgreer): cache trailers.
Expand Down
7 changes: 3 additions & 4 deletions source/extensions/filters/http/cache/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpCache> cache;
if (!config.disabled().value()) {
if (!config.has_typed_config()) {
Expand All @@ -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<CacheFilterConfig>(config, context.serverFactoryContext()),
cache](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamFilter(std::make_shared<CacheFilter>(config, stats_prefix, context.scope(),
context.serverFactoryContext(), cache));
callbacks.addStreamFilter(std::make_shared<CacheFilter>(config, cache));
};
}

Expand Down
7 changes: 5 additions & 2 deletions source/extensions/filters/http/cache/http_cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Http::RequestHeaderMapImpl>(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;
Expand All @@ -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.

Expand Down
3 changes: 2 additions & 1 deletion source/extensions/filters/http/cache/http_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }

Expand Down
5 changes: 2 additions & 3 deletions test/extensions/filters/http/cache/cache_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpCache> cache, bool auto_destroy = true) {
std::shared_ptr<CacheFilter> filter(new CacheFilter(config_, /*stats_prefix=*/"",
context_.scope(),
context_.server_factory_context_, cache),
auto config = std::make_shared<CacheFilterConfig>(config_, context_.server_factory_context_);
std::shared_ptr<CacheFilter> filter(new CacheFilter(config, cache),
[auto_destroy](CacheFilter* f) {
if (auto_destroy) {
f->onDestroy();
Expand Down
24 changes: 24 additions & 0 deletions test/extensions/filters/http/cache/http_cache_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down