Skip to content

[cache-filter] Add disabled option #25916

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

Merged
ravenblackx merged 8 commits into from
Mar 14, 2023
Merged

[cache-filter] Add disabled option #25916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
17b05c8
Add disabled option to cache filter
ravenblackx Mar 2, 2023
96547d5
Avoid constructing OptRef from nullptr
ravenblackx Mar 3, 2023
edbe786
Disabled test should use cacheable request
ravenblackx Mar 3, 2023
343da0e
Don't use oneof, make disabled separate
ravenblackx Mar 6, 2023
29d8526
No longer using validate.proto
ravenblackx Mar 6, 2023
44d8554
Add use-cases to disabled field doc
ravenblackx Mar 13, 2023
7c2957c
Doc formatting better
ravenblackx Mar 13, 2023
7432e4b
Use ref link
ravenblackx Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions 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 @@ -6,10 +6,10 @@ import "envoy/config/route/v3/route_components.proto";
import "envoy/type/matcher/v3/string.proto";

import "google/protobuf/any.proto";
import "google/protobuf/wrappers.proto";

import "udpa/annotations/status.proto";
import "udpa/annotations/versioning.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.extensions.filters.http.cache.v3";
option java_outer_classname = "CacheProto";
Expand All @@ -20,6 +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]
message CacheConfig {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.cache.v2alpha.CacheConfig";
Expand Down Expand Up @@ -49,9 +50,17 @@ message CacheConfig {
repeated config.route.v3.QueryParameterMatcher query_parameters_excluded = 4;
}

// Config specific to the cache storage implementation.
// Config specific to the cache storage implementation. Required unless ``disabled``
// is true.
// [#extension-category: envoy.http.cache]
google.protobuf.Any typed_config = 1 [(validate.rules).any = {required: true}];
google.protobuf.Any typed_config = 1;

// When true, the cache filter is a no-op filter.
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should describe the use-cases where this is true (e.g., override by route and enable it).

Copy link
Copy Markdown
Contributor Author

@ravenblackx ravenblackx Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, doing that would probably be more confusing than helpful at this point because overriding by route isn't actually implemented yet. Describing things you can't actually do in the documentation seems worse than nothing.

At this version, the use for supporting disabling (until route overrides are available) is allowing ECDS to turn the filter off.

Which is also why the route-level generic disable isn't useful for this case - you can't toggle that with ECDS, you'd have to update some larger entity (the listener?).

The generic disable also doesn't allow for providing a disabled config at the base level and overriding it to enabled per-route (once per-route overrides are implemented).

Copy link
Copy Markdown
Contributor Author

@ravenblackx ravenblackx Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion was to add the ECDS use case to the doc even though it's a less-likely one, and the unimplemented one as a [#comment:]; done.

//
// Possible use-cases for this include:
// - Turning a filter on and off with :ref:`ECDS <envoy_v3_api_file_envoy/service/extension/v3/config_discovery.proto>`.
// [#comment: once route-specific overrides are implemented, they are the more likely use-case.]
google.protobuf.BoolValue disabled = 5;
Comment thread
alyssawilk marked this conversation as resolved.

// List of matching rules that defines allowed ``Vary`` headers.
//
Expand Down
14 changes: 9 additions & 5 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 @@ -33,7 +33,7 @@ using CacheResponseCodeDetails = ConstSingleton<CacheResponseCodeDetailValues>;

CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3::CacheConfig& config,
const std::string&, Stats::Scope&, TimeSource& time_source,
HttpCache& http_cache)
OptRef<HttpCache> http_cache)
: time_source_(time_source), cache_(http_cache),
vary_allow_list_(config.allowed_vary_headers()) {}

