From 17b05c8d47d77345921cd28bdb5aa82b29d1c145 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Thu, 2 Mar 2023 23:09:36 +0000 Subject: [PATCH 1/8] Add disabled option to cache filter Signed-off-by: Raven Black --- .../filters/http/cache/v3/cache.proto | 14 +++++++++++--- .../filters/http/cache/cache_filter.cc | 14 +++++++++----- .../filters/http/cache/cache_filter.h | 4 ++-- .../extensions/filters/http/cache/config.cc | 19 +++++++++++-------- .../filters/http/cache/cache_filter_test.cc | 9 ++++++++- .../filters/http/cache/config_test.cc | 10 ++++++++++ 6 files changed, 51 insertions(+), 19 deletions(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index d47691c9b8c6c..6686bddf36f9b 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -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"; @@ -49,9 +50,16 @@ message CacheConfig { repeated config.route.v3.QueryParameterMatcher query_parameters_excluded = 4; } - // Config specific to the cache storage implementation. - // [#extension-category: envoy.http.cache] - google.protobuf.Any typed_config = 1 [(validate.rules).any = {required: true}]; + oneof cache_implementation { + option (validate.required) = true; + + // Config specific to the cache storage implementation. + // [#extension-category: envoy.http.cache] + google.protobuf.Any typed_config = 1; + + // When set, the cache filter is a no-op filter. + bool disabled = 5 [(validate.rules).bool = {const: true}]; + } // List of matching rules that defines allowed ``Vary`` headers. // diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 704c969f4e351..17e221308ab33 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -33,7 +33,7 @@ using CacheResponseCodeDetails = ConstSingleton; CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3::CacheConfig& config, const std::string&, Stats::Scope&, TimeSource& time_source, - HttpCache& http_cache) + OptRef http_cache) : time_source_(time_source), cache_(http_cache), vary_allow_list_(config.allowed_vary_headers()) {} @@ -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( @@ -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); @@ -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 @@ -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; } diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index b6abcc9277316..5b4977d1bbfc2 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -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 http_cache); // Http::StreamFilterBase void onDestroy() override; void onStreamComplete() override; @@ -127,7 +127,7 @@ class CacheFilter : public Http::PassThroughFilter, InsertStatus insertStatus() const; TimeSource& time_source_; - HttpCache& cache_; + OptRef cache_; LookupContextPtr lookup_; InsertContextPtr insert_; LookupResultPtr lookup_result_; diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 47e7f370d2f09..8a1cc64597a3b 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -10,15 +10,18 @@ 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::getFactoryByType(type); - if (http_cache_factory == nullptr) { - throw EnvoyException( - fmt::format("Didn't find a registered implementation for type: '{}'", type)); - } + std::shared_ptr cache; + if (!config.disabled()) { + const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + HttpCacheFactory* const http_cache_factory = + Registry::FactoryRegistry::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 { diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 0c0d88c6c2ef2..8fcb17d2fe38d 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -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 cache) { auto filter = std::make_shared(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource(), cache); filter_state_ = std::make_shared( @@ -251,6 +251,13 @@ TEST_F(CacheFilterTest, CacheMiss) { } } +TEST_F(CacheFilterTest, Disabled) { + request_headers_.setHost("CacheDisabled"); + CacheFilterSharedPtr filter = makeFilter(OptRef{}); + EXPECT_EQ(filter->decodeHeaders(request_headers_, false), Http::FilterHeadersStatus::Continue); + filter->onDestroy(); +} + TEST_F(CacheFilterTest, CacheMissWithTrailers) { request_headers_.setHost("CacheMissWithTrailers"); const std::string body = "abc"; diff --git a/test/extensions/filters/http/cache/config_test.cc b/test/extensions/filters/http/cache/config_test.cc index aad9398b6d317..cfe8a24d9339c 100644 --- a/test/extensions/filters/http/cache/config_test.cc +++ b/test/extensions/filters/http/cache/config_test.cc @@ -33,6 +33,16 @@ TEST_F(CacheFilterFactoryTest, Basic) { ASSERT(dynamic_cast(filter.get())); } +TEST_F(CacheFilterFactoryTest, Disabled) { + config_.set_disabled(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(filter.get())); +} + TEST_F(CacheFilterFactoryTest, NoTypedConfig) { EXPECT_THROW(factory_.createFilterFactoryFromProto(config_, "stats", context_), EnvoyException); } From 96547d5cad47c0e3c32bafef892696db39784900 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Fri, 3 Mar 2023 19:34:44 +0000 Subject: [PATCH 2/8] Avoid constructing OptRef from nullptr Signed-off-by: Raven Black --- source/extensions/filters/http/cache/config.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 8a1cc64597a3b..03cec45309e25 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -26,7 +26,8 @@ Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( return [config, stats_prefix, &context, cache](Http::FilterChainFactoryCallbacks& callbacks) -> void { callbacks.addStreamFilter(std::make_shared(config, stats_prefix, context.scope(), - context.timeSource(), *cache)); + context.timeSource(), + cache ? *cache : OptRef{})); }; } From edbe786e77b04ac2b80bf3e873816f2b1de7fba2 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Fri, 3 Mar 2023 20:10:17 +0000 Subject: [PATCH 3/8] Disabled test should use cacheable request Signed-off-by: Raven Black --- test/extensions/filters/http/cache/cache_filter_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 8fcb17d2fe38d..ff23ccd7f76cd 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -254,7 +254,7 @@ TEST_F(CacheFilterTest, CacheMiss) { TEST_F(CacheFilterTest, Disabled) { request_headers_.setHost("CacheDisabled"); CacheFilterSharedPtr filter = makeFilter(OptRef{}); - EXPECT_EQ(filter->decodeHeaders(request_headers_, false), Http::FilterHeadersStatus::Continue); + EXPECT_EQ(filter->decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue); filter->onDestroy(); } From 343da0e17bfbefc3a8452d7e1b350f37521ec2a4 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 6 Mar 2023 16:43:36 +0000 Subject: [PATCH 4/8] Don't use oneof, make disabled separate Signed-off-by: Raven Black --- .../extensions/filters/http/cache/v3/cache.proto | 16 +++++++--------- source/extensions/filters/http/cache/config.cc | 5 ++++- .../extensions/filters/http/cache/config_test.cc | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index 6686bddf36f9b..60ed9cdf2abe4 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -6,6 +6,7 @@ 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"; @@ -50,16 +51,13 @@ message CacheConfig { repeated config.route.v3.QueryParameterMatcher query_parameters_excluded = 4; } - oneof cache_implementation { - option (validate.required) = true; + // Config specific to the cache storage implementation. Required unless ``disabled`` + // is true. + // [#extension-category: envoy.http.cache] + google.protobuf.Any typed_config = 1; - // Config specific to the cache storage implementation. - // [#extension-category: envoy.http.cache] - google.protobuf.Any typed_config = 1; - - // When set, the cache filter is a no-op filter. - bool disabled = 5 [(validate.rules).bool = {const: true}]; - } + // When true, the cache filter is a no-op filter. + google.protobuf.BoolValue disabled = 5; // List of matching rules that defines allowed ``Vary`` headers. // diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 03cec45309e25..bccdf4768b9cb 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -11,7 +11,10 @@ Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::cache::v3::CacheConfig& config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { std::shared_ptr cache; - if (!config.disabled()) { + 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::getFactoryByType(type); diff --git a/test/extensions/filters/http/cache/config_test.cc b/test/extensions/filters/http/cache/config_test.cc index cfe8a24d9339c..109d1e000fd7a 100644 --- a/test/extensions/filters/http/cache/config_test.cc +++ b/test/extensions/filters/http/cache/config_test.cc @@ -34,7 +34,7 @@ TEST_F(CacheFilterFactoryTest, Basic) { } TEST_F(CacheFilterFactoryTest, Disabled) { - config_.set_disabled(true); + 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)); From 29d85261d999178486fb730ef8cfc8ec2c3d3766 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 6 Mar 2023 17:10:17 +0000 Subject: [PATCH 5/8] No longer using validate.proto Signed-off-by: Raven Black --- api/envoy/extensions/filters/http/cache/v3/cache.proto | 1 - 1 file changed, 1 deletion(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index 60ed9cdf2abe4..5227b97437c1a 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -10,7 +10,6 @@ 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"; From 44d8554a32f3a929b75736a0a81e1ac7b5017ac5 Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 13 Mar 2023 19:57:15 +0000 Subject: [PATCH 6/8] Add use-cases to disabled field doc Signed-off-by: Raven Black --- api/envoy/extensions/filters/http/cache/v3/cache.proto | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index 5227b97437c1a..f79802a3cba66 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -56,6 +56,9 @@ message CacheConfig { google.protobuf.Any typed_config = 1; // When true, the cache filter is a no-op filter. + // Possible use-cases for this include: + // * Turning a filter on and off with [ECDS](https://www.envoyproxy.io/docs/envoy/latest/api-v3/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; // List of matching rules that defines allowed ``Vary`` headers. From 7c2957cab27d4dc6975348f59a53397f28c3a6ac Mon Sep 17 00:00:00 2001 From: Raven Black Date: Mon, 13 Mar 2023 19:58:26 +0000 Subject: [PATCH 7/8] Doc formatting better Signed-off-by: Raven Black --- api/envoy/extensions/filters/http/cache/v3/cache.proto | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index f79802a3cba66..9f6cb930bb66f 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -56,8 +56,9 @@ message CacheConfig { google.protobuf.Any typed_config = 1; // When true, the cache filter is a no-op filter. + // // Possible use-cases for this include: - // * Turning a filter on and off with [ECDS](https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/extension/v3/config_discovery.proto). + // - Turning a filter on and off with [ECDS](https://www.envoyproxy.io/docs/envoy/latest/api-v3/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; From 7432e4b75e6713a03da78e6697e6c67e0dbb270a Mon Sep 17 00:00:00 2001 From: Raven Black Date: Tue, 14 Mar 2023 19:20:56 +0000 Subject: [PATCH 8/8] Use ref link Signed-off-by: Raven Black --- api/envoy/extensions/filters/http/cache/v3/cache.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/envoy/extensions/filters/http/cache/v3/cache.proto b/api/envoy/extensions/filters/http/cache/v3/cache.proto index 9f6cb930bb66f..6c124e10f90f3 100644 --- a/api/envoy/extensions/filters/http/cache/v3/cache.proto +++ b/api/envoy/extensions/filters/http/cache/v3/cache.proto @@ -58,7 +58,7 @@ message CacheConfig { // When true, the cache filter is a no-op filter. // // Possible use-cases for this include: - // - Turning a filter on and off with [ECDS](https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/extension/v3/config_discovery.proto). + // - Turning a filter on and off with :ref:`ECDS `. // [#comment: once route-specific overrides are implemented, they are the more likely use-case.] google.protobuf.BoolValue disabled = 5;