Expand All @@ -58,6 +58,10 @@ void CacheFilter::onStreamComplete() {

Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers,
bool end_stream) {
if (!cache_) {
filter_state_ = FilterState::NotServingFromCache;
return Http::FilterHeadersStatus::Continue;
}
ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers);
if (!end_stream) {
ENVOY_STREAM_LOG(
Expand All @@ -79,7 +83,7 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& hea
LookupRequest lookup_request(headers, time_source_.systemTime(), vary_allow_list_);
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_);
lookup_ = cache_->makeLookupContext(std::move(lookup_request), *decoder_callbacks_);

ASSERT(lookup_);
getHeaders(headers);
Expand Down Expand Up @@ -130,7 +134,7 @@ Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& he
if (request_allows_inserts_ && !is_head_request_ &&
CacheabilityUtils::isCacheableResponse(headers, vary_allow_list_)) {
ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting headers", *encoder_callbacks_);
insert_ = cache_.makeInsertContext(std::move(lookup_), *encoder_callbacks_);
insert_ = cache_->makeInsertContext(std::move(lookup_), *encoder_callbacks_);
// Add metadata associated with the cached response. Right now this is only response_time;
const ResponseMetadata metadata = {time_source_.systemTime()};
// TODO(capoferro): Note that there is currently no way to communicate back to the CacheFilter
Expand Down Expand Up @@ -567,8 +571,8 @@ void CacheFilter::processSuccessfulValidation(Http::ResponseHeaderMap& response_
// 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()};
cache_.updateHeaders(*lookup_, response_headers, metadata,
[](bool updated ABSL_ATTRIBUTE_UNUSED) {});
cache_->updateHeaders(*lookup_, response_headers, metadata,
[](bool updated ABSL_ATTRIBUTE_UNUSED) {});
insert_status_ = InsertStatus::HeaderUpdate;
}

Expand Down
4 changes: 2 additions & 2 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 @@ -54,7 +54,7 @@ class CacheFilter : public Http::PassThroughFilter,
public:
CacheFilter(const envoy::extensions::filters::http::cache::v3::CacheConfig& config,
const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source,
HttpCache& http_cache);
OptRef<HttpCache> http_cache);
// Http::StreamFilterBase
void onDestroy() override;
void onStreamComplete() override;
Expand Down Expand Up @@ -127,7 +127,7 @@ class CacheFilter : public Http::PassThroughFilter,
InsertStatus insertStatus() const;

TimeSource& time_source_;
HttpCache& cache_;
OptRef<HttpCache> cache_;
LookupContextPtr lookup_;
InsertContextPtr insert_;
LookupResultPtr lookup_result_;
Expand Down
25 changes: 16 additions & 9 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 @@ -10,20 +10,27 @@ 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 type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())};
HttpCacheFactory* const http_cache_factory =
Registry::FactoryRegistry<HttpCacheFactory>::getFactoryByType(type);
if (http_cache_factory == nullptr) {
throw EnvoyException(
fmt::format("Didn't find a registered implementation for type: '{}'", type));
}
std::shared_ptr<HttpCache> cache;
if (!config.disabled().value()) {
if (!config.has_typed_config()) {
throw EnvoyException("at least one of typed_config or disabled must be set");
}
const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())};
HttpCacheFactory* const http_cache_factory =
Registry::FactoryRegistry<HttpCacheFactory>::getFactoryByType(type);
if (http_cache_factory == nullptr) {
throw EnvoyException(
fmt::format("Didn't find a registered implementation for type: '{}'", type));
}

auto cache = http_cache_factory->getCache(config, context);
cache = http_cache_factory->getCache(config, context);
}

return [config, stats_prefix, &context,
cache](Http::FilterChainFactoryCallbacks& callbacks) -> void {
callbacks.addStreamFilter(std::make_shared<CacheFilter>(config, stats_prefix, context.scope(),
context.timeSource(), *cache));
context.timeSource(),
cache ? *cache : OptRef<HttpCache>{}));
};
}

Expand Down
9 changes: 8 additions & 1 deletion 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 @@ -27,7 +27,7 @@ class CacheFilterTest : public ::testing::Test {
protected:
// The filter has to be created as a shared_ptr to enable shared_from_this() which is used in the
// cache callbacks.
CacheFilterSharedPtr makeFilter(HttpCache& cache) {
CacheFilterSharedPtr makeFilter(OptRef<HttpCache> cache) {
auto filter = std::make_shared<CacheFilter>(config_, /*stats_prefix=*/"", context_.scope(),
context_.timeSource(), cache);
filter_state_ = std::make_shared<StreamInfo::FilterStateImpl>(
Expand Down Expand Up @@ -251,6 +251,13 @@ TEST_F(CacheFilterTest, CacheMiss) {
}
}

TEST_F(CacheFilterTest, Disabled) {
request_headers_.setHost("CacheDisabled");
CacheFilterSharedPtr filter = makeFilter(OptRef<HttpCache>{});
EXPECT_EQ(filter->decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue);
filter->onDestroy();
}

TEST_F(CacheFilterTest, CacheMissWithTrailers) {
request_headers_.setHost("CacheMissWithTrailers");
const std::string body = "abc";
Expand Down
10 changes: 10 additions & 0 deletions test/extensions/filters/http/cache/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ TEST_F(CacheFilterFactoryTest, Basic) {
ASSERT(dynamic_cast<CacheFilter*>(filter.get()));
}

TEST_F(CacheFilterFactoryTest, Disabled) {
config_.mutable_disabled()->set_value(true);
Http::FilterFactoryCb cb = factory_.createFilterFactoryFromProto(config_, "stats", context_);
Http::StreamFilterSharedPtr filter;
EXPECT_CALL(filter_callback_, addStreamFilter(_)).WillOnce(::testing::SaveArg<0>(&filter));
cb(filter_callback_);
ASSERT(filter);
ASSERT(dynamic_cast<CacheFilter*>(filter.get()));
}

TEST_F(CacheFilterFactoryTest, NoTypedConfig) {
EXPECT_THROW(factory_.createFilterFactoryFromProto(config_, "stats", context_), EnvoyException);
}
Expand Down