From 361326d66bc9426bb41eb7c401a2dbc983a3fb8a Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Tue, 11 Feb 2020 11:02:11 -0800 Subject: [PATCH 01/17] Add CacheFilter: an HTTP caching filter based on the HttpCache plugin interface. Signed-off-by: Todd Greer --- source/common/common/logger.h | 1 + source/common/http/headers.h | 2 + source/extensions/filters/http/cache/BUILD | 18 ++ .../filters/http/cache/cache_filter.cc | 217 ++++++++++++++++++ .../filters/http/cache/cache_filter.h | 88 +++++++ .../filters/http/cache/http_cache.h | 2 +- .../filters/http/well_known_names.h | 2 + test/extensions/filters/http/cache/BUILD | 14 ++ .../filters/http/cache/cache_filter_test.cc | 127 ++++++++++ tools/spelling_dictionary.txt | 1 + 10 files changed, 471 insertions(+), 1 deletion(-) create mode 100644 source/extensions/filters/http/cache/cache_filter.cc create mode 100644 source/extensions/filters/http/cache/cache_filter.h create mode 100644 test/extensions/filters/http/cache/cache_filter_test.cc diff --git a/source/common/common/logger.h b/source/common/common/logger.h index cc4352ea02133..324d08e476a28 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -27,6 +27,7 @@ namespace Logger { FUNCTION(aws) \ FUNCTION(assert) \ FUNCTION(backtrace) \ + FUNCTION(cache_filter) \ FUNCTION(client) \ FUNCTION(config) \ FUNCTION(connection) \ diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 868845ef02485..90d0fa134edb7 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -59,6 +59,7 @@ class HeaderValues { const LowerCaseString AccessControlExposeHeaders{"access-control-expose-headers"}; const LowerCaseString AccessControlMaxAge{"access-control-max-age"}; const LowerCaseString AccessControlAllowCredentials{"access-control-allow-credentials"}; + const LowerCaseString Age{"age"}; const LowerCaseString Authorization{"authorization"}; const LowerCaseString ProxyAuthenticate{"proxy-authenticate"}; const LowerCaseString ProxyAuthorization{"proxy-authorization"}; @@ -172,6 +173,7 @@ class HeaderValues { const std::string NoCache{"no-cache"}; const std::string NoCacheMaxAge0{"no-cache, max-age=0"}; const std::string NoTransform{"no-transform"}; + const std::string Private{"private"}; } CacheControlValues; struct { diff --git a/source/extensions/filters/http/cache/BUILD b/source/extensions/filters/http/cache/BUILD index aa8b976639b66..450c300fce109 100644 --- a/source/extensions/filters/http/cache/BUILD +++ b/source/extensions/filters/http/cache/BUILD @@ -12,6 +12,24 @@ load( envoy_package() +envoy_cc_library( + name = "cache_filter_lib", + srcs = ["cache_filter.cc"], + hdrs = ["cache_filter.h"], + deps = [ + ":http_cache_lib", + "//include/envoy/registry", + "//source/common/common:logger_lib", + "//source/common/common:macros", + "//source/common/config:utility_lib", + "//source/common/http:header_map_lib", + "//source/common/http:headers_lib", + "//source/common/protobuf", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "@envoy_api//envoy/extensions/filters/http/cache/v3alpha:pkg_cc_proto", + ], +) + envoy_proto_library( name = "key", srcs = ["key.proto"], diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc new file mode 100644 index 0000000000000..86d98034b27cd --- /dev/null +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -0,0 +1,217 @@ +#include "extensions/filters/http/cache/cache_filter.h" + +#include "envoy/registry/registry.h" + +#include "common/config/utility.h" +#include "common/http/headers.h" + +#include "absl/memory/memory.h" +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { + +bool CacheFilter::isCacheableRequest(Http::HeaderMap& headers) { + const Http::HeaderEntry* method = headers.Method(); + const Http::HeaderEntry* forwarded_proto = headers.ForwardedProto(); + const Http::HeaderValues& header_values = Http::Headers::get(); + // TODO(toddmgreer): Also serve HEAD requests from cache. + // TODO(toddmgreer): Check all the other cache-related headers. + return method && forwarded_proto && headers.Path() && headers.Host() && + (method->value() == header_values.MethodValues.Get) && + (forwarded_proto->value() == header_values.SchemeValues.Http || + forwarded_proto->value() == header_values.SchemeValues.Https); +} + +bool CacheFilter::isCacheableResponse(Http::HeaderMap& headers) { + const Http::HeaderEntry* cache_control = headers.CacheControl(); + // TODO(toddmgreer): fully check for cacheability. See for example + // https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/http/caching_headers.h. + if (cache_control) { + return !StringUtil::caseFindToken(cache_control->value().getStringView(), ",", + Http::Headers::get().CacheControlValues.Private); + } + return false; +} + +HttpCache& +CacheFilter::getCache(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config) { + const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + HttpCacheFactory* const factory = + Registry::FactoryRegistry::getFactoryByType(type); + if (factory == nullptr) { + throw EnvoyException( + fmt::format("Didn't find a registered implementation for type: '{}'", type)); + } + return factory->getCache(config); +} + +CacheFilter::CacheFilter( + const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, const std::string&, + Stats::Scope&, TimeSource& time_source) + : time_source_(time_source), cache_(getCache(config)) {} + +void CacheFilter::onDestroy() { + lookup_ = nullptr; + insert_ = nullptr; +} + +Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::HeaderMap& headers, bool) { + ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers); + if (!isCacheableRequest(headers)) { + ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders ignoring uncacheable request: {}", + *decoder_callbacks_, headers); + return Http::FilterHeadersStatus::Continue; + } + ASSERT(decoder_callbacks_); + lookup_ = cache_.makeLookupContext(LookupRequest(headers, time_source_.systemTime())); + ASSERT(lookup_); + + CacheFilterSharedPtr self = shared_from_this(); + ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_); + lookup_->getHeaders([self](LookupResult&& result) { onHeadersAsync(self, std::move(result)); }); + return Http::FilterHeadersStatus::StopIteration; +} + +Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::HeaderMap& headers, bool end_stream) { + if (lookup_ && isCacheableResponse(headers)) { + ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting headers", *encoder_callbacks_); + insert_ = cache_.makeInsertContext(std::move(lookup_)); + insert_->insertHeaders(headers, end_stream); + } + return Http::FilterHeadersStatus::Continue; +} + +Http::FilterDataStatus CacheFilter::encodeData(Buffer::Instance& data, bool end_stream) { + if (insert_) { + ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting body", *encoder_callbacks_); + // TODO(toddmgreer): Wait for the cache if necessary. + insert_->insertBody( + data, [](bool) {}, end_stream); + } + return Http::FilterDataStatus::Continue; +} + +void CacheFilter::onOkHeaders(Http::HeaderMapPtr&& headers, + std::vector&& /*response_ranges*/, + uint64_t content_length, bool has_trailers) { + if (!lookup_) { + return; + } + response_has_trailers_ = has_trailers; + const bool end_stream = (content_length == 0 && !response_has_trailers_); + // TODO(toddmgreer): Calculate age per https://httpwg.org/specs/rfc7234.html#age.calculations + headers->addReferenceKey(Http::Headers::get().Age, 0); + decoder_callbacks_->encodeHeaders(std::move(headers), end_stream); + if (end_stream) { + return; + } + if (content_length > 0) { + remaining_body_.emplace_back(0, content_length); + getBody(); + } else { + lookup_->getTrailers([self = shared_from_this()](Http::HeaderMapPtr&& trailers) { + onTrailersAsync(self, std::move(trailers)); + }); + } +} + +void CacheFilter::onUnusableHeaders() { + if (lookup_) { + decoder_callbacks_->continueDecoding(); + } +} + +void CacheFilter::onHeadersAsync(const CacheFilterSharedPtr& self, LookupResult&& result) { + switch (result.cache_entry_status_) { + case CacheEntryStatus::RequiresValidation: + case CacheEntryStatus::FoundNotModified: + case CacheEntryStatus::UnsatisfiableRange: + ASSERT(false); // We don't yet return or support these codes. + FALLTHRU; + case CacheEntryStatus::Unusable: { + self->post([self] { self->onUnusableHeaders(); }); + return; + } + case CacheEntryStatus::Ok: + self->post([self, headers = result.headers_.release(), + response_ranges = std::move(result.response_ranges_), + content_length = result.content_length_, + has_trailers = result.has_trailers_]() mutable { + self->onOkHeaders(absl::WrapUnique(headers), std::move(response_ranges), content_length, + has_trailers); + }); + } +} + +void CacheFilter::getBody() { + ASSERT(!remaining_body_.empty(), "No reason to call getBody when there's no body to get."); + CacheFilterSharedPtr self = shared_from_this(); + lookup_->getBody(remaining_body_[0], + [self](Buffer::InstancePtr&& body) { self->onBody(std::move(body)); }); +} + +void CacheFilter::onBodyAsync(const CacheFilterSharedPtr& self, Buffer::InstancePtr&& body) { + self->post([self, body = body.release()] { self->onBody(absl::WrapUnique(body)); }); +} + +// TODO(toddmgreer): Handle downstream backpressure. +void CacheFilter::onBody(Buffer::InstancePtr&& body) { + if (!lookup_) { + return; + } + if (remaining_body_.empty()) { + ASSERT(false, "CacheFilter doesn't call getBody unless there's more body to get, so this is a " + "bogus callback."); + decoder_callbacks_->resetStream(); + return; + } + + if (!body) { + ASSERT(false, "Cache said it had a body, but isn't giving it to us."); + decoder_callbacks_->resetStream(); + return; + } + + const uint64_t bytes_from_cache = body->length(); + if (bytes_from_cache < remaining_body_[0].length()) { + remaining_body_[0].trimFront(bytes_from_cache); + } else if (bytes_from_cache == remaining_body_[0].length()) { + remaining_body_.erase(remaining_body_.begin()); + } else { + ASSERT(false, "Received oversized body from cache."); + decoder_callbacks_->resetStream(); + return; + } + + const bool end_stream = remaining_body_.empty() && !response_has_trailers_; + decoder_callbacks_->encodeData(*body, end_stream); + if (!remaining_body_.empty()) { + getBody(); + } else if (response_has_trailers_) { + lookup_->getTrailers([self = shared_from_this()](Http::HeaderMapPtr&& trailers) { + onTrailersAsync(self, std::move(trailers)); + }); + } +} + +void CacheFilter::onTrailers(Http::HeaderMapPtr&& trailers) { + if (lookup_) { + decoder_callbacks_->encodeTrailers(std::move(trailers)); + } +} + +void CacheFilter::onTrailersAsync(const CacheFilterSharedPtr& self, Http::HeaderMapPtr&& trailers) { + self->post( + [self, trailers = trailers.release()] { self->onTrailers(absl::WrapUnique(trailers)); }); +} + +void CacheFilter::post(std::function f) const { + decoder_callbacks_->dispatcher().post(std::move(f)); +} +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h new file mode 100644 index 0000000000000..47eca6121048f --- /dev/null +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -0,0 +1,88 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/extensions/filters/http/cache/v3alpha/cache.pb.h" + +#include "common/common/logger.h" + +#include "extensions/filters/http/cache/http_cache.h" +#include "extensions/filters/http/common/pass_through_filter.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { + +/** + * A filter that caches responses and attempts to satisfy requests from cache. + * It also inherits from std::enable_shared_from_this so it can pass shared_ptrs to async methods, + * to ensure that it doesn't get destroyed before they complete. + */ +class CacheFilter; +using CacheFilterSharedPtr = std::shared_ptr; +class CacheFilter : public Http::PassThroughFilter, + public Logger::Loggable, + public std::enable_shared_from_this { +public: + // Throws EnvoyException if no registered HttpCacheFactory for config.typed_config. + static CacheFilterSharedPtr + make(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, + const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source) { + // Can't use make_shared due to private constructor. + return std::shared_ptr(new CacheFilter(config, stats_prefix, scope, time_source)); + } + // Http::StreamFilterBase + void onDestroy() override; + // Http::StreamDecoderFilter + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + // Http::StreamEncoderFilter + Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterDataStatus encodeData(Buffer::Instance& buffer, bool end_stream) override; + +private: + // Throws EnvoyException if no registered HttpCacheFactory for config.typed_config. + // Constructor is private to enforce enable_shared_from_this's requirement that this must be owned + // by a shared_ptr. + CacheFilter(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, + const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source); + + void getBody(); + void onOkHeaders(Http::HeaderMapPtr&& headers, std::vector&& response_ranges, + uint64_t content_length, bool has_trailers); + void onUnusableHeaders(); + void onBody(Buffer::InstancePtr&& body); + void onTrailers(Http::HeaderMapPtr&& trailers); + static void onHeadersAsync(const CacheFilterSharedPtr& self, LookupResult&& result); + static void onBodyAsync(const CacheFilterSharedPtr& self, Buffer::InstancePtr&& body); + static void onTrailersAsync(const CacheFilterSharedPtr& self, Http::HeaderMapPtr&& trailers); + void post(std::function f) const; + + // These don't require private access, but are members per envoy convention. + static bool isCacheableRequest(Http::HeaderMap& headers); + static bool isCacheableResponse(Http::HeaderMap& headers); + static HttpCache& + getCache(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config); + + TimeSource& time_source_; + HttpCache& cache_; + LookupContextPtr lookup_; + InsertContextPtr insert_; + + // Tracks what body bytes still need to be read from the cache. This is + // currently only one Range, but will expand when full range support is added. Initialized by + // onOkHeaders. + std::vector remaining_body_; + + // True if the response has trailers. + // TODO(toddmgreer): cache trailers. + bool response_has_trailers_; +}; + +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 9bea3a43ea38c..2863343236bed 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -90,7 +90,7 @@ class AdjustedByteRange { private: uint64_t first_; - const uint64_t last_; + uint64_t last_; }; inline bool operator==(const AdjustedByteRange& lhs, const AdjustedByteRange& rhs) { diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 32297b0a71a40..e7dc609ef3cf2 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -14,6 +14,8 @@ class HttpFilterNameValues { public: // Buffer filter const std::string Buffer = "envoy.buffer"; + // Cache filter + const std::string Cache = "envoy.filters.http.cache"; // CORS filter const std::string Cors = "envoy.cors"; // CSRF filter diff --git a/test/extensions/filters/http/cache/BUILD b/test/extensions/filters/http/cache/BUILD index d2ac0fb3f74a4..7f37d15c64a88 100644 --- a/test/extensions/filters/http/cache/BUILD +++ b/test/extensions/filters/http/cache/BUILD @@ -31,3 +31,17 @@ envoy_extension_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_extension_cc_test( + name = "cache_filter_test", + srcs = ["cache_filter_test.cc"], + extension_name = "envoy.filters.http.cache", + deps = [ + "//source/extensions/filters/http/cache:cache_filter_lib", + "//source/extensions/filters/http/cache/simple_http_cache:config_cc_proto", + "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib", + "//test/mocks/server:server_mocks", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc new file mode 100644 index 0000000000000..ff1e1c91bdbcd --- /dev/null +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -0,0 +1,127 @@ +#include "source/extensions/filters/http/cache/simple_http_cache/config.pb.h" + +#include "extensions/filters/http/cache/cache_filter.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { +namespace { + +class CacheFilterTest : public ::testing::Test { +protected: + CacheFilterTest() { + config_.mutable_typed_config()->PackFrom( + envoy::source::extensions::filters::http::cache::SimpleHttpCacheConfig()); + ASSERT(config_.typed_config() + .Is()); + } + + CacheFilterSharedPtr makeFilter() { + CacheFilterSharedPtr filter = + CacheFilter::make(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource()); + if (filter) { + filter->setDecoderFilterCallbacks(decoder_callbacks_); + filter->setEncoderFilterCallbacks(encoder_callbacks_); + } + return filter; + } + + envoy::extensions::filters::http::cache::v3alpha::CacheConfig config_; + NiceMock context_; + Event::SimulatedTimeSystem time_source_; + DateFormatter formatter_{"%a, %d %b %Y %H:%M:%S GMT"}; + Http::TestHeaderMapImpl request_headers_{ + {":path", "/"}, {":method", "GET"}, {"x-forwarded-proto", "https"}}; + Http::TestHeaderMapImpl response_headers_{{":status", "200"}, + {"date", formatter_.now(time_source_)}, + {"cache-control", "public,max-age=3600"}}; + NiceMock decoder_callbacks_; + NiceMock encoder_callbacks_; +}; + +TEST_F(CacheFilterTest, ImmediateHitNoBody) { + request_headers_.setHost("ImmediateHitNoBody"); + ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); + ON_CALL(context_.dispatcher_, post(_)).WillByDefault(::testing::InvokeArgument<0>()); + + // Create filter for request 1 + CacheFilterSharedPtr filter = makeFilter(); + ASSERT_TRUE(filter); + + // Decode request 1 header + EXPECT_CALL(decoder_callbacks_, continueDecoding); + EXPECT_EQ(filter->decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + + // Encode response header + EXPECT_EQ(filter->encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); + filter->onDestroy(); + + // Create filter for request 2 + filter = makeFilter(); + ASSERT_TRUE(filter); + + // Decode request 2 header + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), + HeaderHasValueRef("age", "0")), + true)); + EXPECT_EQ(filter->decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + filter->onDestroy(); +} + +TEST_F(CacheFilterTest, ImmediateHitBody) { + request_headers_.setHost("ImmediateHitBody"); + ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); + ON_CALL(context_.dispatcher_, post(_)).WillByDefault(::testing::InvokeArgument<0>()); + + // Create filter for request 1 + CacheFilterSharedPtr filter = makeFilter(); + ASSERT_TRUE(filter); + + // Decode request 1 header + EXPECT_CALL(decoder_callbacks_, continueDecoding); + EXPECT_EQ(filter->decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + + // Encode response header + const std::string body = "abc"; + Buffer::OwnedImpl buffer(body); + response_headers_.setContentLength(body.size()); + EXPECT_EQ(filter->encodeHeaders(response_headers_, false), Http::FilterHeadersStatus::Continue); + EXPECT_EQ(filter->encodeData(buffer, true), Http::FilterDataStatus::Continue); + filter->onDestroy(); + + // Create filter for request 2 + filter = makeFilter(); + ASSERT_TRUE(filter); + + // Decode request 2 header + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), + HeaderHasValueRef("age", "0")), + false)); + EXPECT_CALL(decoder_callbacks_, + encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); + EXPECT_EQ(filter->decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + filter->onDestroy(); +} + +} // namespace +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 217468c6db3c3..aea5e245c3f76 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -413,6 +413,7 @@ builtins bulkstrings bursty bytecode +cacheability callee callsite callsites From 566e086091c341ae755d82fb029b8971391c4132 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 13 Feb 2020 17:44:22 -0800 Subject: [PATCH 02/17] Use OnDestroy to manage async events Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter.cc | 108 +++++++------- .../filters/http/cache/cache_filter.h | 36 ++--- .../filters/http/cache/http_cache.h | 22 ++- .../simple_http_cache/simple_http_cache.cc | 3 + .../filters/http/cache/cache_filter_test.cc | 133 +++++++++--------- 5 files changed, 149 insertions(+), 153 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 86d98034b27cd..939ab8196d484 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -5,7 +5,6 @@ #include "common/config/utility.h" #include "common/http/headers.h" -#include "absl/memory/memory.h" #include "absl/strings/string_view.h" namespace Envoy { @@ -54,8 +53,15 @@ CacheFilter::CacheFilter( : time_source_(time_source), cache_(getCache(config)) {} void CacheFilter::onDestroy() { - lookup_ = nullptr; - insert_ = nullptr; + // Clear decoder_callbacks_ so any pending callbacks will see that this filter is no longer + // active(). + decoder_callbacks_ = nullptr; + if (lookup_) { + lookup_->onDestroy(); + } + if (insert_) { + insert_->onDestroy(); + } } Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::HeaderMap& headers, bool) { @@ -69,9 +75,8 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::HeaderMap& headers, b lookup_ = cache_.makeLookupContext(LookupRequest(headers, time_source_.systemTime())); ASSERT(lookup_); - CacheFilterSharedPtr self = shared_from_this(); ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_); - lookup_->getHeaders([self](LookupResult&& result) { onHeadersAsync(self, std::move(result)); }); + lookup_->getHeaders([this](LookupResult&& result) { onHeadersAsync(std::move(result)); }); return Http::FilterHeadersStatus::StopIteration; } @@ -94,72 +99,59 @@ Http::FilterDataStatus CacheFilter::encodeData(Buffer::Instance& data, bool end_ return Http::FilterDataStatus::Continue; } -void CacheFilter::onOkHeaders(Http::HeaderMapPtr&& headers, - std::vector&& /*response_ranges*/, - uint64_t content_length, bool has_trailers) { - if (!lookup_) { - return; - } - response_has_trailers_ = has_trailers; - const bool end_stream = (content_length == 0 && !response_has_trailers_); - // TODO(toddmgreer): Calculate age per https://httpwg.org/specs/rfc7234.html#age.calculations - headers->addReferenceKey(Http::Headers::get().Age, 0); - decoder_callbacks_->encodeHeaders(std::move(headers), end_stream); - if (end_stream) { +void CacheFilter::onHeaders(LookupResult&& result) { + if (!active()) { return; } - if (content_length > 0) { - remaining_body_.emplace_back(0, content_length); - getBody(); - } else { - lookup_->getTrailers([self = shared_from_this()](Http::HeaderMapPtr&& trailers) { - onTrailersAsync(self, std::move(trailers)); - }); - } -} - -void CacheFilter::onUnusableHeaders() { - if (lookup_) { - decoder_callbacks_->continueDecoding(); - } -} - -void CacheFilter::onHeadersAsync(const CacheFilterSharedPtr& self, LookupResult&& result) { switch (result.cache_entry_status_) { case CacheEntryStatus::RequiresValidation: case CacheEntryStatus::FoundNotModified: case CacheEntryStatus::UnsatisfiableRange: - ASSERT(false); // We don't yet return or support these codes. - FALLTHRU; - case CacheEntryStatus::Unusable: { - self->post([self] { self->onUnusableHeaders(); }); + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // We don't yet return or support these codes. + case CacheEntryStatus::Unusable: + decoder_callbacks_->continueDecoding(); return; - } case CacheEntryStatus::Ok: - self->post([self, headers = result.headers_.release(), - response_ranges = std::move(result.response_ranges_), - content_length = result.content_length_, - has_trailers = result.has_trailers_]() mutable { - self->onOkHeaders(absl::WrapUnique(headers), std::move(response_ranges), content_length, - has_trailers); - }); - } + response_has_trailers_ = result.has_trailers_; + const bool end_stream = (result.content_length_ == 0 && !response_has_trailers_); + // TODO(toddmgreer): Calculate age per https://httpwg.org/specs/rfc7234.html#age.calculations + result.headers_->addReferenceKey(Http::Headers::get().Age, 0); + decoder_callbacks_->encodeHeaders(std::move(result.headers_), end_stream); + if (end_stream) { + return; + } + if (result.content_length_ > 0) { + remaining_body_.emplace_back(0, result.content_length_); + getBody(); + } else { + lookup_->getTrailers( + [this](Http::HeaderMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); + } + } +} + +void CacheFilter::onHeadersAsync(LookupResult&& result) { + post([this, status = result.cache_entry_status_, headers = result.headers_.release(), + response_ranges = std::move(result.response_ranges_), + content_length = result.content_length_, has_trailers = result.has_trailers_] { + onHeaders(LookupResult{status, absl::WrapUnique(headers), content_length, response_ranges, + has_trailers}); + }); } void CacheFilter::getBody() { ASSERT(!remaining_body_.empty(), "No reason to call getBody when there's no body to get."); - CacheFilterSharedPtr self = shared_from_this(); lookup_->getBody(remaining_body_[0], - [self](Buffer::InstancePtr&& body) { self->onBody(std::move(body)); }); + [this](Buffer::InstancePtr&& body) { onBody(std::move(body)); }); } -void CacheFilter::onBodyAsync(const CacheFilterSharedPtr& self, Buffer::InstancePtr&& body) { - self->post([self, body = body.release()] { self->onBody(absl::WrapUnique(body)); }); +void CacheFilter::onBodyAsync(Buffer::InstancePtr&& body) { + post([this, body = body.release()] { onBody(absl::WrapUnique(body)); }); } // TODO(toddmgreer): Handle downstream backpressure. void CacheFilter::onBody(Buffer::InstancePtr&& body) { - if (!lookup_) { + if (!active()) { return; } if (remaining_body_.empty()) { @@ -191,21 +183,19 @@ void CacheFilter::onBody(Buffer::InstancePtr&& body) { if (!remaining_body_.empty()) { getBody(); } else if (response_has_trailers_) { - lookup_->getTrailers([self = shared_from_this()](Http::HeaderMapPtr&& trailers) { - onTrailersAsync(self, std::move(trailers)); - }); + lookup_->getTrailers( + [this](Http::HeaderMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); } } void CacheFilter::onTrailers(Http::HeaderMapPtr&& trailers) { - if (lookup_) { + if (active()) { decoder_callbacks_->encodeTrailers(std::move(trailers)); } } -void CacheFilter::onTrailersAsync(const CacheFilterSharedPtr& self, Http::HeaderMapPtr&& trailers) { - self->post( - [self, trailers = trailers.release()] { self->onTrailers(absl::WrapUnique(trailers)); }); +void CacheFilter::onTrailersAsync(Http::HeaderMapPtr&& trailers) { + post([this, trailers = trailers.release()] { onTrailers(absl::WrapUnique(trailers)); }); } void CacheFilter::post(std::function f) const { diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index 47eca6121048f..a58894534e2e4 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -19,22 +19,13 @@ namespace Cache { /** * A filter that caches responses and attempts to satisfy requests from cache. - * It also inherits from std::enable_shared_from_this so it can pass shared_ptrs to async methods, - * to ensure that it doesn't get destroyed before they complete. */ -class CacheFilter; -using CacheFilterSharedPtr = std::shared_ptr; class CacheFilter : public Http::PassThroughFilter, - public Logger::Loggable, - public std::enable_shared_from_this { + public Logger::Loggable { public: // Throws EnvoyException if no registered HttpCacheFactory for config.typed_config. - static CacheFilterSharedPtr - make(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, - const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source) { - // Can't use make_shared due to private constructor. - return std::shared_ptr(new CacheFilter(config, stats_prefix, scope, time_source)); - } + CacheFilter(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, + const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source); // Http::StreamFilterBase void onDestroy() override; // Http::StreamDecoderFilter @@ -44,22 +35,16 @@ class CacheFilter : public Http::PassThroughFilter, Http::FilterDataStatus encodeData(Buffer::Instance& buffer, bool end_stream) override; private: - // Throws EnvoyException if no registered HttpCacheFactory for config.typed_config. - // Constructor is private to enforce enable_shared_from_this's requirement that this must be owned - // by a shared_ptr. - CacheFilter(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, - const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source); - void getBody(); - void onOkHeaders(Http::HeaderMapPtr&& headers, std::vector&& response_ranges, - uint64_t content_length, bool has_trailers); - void onUnusableHeaders(); + void post(std::function f) const; + bool active() const { return decoder_callbacks_; } + + void onHeaders(LookupResult&& result); + void onHeadersAsync(LookupResult&& result); void onBody(Buffer::InstancePtr&& body); + void onBodyAsync(Buffer::InstancePtr&& body); void onTrailers(Http::HeaderMapPtr&& trailers); - static void onHeadersAsync(const CacheFilterSharedPtr& self, LookupResult&& result); - static void onBodyAsync(const CacheFilterSharedPtr& self, Buffer::InstancePtr&& body); - static void onTrailersAsync(const CacheFilterSharedPtr& self, Http::HeaderMapPtr&& trailers); - void post(std::function f) const; + void onTrailersAsync(Http::HeaderMapPtr&& trailers); // These don't require private access, but are members per envoy convention. static bool isCacheableRequest(Http::HeaderMap& headers); @@ -81,6 +66,7 @@ class CacheFilter : public Http::PassThroughFilter, // TODO(toddmgreer): cache trailers. bool response_has_trailers_; }; +using CacheFilterPtr = std::unique_ptr; } // namespace Cache } // namespace HttpFilters diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 2863343236bed..f8441d5e1afbd 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -231,6 +231,15 @@ class InsertContext { // Inserts trailers into the cache. virtual void insertTrailers(const Http::HeaderMap& trailers) PURE; + /** + * This routine is called prior to an InsertContext being destroyed. InsertContext is responsible + * for making sure that any async events are cleaned up in the context of this routine. This + * includes timers, network calls, etc. The reason there is an onDestroy() method vs. doing this + * type of cleanup in the destructor is due to the deferred deletion model that Envoy uses to + * avoid stack unwind complications. InsertContext must not invoke any callbacks after having + * onDestroy() invoked. + */ + virtual void onDestroy() PURE; virtual ~InsertContext() = default; }; using InsertContextPtr = std::unique_ptr; @@ -240,8 +249,6 @@ using InsertContextPtr = std::unique_ptr; // an in-progress lookup by simply dropping the LookupContextPtr. class LookupContext { public: - virtual ~LookupContext() = default; - // Get the headers from the cache. It is a programming error to call this // twice. virtual void getHeaders(LookupHeadersCallback&& cb) PURE; @@ -267,6 +274,17 @@ class LookupContext { // Get the trailers from the cache. Only called if LookupResult::has_trailers // == true. virtual void getTrailers(LookupTrailersCallback&& cb) PURE; + + /** + * This routine is called prior to an LookupContext being destroyed. LookupContext is responsible + * for making sure that any async events are cleaned up in the context of this routine. This + * includes timers, network calls, etc. The reason there is an onDestroy() method vs. doing this + * type of cleanup in the destructor is due to the deferred deletion model that Envoy uses to + * avoid stack unwind complications. LookupContext must not invoke any callbacks after having + * onDestroy() invoked. + */ + virtual void onDestroy() PURE; + virtual ~LookupContext() = default; }; using LookupContextPtr = std::unique_ptr; diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc index 8c0a1e205b89a..205abd0f408b8 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc @@ -37,6 +37,7 @@ class SimpleLookupContext : public LookupContext { } const LookupRequest& request() const { return request_; } + void onDestroy() override {} private: SimpleHttpCache& cache_; @@ -74,6 +75,8 @@ class SimpleInsertContext : public InsertContext { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // TODO(toddmgreer): support trailers } + void onDestroy() override {} + private: void commit() { committed_ = true; diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index ff1e1c91bdbcd..fd2508ff43a7a 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -23,13 +23,10 @@ class CacheFilterTest : public ::testing::Test { .Is()); } - CacheFilterSharedPtr makeFilter() { - CacheFilterSharedPtr filter = - CacheFilter::make(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource()); - if (filter) { - filter->setDecoderFilterCallbacks(decoder_callbacks_); - filter->setEncoderFilterCallbacks(encoder_callbacks_); - } + CacheFilter makeFilter() { + CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource()); + filter.setDecoderFilterCallbacks(decoder_callbacks_); + filter.setEncoderFilterCallbacks(encoder_callbacks_); return filter; } @@ -51,73 +48,75 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); ON_CALL(context_.dispatcher_, post(_)).WillByDefault(::testing::InvokeArgument<0>()); - // Create filter for request 1 - CacheFilterSharedPtr filter = makeFilter(); - ASSERT_TRUE(filter); - - // Decode request 1 header - EXPECT_CALL(decoder_callbacks_, continueDecoding); - EXPECT_EQ(filter->decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); - - // Encode response header - EXPECT_EQ(filter->encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); - filter->onDestroy(); - - // Create filter for request 2 - filter = makeFilter(); - ASSERT_TRUE(filter); - - // Decode request 2 header - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), - HeaderHasValueRef("age", "0")), - true)); - EXPECT_EQ(filter->decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); - filter->onDestroy(); + { + // Create filter for request 1 + CacheFilter filter = makeFilter(); + + // Decode request 1 header + EXPECT_CALL(decoder_callbacks_, continueDecoding); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + + // Encode response header + EXPECT_EQ(filter.encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); + filter.onDestroy(); + } + { + // Create filter for request 2 + CacheFilter filter = makeFilter(); + + // Decode request 2 header + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), + HeaderHasValueRef("age", "0")), + true)); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + filter.onDestroy(); + } } TEST_F(CacheFilterTest, ImmediateHitBody) { request_headers_.setHost("ImmediateHitBody"); ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); ON_CALL(context_.dispatcher_, post(_)).WillByDefault(::testing::InvokeArgument<0>()); - - // Create filter for request 1 - CacheFilterSharedPtr filter = makeFilter(); - ASSERT_TRUE(filter); - - // Decode request 1 header - EXPECT_CALL(decoder_callbacks_, continueDecoding); - EXPECT_EQ(filter->decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); - - // Encode response header const std::string body = "abc"; - Buffer::OwnedImpl buffer(body); - response_headers_.setContentLength(body.size()); - EXPECT_EQ(filter->encodeHeaders(response_headers_, false), Http::FilterHeadersStatus::Continue); - EXPECT_EQ(filter->encodeData(buffer, true), Http::FilterDataStatus::Continue); - filter->onDestroy(); - - // Create filter for request 2 - filter = makeFilter(); - ASSERT_TRUE(filter); - - // Decode request 2 header - EXPECT_CALL(decoder_callbacks_, - encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), - HeaderHasValueRef("age", "0")), - false)); - EXPECT_CALL(decoder_callbacks_, - encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); - EXPECT_EQ(filter->decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); - filter->onDestroy(); + + { + // Create filter for request 1 + CacheFilter filter = makeFilter(); + + // Decode request 1 header + EXPECT_CALL(decoder_callbacks_, continueDecoding); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + + // Encode response header + Buffer::OwnedImpl buffer(body); + response_headers_.setContentLength(body.size()); + EXPECT_EQ(filter.encodeHeaders(response_headers_, false), Http::FilterHeadersStatus::Continue); + EXPECT_EQ(filter.encodeData(buffer, true), Http::FilterDataStatus::Continue); + filter.onDestroy(); + } + { + // Create filter for request 2 + CacheFilter filter = makeFilter(); + + // Decode request 2 header + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), + HeaderHasValueRef("age", "0")), + false)); + EXPECT_CALL(decoder_callbacks_, + encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + filter.onDestroy(); + } } } // namespace From e57d95ee4211c0f5bbe0ce60407b82393f0c9bef Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 13 Feb 2020 18:19:44 -0800 Subject: [PATCH 03/17] Find HttpCache implementation in factory Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/BUILD | 4 ---- .../filters/http/cache/cache_filter.cc | 23 ++++--------------- .../filters/http/cache/cache_filter.h | 6 ++--- .../filters/http/cache/http_cache.h | 2 +- test/extensions/filters/http/cache/BUILD | 1 - .../filters/http/cache/cache_filter_test.cc | 13 +++-------- 6 files changed, 10 insertions(+), 39 deletions(-) diff --git a/source/extensions/filters/http/cache/BUILD b/source/extensions/filters/http/cache/BUILD index 450c300fce109..4be1e0c676428 100644 --- a/source/extensions/filters/http/cache/BUILD +++ b/source/extensions/filters/http/cache/BUILD @@ -18,13 +18,9 @@ envoy_cc_library( hdrs = ["cache_filter.h"], deps = [ ":http_cache_lib", - "//include/envoy/registry", "//source/common/common:logger_lib", "//source/common/common:macros", - "//source/common/config:utility_lib", - "//source/common/http:header_map_lib", "//source/common/http:headers_lib", - "//source/common/protobuf", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//envoy/extensions/filters/http/cache/v3alpha:pkg_cc_proto", ], diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 939ab8196d484..98bbabca3de07 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -1,8 +1,5 @@ #include "extensions/filters/http/cache/cache_filter.h" -#include "envoy/registry/registry.h" - -#include "common/config/utility.h" #include "common/http/headers.h" #include "absl/strings/string_view.h" @@ -35,22 +32,10 @@ bool CacheFilter::isCacheableResponse(Http::HeaderMap& headers) { return false; } -HttpCache& -CacheFilter::getCache(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config) { - const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; - HttpCacheFactory* const factory = - Registry::FactoryRegistry::getFactoryByType(type); - if (factory == nullptr) { - throw EnvoyException( - fmt::format("Didn't find a registered implementation for type: '{}'", type)); - } - return factory->getCache(config); -} - -CacheFilter::CacheFilter( - const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, const std::string&, - Stats::Scope&, TimeSource& time_source) - : time_source_(time_source), cache_(getCache(config)) {} +CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig&, + const std::string&, Stats::Scope&, TimeSource& time_source, + HttpCache& http_cache) + : time_source_(time_source), cache_(http_cache) {} void CacheFilter::onDestroy() { // Clear decoder_callbacks_ so any pending callbacks will see that this filter is no longer diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index a58894534e2e4..237491002f419 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -23,9 +23,9 @@ namespace Cache { class CacheFilter : public Http::PassThroughFilter, public Logger::Loggable { public: - // Throws EnvoyException if no registered HttpCacheFactory for config.typed_config. CacheFilter(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, - const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source); + const std::string& stats_prefix, Stats::Scope& scope, TimeSource& time_source, + HttpCache& http_cache); // Http::StreamFilterBase void onDestroy() override; // Http::StreamDecoderFilter @@ -49,8 +49,6 @@ class CacheFilter : public Http::PassThroughFilter, // These don't require private access, but are members per envoy convention. static bool isCacheableRequest(Http::HeaderMap& headers); static bool isCacheableResponse(Http::HeaderMap& headers); - static HttpCache& - getCache(const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config); TimeSource& time_source_; HttpCache& cache_; diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index f8441d5e1afbd..6f11e0986a690 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -168,7 +168,7 @@ class LookupRequest { Key& key() { return key_; } // Returns the subset of this request's headers that are listed in - // envoy::config::filter::http::cache::v3::CacheConfig::allowed_vary_headers. If a cache + // envoy::extensions::filters::http::cache::v3alpha::CacheConfig::allowed_vary_headers. If a cache // storage implementation forwards lookup requests to a remote cache server that supports *vary* // headers, that server may need to see these headers. For local implementations, it may be // simpler to instead call makeLookupResult with each potential response. diff --git a/test/extensions/filters/http/cache/BUILD b/test/extensions/filters/http/cache/BUILD index 7f37d15c64a88..3a53a12ac9eb6 100644 --- a/test/extensions/filters/http/cache/BUILD +++ b/test/extensions/filters/http/cache/BUILD @@ -38,7 +38,6 @@ envoy_extension_cc_test( extension_name = "envoy.filters.http.cache", deps = [ "//source/extensions/filters/http/cache:cache_filter_lib", - "//source/extensions/filters/http/cache/simple_http_cache:config_cc_proto", "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib", "//test/mocks/server:server_mocks", "//test/test_common:simulated_time_system_lib", diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index fd2508ff43a7a..d38ff87863e2f 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -1,6 +1,5 @@ -#include "source/extensions/filters/http/cache/simple_http_cache/config.pb.h" - #include "extensions/filters/http/cache/cache_filter.h" +#include "extensions/filters/http/cache/simple_http_cache/simple_http_cache.h" #include "test/mocks/server/mocks.h" #include "test/test_common/simulated_time_system.h" @@ -16,20 +15,14 @@ namespace { class CacheFilterTest : public ::testing::Test { protected: - CacheFilterTest() { - config_.mutable_typed_config()->PackFrom( - envoy::source::extensions::filters::http::cache::SimpleHttpCacheConfig()); - ASSERT(config_.typed_config() - .Is()); - } - CacheFilter makeFilter() { - CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource()); + CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource(), cache_); filter.setDecoderFilterCallbacks(decoder_callbacks_); filter.setEncoderFilterCallbacks(encoder_callbacks_); return filter; } + SimpleHttpCache cache_; envoy::extensions::filters::http::cache::v3alpha::CacheConfig config_; NiceMock context_; Event::SimulatedTimeSystem time_source_; From ecc59d9041a900fcf90902019714b40a2c9418d2 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 13 Feb 2020 23:55:13 -0800 Subject: [PATCH 04/17] format fix Signed-off-by: Todd Greer --- test/extensions/filters/http/cache/cache_filter_test.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index d38ff87863e2f..4b6470c8eb7aa 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -16,7 +16,8 @@ namespace { class CacheFilterTest : public ::testing::Test { protected: CacheFilter makeFilter() { - CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource(), cache_); + CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource(), + cache_); filter.setDecoderFilterCallbacks(decoder_callbacks_); filter.setEncoderFilterCallbacks(encoder_callbacks_); return filter; @@ -103,8 +104,9 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), HeaderHasValueRef("age", "0")), false)); - EXPECT_CALL(decoder_callbacks_, - encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); + EXPECT_CALL( + decoder_callbacks_, + encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); EXPECT_EQ(filter.decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::StopIteration); ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); From ce5dfad6be8d45a03a4771c435c013440f74b2ec Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Tue, 18 Feb 2020 17:59:47 -0800 Subject: [PATCH 05/17] Replace HeaderMap with more specific subtype. Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter.cc | 17 +++++++++-------- .../filters/http/cache/cache_filter.h | 14 ++++++++------ .../extensions/filters/http/cache/http_cache.cc | 15 ++++++++------- .../extensions/filters/http/cache/http_cache.h | 16 ++++++++-------- .../simple_http_cache/simple_http_cache.cc | 8 ++++---- .../cache/simple_http_cache/simple_http_cache.h | 6 +++--- .../filters/http/cache/cache_filter_test.cc | 8 ++++---- .../filters/http/cache/http_cache_test.cc | 4 ++-- .../simple_http_cache/simple_http_cache_test.cc | 5 +++-- 9 files changed, 49 insertions(+), 44 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 98bbabca3de07..f22d5e74b1b97 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -9,7 +9,7 @@ namespace Extensions { namespace HttpFilters { namespace Cache { -bool CacheFilter::isCacheableRequest(Http::HeaderMap& headers) { +bool CacheFilter::isCacheableRequest(Http::RequestHeaderMap& headers) { const Http::HeaderEntry* method = headers.Method(); const Http::HeaderEntry* forwarded_proto = headers.ForwardedProto(); const Http::HeaderValues& header_values = Http::Headers::get(); @@ -21,7 +21,7 @@ bool CacheFilter::isCacheableRequest(Http::HeaderMap& headers) { forwarded_proto->value() == header_values.SchemeValues.Https); } -bool CacheFilter::isCacheableResponse(Http::HeaderMap& headers) { +bool CacheFilter::isCacheableResponse(Http::ResponseHeaderMap& headers) { const Http::HeaderEntry* cache_control = headers.CacheControl(); // TODO(toddmgreer): fully check for cacheability. See for example // https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/http/caching_headers.h. @@ -49,7 +49,7 @@ void CacheFilter::onDestroy() { } } -Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::HeaderMap& headers, bool) { +Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers); if (!isCacheableRequest(headers)) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders ignoring uncacheable request: {}", @@ -65,7 +65,8 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::HeaderMap& headers, b return Http::FilterHeadersStatus::StopIteration; } -Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::HeaderMap& headers, bool end_stream) { +Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& headers, + bool end_stream) { if (lookup_ && isCacheableResponse(headers)) { ENVOY_STREAM_LOG(debug, "CacheFilter::encodeHeaders inserting headers", *encoder_callbacks_); insert_ = cache_.makeInsertContext(std::move(lookup_)); @@ -110,7 +111,7 @@ void CacheFilter::onHeaders(LookupResult&& result) { getBody(); } else { lookup_->getTrailers( - [this](Http::HeaderMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); + [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); } } } @@ -169,17 +170,17 @@ void CacheFilter::onBody(Buffer::InstancePtr&& body) { getBody(); } else if (response_has_trailers_) { lookup_->getTrailers( - [this](Http::HeaderMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); + [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); } } -void CacheFilter::onTrailers(Http::HeaderMapPtr&& trailers) { +void CacheFilter::onTrailers(Http::ResponseTrailerMapPtr&& trailers) { if (active()) { decoder_callbacks_->encodeTrailers(std::move(trailers)); } } -void CacheFilter::onTrailersAsync(Http::HeaderMapPtr&& trailers) { +void CacheFilter::onTrailersAsync(Http::ResponseTrailerMapPtr&& trailers) { post([this, trailers = trailers.release()] { onTrailers(absl::WrapUnique(trailers)); }); } diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index 237491002f419..e0e34a7d7ca39 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -29,9 +29,11 @@ class CacheFilter : public Http::PassThroughFilter, // Http::StreamFilterBase void onDestroy() override; // Http::StreamDecoderFilter - Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, + bool end_stream) override; // Http::StreamEncoderFilter - Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, + bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance& buffer, bool end_stream) override; private: @@ -43,12 +45,12 @@ class CacheFilter : public Http::PassThroughFilter, void onHeadersAsync(LookupResult&& result); void onBody(Buffer::InstancePtr&& body); void onBodyAsync(Buffer::InstancePtr&& body); - void onTrailers(Http::HeaderMapPtr&& trailers); - void onTrailersAsync(Http::HeaderMapPtr&& trailers); + void onTrailers(Http::ResponseTrailerMapPtr&& trailers); + void onTrailersAsync(Http::ResponseTrailerMapPtr&& trailers); // These don't require private access, but are members per envoy convention. - static bool isCacheableRequest(Http::HeaderMap& headers); - static bool isCacheableResponse(Http::HeaderMap& headers); + static bool isCacheableRequest(Http::RequestHeaderMap& headers); + static bool isCacheableResponse(Http::ResponseHeaderMap& headers); TimeSource& time_source_; HttpCache& cache_; diff --git a/source/extensions/filters/http/cache/http_cache.cc b/source/extensions/filters/http/cache/http_cache.cc index 45a91f56c866e..7ae556d4891b2 100644 --- a/source/extensions/filters/http/cache/http_cache.cc +++ b/source/extensions/filters/http/cache/http_cache.cc @@ -37,18 +37,19 @@ std::ostream& operator<<(std::ostream& os, const AdjustedByteRange& range) { return os << "[" << range.begin() << "," << range.end() << ")"; } -LookupRequest::LookupRequest(const Http::HeaderMap& request_headers, SystemTime timestamp) +LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp) : timestamp_(timestamp), request_cache_control_(request_headers.CacheControl() == nullptr ? "" : request_headers.CacheControl()->value().getStringView()) { // These ASSERTs check prerequisites. A request without these headers can't be looked up in cache; // CacheFilter doesn't create LookupRequests for such requests. - ASSERT(request_headers.Path(), "Can't form cache lookup key for malformed Http::HeaderMap " + ASSERT(request_headers.Path(), "Can't form cache lookup key for malformed Http::RequestHeaderMap " "with null Path."); - ASSERT(request_headers.ForwardedProto(), - "Can't form cache lookup key for malformed Http::HeaderMap with null ForwardedProto."); - ASSERT(request_headers.Host(), "Can't form cache lookup key for malformed Http::HeaderMap " + ASSERT( + request_headers.ForwardedProto(), + "Can't form cache lookup key for malformed Http::RequestHeaderMap with null ForwardedProto."); + ASSERT(request_headers.Host(), "Can't form cache lookup key for malformed Http::RequestHeaderMap " "with null Host."); const Http::HeaderString& forwarded_proto = request_headers.ForwardedProto()->value(); const auto& scheme_values = Http::Headers::get().SchemeValues; @@ -71,7 +72,7 @@ size_t stableHashKey(const Key& key) { return MessageUtil::hash(key); } size_t localHashKey(const Key& key) { return stableHashKey(key); } // Returns true if response_headers is fresh. -bool LookupRequest::isFresh(const Http::HeaderMap& response_headers) const { +bool LookupRequest::isFresh(const Http::ResponseHeaderMap& response_headers) const { if (!response_headers.Date()) { return false; } @@ -86,7 +87,7 @@ bool LookupRequest::isFresh(const Http::HeaderMap& response_headers) const { return timestamp_ <= Utils::httpTime(response_headers.get(Http::Headers::get().Expires)); } -LookupResult LookupRequest::makeLookupResult(Http::HeaderMapPtr&& response_headers, +LookupResult LookupRequest::makeLookupResult(Http::ResponseHeaderMapPtr&& response_headers, uint64_t content_length) const { // TODO(toddmgreer): Implement all HTTP caching semantics. ASSERT(response_headers); diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 6f11e0986a690..218f783214fe4 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -115,7 +115,7 @@ struct LookupResult { CacheEntryStatus cache_entry_status_ = CacheEntryStatus::Unusable; // Headers of the cached response. - Http::HeaderMapPtr headers_; + Http::ResponseHeaderMapPtr headers_; // Size of the full response body. Cache filter will generate a content-length // header with this value, replacing any preexisting content-length header. @@ -160,7 +160,7 @@ class LookupRequest { using HeaderVector = std::vector; // Prereq: request_headers's Path(), Scheme(), and Host() are non-null. - LookupRequest(const Http::HeaderMap& request_headers, SystemTime timestamp); + LookupRequest(const Http::RequestHeaderMap& request_headers, SystemTime timestamp); // Caches may modify the key according to local needs, though care must be // taken to ensure that meaningfully distinct responses have distinct keys. @@ -187,11 +187,11 @@ class LookupRequest { // - LookupResult::content_length == content_length. // - LookupResult::response_ranges entries are satisfiable (as documented // there). - LookupResult makeLookupResult(Http::HeaderMapPtr&& response_headers, + LookupResult makeLookupResult(Http::ResponseHeaderMapPtr&& response_headers, uint64_t content_length) const; private: - bool isFresh(const Http::HeaderMap& response_headers) const; + bool isFresh(const Http::ResponseHeaderMap& response_headers) const; Key key_; std::vector request_range_spec_; @@ -208,14 +208,14 @@ struct CacheInfo { using LookupBodyCallback = std::function; using LookupHeadersCallback = std::function; -using LookupTrailersCallback = std::function; +using LookupTrailersCallback = std::function; using InsertCallback = std::function; // Manages the lifetime of an insertion. class InsertContext { public: // Accepts response_headers for caching. Only called once. - virtual void insertHeaders(const Http::HeaderMap& response_headers, bool end_stream) PURE; + virtual void insertHeaders(const Http::ResponseHeaderMap& response_headers, bool end_stream) PURE; // The insertion is streamed into the cache in chunks whose size is determined // by the client, but with a pace determined by the cache. To avoid streaming @@ -229,7 +229,7 @@ class InsertContext { bool end_stream) PURE; // Inserts trailers into the cache. - virtual void insertTrailers(const Http::HeaderMap& trailers) PURE; + virtual void insertTrailers(const Http::ResponseTrailerMap& trailers) PURE; /** * This routine is called prior to an InsertContext being destroyed. InsertContext is responsible @@ -310,7 +310,7 @@ class HttpCache { // This is called when an expired cache entry is successfully validated, to // update the cache entry. virtual void updateHeaders(LookupContextPtr&& lookup_context, - Http::HeaderMapPtr&& response_headers) PURE; + Http::ResponseHeaderMapPtr&& response_headers) PURE; // Returns statically known information about a cache. virtual CacheInfo cacheInfo() const PURE; diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc index 1ea6f1e5f8f78..47c84e0232a18 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc @@ -50,7 +50,7 @@ class SimpleInsertContext : public InsertContext { SimpleInsertContext(LookupContext& lookup_context, SimpleHttpCache& cache) : key_(dynamic_cast(lookup_context).request().key()), cache_(cache) {} - void insertHeaders(const Http::HeaderMap& response_headers, bool end_stream) override { + void insertHeaders(const Http::ResponseHeaderMap& response_headers, bool end_stream) override { ASSERT(!committed_); response_headers_ = Http::createHeaderMap(response_headers); if (end_stream) { @@ -71,7 +71,7 @@ class SimpleInsertContext : public InsertContext { } } - void insertTrailers(const Http::HeaderMap&) override { + void insertTrailers(const Http::ResponseTrailerMap&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // TODO(toddmgreer): support trailers } @@ -96,7 +96,7 @@ LookupContextPtr SimpleHttpCache::makeLookupContext(LookupRequest&& request) { } void SimpleHttpCache::updateHeaders(LookupContextPtr&& lookup_context, - Http::HeaderMapPtr&& response_headers) { + Http::ResponseHeaderMapPtr&& response_headers) { ASSERT(lookup_context); ASSERT(response_headers); // TODO(toddmgreer): Support updating headers. @@ -115,7 +115,7 @@ SimpleHttpCache::Entry SimpleHttpCache::lookup(const LookupRequest& request) { iter->second.body_}; } -void SimpleHttpCache::insert(const Key& key, Http::HeaderMapPtr&& response_headers, +void SimpleHttpCache::insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, std::string&& body) { absl::WriterMutexLock lock(&mutex_); map_[key] = SimpleHttpCache::Entry{std::move(response_headers), std::move(body)}; diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h index 4f53c28ec32af..ba3851142874c 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.h @@ -17,7 +17,7 @@ namespace Cache { class SimpleHttpCache : public HttpCache { private: struct Entry { - Http::HeaderMapPtr response_headers_; + Http::ResponseHeaderMapPtr response_headers_; std::string body_; }; @@ -26,11 +26,11 @@ class SimpleHttpCache : public HttpCache { LookupContextPtr makeLookupContext(LookupRequest&& request) override; InsertContextPtr makeInsertContext(LookupContextPtr&& lookup_context) override; void updateHeaders(LookupContextPtr&& lookup_context, - Http::HeaderMapPtr&& response_headers) override; + Http::ResponseHeaderMapPtr&& response_headers) override; CacheInfo cacheInfo() const override; Entry lookup(const LookupRequest& request); - void insert(const Key& key, Http::HeaderMapPtr&& response_headers, std::string&& body); + void insert(const Key& key, Http::ResponseHeaderMapPtr&& response_headers, std::string&& body); absl::Mutex mutex_; absl::flat_hash_map map_ GUARDED_BY(mutex_); diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 4b6470c8eb7aa..9058dd6aa4070 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -28,11 +28,11 @@ class CacheFilterTest : public ::testing::Test { NiceMock context_; Event::SimulatedTimeSystem time_source_; DateFormatter formatter_{"%a, %d %b %Y %H:%M:%S GMT"}; - Http::TestHeaderMapImpl request_headers_{ + Http::TestRequestHeaderMapImpl request_headers_{ {":path", "/"}, {":method", "GET"}, {"x-forwarded-proto", "https"}}; - Http::TestHeaderMapImpl response_headers_{{":status", "200"}, - {"date", formatter_.now(time_source_)}, - {"cache-control", "public,max-age=3600"}}; + Http::TestResponseHeaderMapImpl response_headers_{{":status", "200"}, + {"date", formatter_.now(time_source_)}, + {"cache-control", "public,max-age=3600"}}; NiceMock decoder_callbacks_; NiceMock encoder_callbacks_; }; diff --git a/test/extensions/filters/http/cache/http_cache_test.cc b/test/extensions/filters/http/cache/http_cache_test.cc index a641b659a73d0..3fd9cf6992939 100644 --- a/test/extensions/filters/http/cache/http_cache_test.cc +++ b/test/extensions/filters/http/cache/http_cache_test.cc @@ -71,10 +71,10 @@ class LookupRequestTest : public testing::Test { }; LookupResult makeLookupResult(const LookupRequest& lookup_request, - const Http::TestHeaderMapImpl& response_headers, + const Http::TestResponseHeaderMapImpl& response_headers, uint64_t content_length = 0) { return lookup_request.makeLookupResult( - std::make_unique(response_headers), content_length); + std::make_unique(response_headers), content_length); } TEST_F(LookupRequestTest, MakeLookupResultNoBody) { diff --git a/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc b/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc index e45d621abac6b..0196b1ffdff97 100644 --- a/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc +++ b/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc @@ -36,14 +36,15 @@ class SimpleHttpCacheTest : public testing::Test { } // Inserts a value into the cache. - void insert(LookupContextPtr lookup, const Http::TestHeaderMapImpl& response_headers, + void insert(LookupContextPtr lookup, const Http::TestResponseHeaderMapImpl& response_headers, const absl::string_view response_body) { InsertContextPtr inserter = cache_.makeInsertContext(move(lookup)); inserter->insertHeaders(response_headers, false); inserter->insertBody(Buffer::OwnedImpl(response_body), nullptr, true); } - void insert(absl::string_view request_path, const Http::TestHeaderMapImpl& response_headers, + void insert(absl::string_view request_path, + const Http::TestResponseHeaderMapImpl& response_headers, const absl::string_view response_body) { insert(lookup(request_path), response_headers, response_body); } From 0a63174d96d32d05d778ecbe926eccc528e85965 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 13:34:33 -0800 Subject: [PATCH 06/17] Remove unused using. Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/cache_filter.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index e0e34a7d7ca39..e3dd4055dd245 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -66,7 +66,6 @@ class CacheFilter : public Http::PassThroughFilter, // TODO(toddmgreer): cache trailers. bool response_has_trailers_; }; -using CacheFilterPtr = std::unique_ptr; } // namespace Cache } // namespace HttpFilters From c9423af1fb88bc9e9a7edd7f5f461d8236c24c68 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 13:45:32 -0800 Subject: [PATCH 07/17] Fix race on decoder_callbacks_, clarify comments, and fix getBody's call to OnBodyAsync. Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter.cc | 26 +++++++------------ .../filters/http/cache/http_cache.h | 5 ++-- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index f22d5e74b1b97..c818159d1fef0 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -38,15 +38,17 @@ CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3alpha: : time_source_(time_source), cache_(http_cache) {} void CacheFilter::onDestroy() { - // Clear decoder_callbacks_ so any pending callbacks will see that this filter is no longer - // active(). - decoder_callbacks_ = nullptr; if (lookup_) { lookup_->onDestroy(); } if (insert_) { insert_->onDestroy(); } + // There should be no more calls to our async callbacks, so they won't be posting any more + // callbacks to our dispatcher. However, there may be already posted callbacks in the queue; we + // clear decoder_callbacks_ so they'll see that this filter is no longer active(), and will just + // return. + decoder_callbacks_ = nullptr; } Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { @@ -128,7 +130,7 @@ void CacheFilter::onHeadersAsync(LookupResult&& result) { void CacheFilter::getBody() { ASSERT(!remaining_body_.empty(), "No reason to call getBody when there's no body to get."); lookup_->getBody(remaining_body_[0], - [this](Buffer::InstancePtr&& body) { onBody(std::move(body)); }); + [this](Buffer::InstancePtr&& body) { onBodyAsync(std::move(body)); }); } void CacheFilter::onBodyAsync(Buffer::InstancePtr&& body) { @@ -140,18 +142,10 @@ void CacheFilter::onBody(Buffer::InstancePtr&& body) { if (!active()) { return; } - if (remaining_body_.empty()) { - ASSERT(false, "CacheFilter doesn't call getBody unless there's more body to get, so this is a " - "bogus callback."); - decoder_callbacks_->resetStream(); - return; - } - - if (!body) { - ASSERT(false, "Cache said it had a body, but isn't giving it to us."); - decoder_callbacks_->resetStream(); - return; - } + ASSERT(!remaining_body_.empty(), + "CacheFilter doesn't call getBody unless there's more body to get, so this is a " + "bogus callback."); + ASSERT(body, "Cache said it had a body, but isn't giving it to us."); const uint64_t bytes_from_cache = body->length(); if (bytes_from_cache < remaining_body_[0].length()) { diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 218f783214fe4..69795dec34aa9 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -254,6 +254,7 @@ class LookupContext { virtual void getHeaders(LookupHeadersCallback&& cb) PURE; // Reads the next chunk from the cache, calling cb when the chunk is ready. + // The Buffer::InstancePtr passed to cb must not be null. // // The cache must call cb with a range of bytes starting at range.start() and // ending at or before range.end(). Caller is responsible for tracking what @@ -271,8 +272,8 @@ class LookupContext { // getBody requests bytes 20-23 .......... callback with bytes 20-23 virtual void getBody(const AdjustedByteRange& range, LookupBodyCallback&& cb) PURE; - // Get the trailers from the cache. Only called if LookupResult::has_trailers - // == true. + // Get the trailers from the cache. Only called if LookupResult::has_trailers == true. The + // Http::ResponseTrailerMapPtr passed to cb must not be null. virtual void getTrailers(LookupTrailersCallback&& cb) PURE; /** From 01543bcfe0cab8f86bbbc57cc71fb2e959ce45a9 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 14:09:08 -0800 Subject: [PATCH 08/17] Remove support for out of thread HttpCache implementations. Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter.cc | 43 +++---------------- .../filters/http/cache/cache_filter.h | 4 -- .../filters/http/cache/http_cache.h | 9 ---- .../simple_http_cache/simple_http_cache.cc | 1 - 4 files changed, 5 insertions(+), 52 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index c818159d1fef0..178c176918393 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -37,19 +37,7 @@ CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3alpha: HttpCache& http_cache) : time_source_(time_source), cache_(http_cache) {} -void CacheFilter::onDestroy() { - if (lookup_) { - lookup_->onDestroy(); - } - if (insert_) { - insert_->onDestroy(); - } - // There should be no more calls to our async callbacks, so they won't be posting any more - // callbacks to our dispatcher. However, there may be already posted callbacks in the queue; we - // clear decoder_callbacks_ so they'll see that this filter is no longer active(), and will just - // return. - decoder_callbacks_ = nullptr; -} +void CacheFilter::onDestroy() {} Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers); @@ -63,7 +51,7 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& hea ASSERT(lookup_); ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_); - lookup_->getHeaders([this](LookupResult&& result) { onHeadersAsync(std::move(result)); }); + lookup_->getHeaders([this](LookupResult&& result) { onHeaders(std::move(result)); }); return Http::FilterHeadersStatus::StopIteration; } @@ -113,28 +101,15 @@ void CacheFilter::onHeaders(LookupResult&& result) { getBody(); } else { lookup_->getTrailers( - [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); + [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailers(std::move(trailers)); }); } } } -void CacheFilter::onHeadersAsync(LookupResult&& result) { - post([this, status = result.cache_entry_status_, headers = result.headers_.release(), - response_ranges = std::move(result.response_ranges_), - content_length = result.content_length_, has_trailers = result.has_trailers_] { - onHeaders(LookupResult{status, absl::WrapUnique(headers), content_length, response_ranges, - has_trailers}); - }); -} - void CacheFilter::getBody() { ASSERT(!remaining_body_.empty(), "No reason to call getBody when there's no body to get."); lookup_->getBody(remaining_body_[0], - [this](Buffer::InstancePtr&& body) { onBodyAsync(std::move(body)); }); -} - -void CacheFilter::onBodyAsync(Buffer::InstancePtr&& body) { - post([this, body = body.release()] { onBody(absl::WrapUnique(body)); }); + [this](Buffer::InstancePtr&& body) { onBody(std::move(body)); }); } // TODO(toddmgreer): Handle downstream backpressure. @@ -164,7 +139,7 @@ void CacheFilter::onBody(Buffer::InstancePtr&& body) { getBody(); } else if (response_has_trailers_) { lookup_->getTrailers( - [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailersAsync(std::move(trailers)); }); + [this](Http::ResponseTrailerMapPtr&& trailers) { onTrailers(std::move(trailers)); }); } } @@ -173,14 +148,6 @@ void CacheFilter::onTrailers(Http::ResponseTrailerMapPtr&& trailers) { decoder_callbacks_->encodeTrailers(std::move(trailers)); } } - -void CacheFilter::onTrailersAsync(Http::ResponseTrailerMapPtr&& trailers) { - post([this, trailers = trailers.release()] { onTrailers(absl::WrapUnique(trailers)); }); -} - -void CacheFilter::post(std::function f) const { - decoder_callbacks_->dispatcher().post(std::move(f)); -} } // namespace Cache } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index e3dd4055dd245..cad6498212a16 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -38,15 +38,11 @@ class CacheFilter : public Http::PassThroughFilter, private: void getBody(); - void post(std::function f) const; bool active() const { return decoder_callbacks_; } void onHeaders(LookupResult&& result); - void onHeadersAsync(LookupResult&& result); void onBody(Buffer::InstancePtr&& body); - void onBodyAsync(Buffer::InstancePtr&& body); void onTrailers(Http::ResponseTrailerMapPtr&& trailers); - void onTrailersAsync(Http::ResponseTrailerMapPtr&& trailers); // These don't require private access, but are members per envoy convention. static bool isCacheableRequest(Http::RequestHeaderMap& headers); diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 69795dec34aa9..4d823c2d344ca 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -276,15 +276,6 @@ class LookupContext { // Http::ResponseTrailerMapPtr passed to cb must not be null. virtual void getTrailers(LookupTrailersCallback&& cb) PURE; - /** - * This routine is called prior to an LookupContext being destroyed. LookupContext is responsible - * for making sure that any async events are cleaned up in the context of this routine. This - * includes timers, network calls, etc. The reason there is an onDestroy() method vs. doing this - * type of cleanup in the destructor is due to the deferred deletion model that Envoy uses to - * avoid stack unwind complications. LookupContext must not invoke any callbacks after having - * onDestroy() invoked. - */ - virtual void onDestroy() PURE; virtual ~LookupContext() = default; }; using LookupContextPtr = std::unique_ptr; diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc index 47c84e0232a18..cd6401035135d 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc @@ -37,7 +37,6 @@ class SimpleLookupContext : public LookupContext { } const LookupRequest& request() const { return request_; } - void onDestroy() override {} private: SimpleHttpCache& cache_; From 18b348b84e512dd6b4ff6761e3d88134380e5747 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 14:48:21 -0800 Subject: [PATCH 09/17] Remove vestigial 'active' method. Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/cache_filter.cc | 10 +--------- source/extensions/filters/http/cache/cache_filter.h | 2 -- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 178c176918393..4b9f1ee051ae7 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -76,9 +76,6 @@ Http::FilterDataStatus CacheFilter::encodeData(Buffer::Instance& data, bool end_ } void CacheFilter::onHeaders(LookupResult&& result) { - if (!active()) { - return; - } switch (result.cache_entry_status_) { case CacheEntryStatus::RequiresValidation: case CacheEntryStatus::FoundNotModified: @@ -114,9 +111,6 @@ void CacheFilter::getBody() { // TODO(toddmgreer): Handle downstream backpressure. void CacheFilter::onBody(Buffer::InstancePtr&& body) { - if (!active()) { - return; - } ASSERT(!remaining_body_.empty(), "CacheFilter doesn't call getBody unless there's more body to get, so this is a " "bogus callback."); @@ -144,9 +138,7 @@ void CacheFilter::onBody(Buffer::InstancePtr&& body) { } void CacheFilter::onTrailers(Http::ResponseTrailerMapPtr&& trailers) { - if (active()) { - decoder_callbacks_->encodeTrailers(std::move(trailers)); - } + decoder_callbacks_->encodeTrailers(std::move(trailers)); } } // namespace Cache } // namespace HttpFilters diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index cad6498212a16..314d0c6d16483 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -38,8 +38,6 @@ class CacheFilter : public Http::PassThroughFilter, private: void getBody(); - bool active() const { return decoder_callbacks_; } - void onHeaders(LookupResult&& result); void onBody(Buffer::InstancePtr&& body); void onTrailers(Http::ResponseTrailerMapPtr&& trailers); From a2bb906155372f9a0ef2aae406aa3bf48244d866 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 14:55:48 -0800 Subject: [PATCH 10/17] Delete insert_ and lookup_ in onDestroy. Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/cache_filter.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 4b9f1ee051ae7..c30038ea98891 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -37,7 +37,10 @@ CacheFilter::CacheFilter(const envoy::extensions::filters::http::cache::v3alpha: HttpCache& http_cache) : time_source_(time_source), cache_(http_cache) {} -void CacheFilter::onDestroy() {} +void CacheFilter::onDestroy() { + lookup_ = nullptr; + insert_ = nullptr; +} Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers); From d866b59105d83d1dae285ff929d740fd84593078 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 16:35:13 -0800 Subject: [PATCH 11/17] Remove vestigial onDestroy Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/http_cache.h | 9 --------- .../http/cache/simple_http_cache/simple_http_cache.cc | 2 -- 2 files changed, 11 deletions(-) diff --git a/source/extensions/filters/http/cache/http_cache.h b/source/extensions/filters/http/cache/http_cache.h index 4d823c2d344ca..5c82eeecf13ac 100644 --- a/source/extensions/filters/http/cache/http_cache.h +++ b/source/extensions/filters/http/cache/http_cache.h @@ -231,15 +231,6 @@ class InsertContext { // Inserts trailers into the cache. virtual void insertTrailers(const Http::ResponseTrailerMap& trailers) PURE; - /** - * This routine is called prior to an InsertContext being destroyed. InsertContext is responsible - * for making sure that any async events are cleaned up in the context of this routine. This - * includes timers, network calls, etc. The reason there is an onDestroy() method vs. doing this - * type of cleanup in the destructor is due to the deferred deletion model that Envoy uses to - * avoid stack unwind complications. InsertContext must not invoke any callbacks after having - * onDestroy() invoked. - */ - virtual void onDestroy() PURE; virtual ~InsertContext() = default; }; using InsertContextPtr = std::unique_ptr; diff --git a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc index cd6401035135d..5eadaa6a36922 100644 --- a/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc +++ b/source/extensions/filters/http/cache/simple_http_cache/simple_http_cache.cc @@ -74,8 +74,6 @@ class SimpleInsertContext : public InsertContext { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // TODO(toddmgreer): support trailers } - void onDestroy() override {} - private: void commit() { committed_ = true; From 09c38cceac0b190f7ab61598b401579969ce076f Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 18:09:35 -0800 Subject: [PATCH 12/17] Fix error of calling continueDecoding before stopping iteration. Signed-off-by: Todd Greer --- .../extensions/filters/http/cache/cache_filter.cc | 15 ++++++++++++++- .../extensions/filters/http/cache/cache_filter.h | 4 ++++ .../filters/http/cache/cache_filter_test.cc | 10 ++-------- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index c30038ea98891..1beb5538667c3 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -55,6 +55,13 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& hea ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders starting lookup", *decoder_callbacks_); lookup_->getHeaders([this](LookupResult&& result) { onHeaders(std::move(result)); }); + if (state_ == GetHeadersState::GetHeadersResultUnusable) { + // onHeaders has already been called, and no usable cache entry was found--continue iteration. + return Http::FilterHeadersStatus::Continue; + } + // onHeaders hasn't been called yet--stop iteration to wait for it, and tell it that we stopped + // iteration. + state_ = GetHeadersState::FinishedGetHeadersCall; return Http::FilterHeadersStatus::StopIteration; } @@ -85,7 +92,13 @@ void CacheFilter::onHeaders(LookupResult&& result) { case CacheEntryStatus::UnsatisfiableRange: NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // We don't yet return or support these codes. case CacheEntryStatus::Unusable: - decoder_callbacks_->continueDecoding(); + if (state_ == GetHeadersState::FinishedGetHeadersCall) { + // decodeHeader returned Http::FilterHeadersStatus::StopIteration--restart it + decoder_callbacks_->continueDecoding(); + } else { + // decodeHeader hasn't yet returned--tell it to return Http::FilterHeadersStatus::Continue. + state_ = GetHeadersState::GetHeadersResultUnusable; + } return; case CacheEntryStatus::Ok: response_has_trailers_ = result.has_trailers_; diff --git a/source/extensions/filters/http/cache/cache_filter.h b/source/extensions/filters/http/cache/cache_filter.h index 314d0c6d16483..212ff8728284f 100644 --- a/source/extensions/filters/http/cache/cache_filter.h +++ b/source/extensions/filters/http/cache/cache_filter.h @@ -59,6 +59,10 @@ class CacheFilter : public Http::PassThroughFilter, // True if the response has trailers. // TODO(toddmgreer): cache trailers. bool response_has_trailers_; + + // Used for coordinating between decodeHeaders and onHeaders. + enum class GetHeadersState { Initial, FinishedGetHeadersCall, GetHeadersResultUnusable }; + GetHeadersState state_ = GetHeadersState::Initial; }; } // namespace Cache diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 9058dd6aa4070..2adb4e32e272d 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -47,10 +47,7 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { CacheFilter filter = makeFilter(); // Decode request 1 header - EXPECT_CALL(decoder_callbacks_, continueDecoding); - EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue); // Encode response header EXPECT_EQ(filter.encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); @@ -83,10 +80,7 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { CacheFilter filter = makeFilter(); // Decode request 1 header - EXPECT_CALL(decoder_callbacks_, continueDecoding); - EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); - ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue); // Encode response header Buffer::OwnedImpl buffer(body); From 0112fd26ef1ab2dc805f3aca8b5c460b6bbd2ce4 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Wed, 19 Feb 2020 19:28:56 -0800 Subject: [PATCH 13/17] Test that CacheFilter can handle HttpCache implementations that both do and don't respond immediately to getHeaders. Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter_test.cc | 87 +++++++++++++++++-- 1 file changed, 80 insertions(+), 7 deletions(-) diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 2adb4e32e272d..675aad4d344c6 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -13,17 +13,53 @@ namespace HttpFilters { namespace Cache { namespace { +// Wrapper for SimpleHttpCache that delays the onHeaders callback from getHeaders, for verifying +// that CacheFilter works correctly whether the onHeaders call happens immediately, or after +// getHeaders and decodeHeaders return. +class DelayedCache : public SimpleHttpCache { +public: + // HttpCache + LookupContextPtr makeLookupContext(LookupRequest&& request) override { + return std::make_unique( + SimpleHttpCache::makeLookupContext(std::move(request)), delayed_cb_); + } + InsertContextPtr makeInsertContext(LookupContextPtr&& lookup_context) override { + return SimpleHttpCache::makeInsertContext( + std::move(dynamic_cast(*lookup_context).context_)); + } + + std::function delayed_cb_; + +private: + class DelayedLookupContext : public LookupContext { + public: + DelayedLookupContext(LookupContextPtr&& context, std::function& delayed_cb) + : context_(std::move(context)), delayed_cb_(delayed_cb) {} + void getHeaders(LookupHeadersCallback&& cb) override { + delayed_cb_ = [this, cb]() mutable { context_->getHeaders(std::move(cb)); }; + } + void getBody(const AdjustedByteRange& range, LookupBodyCallback&& cb) override { + context_->getBody(range, std::move(cb)); + } + void getTrailers(LookupTrailersCallback&& cb) override { context_->getTrailers(std::move(cb)); } + + LookupContextPtr context_; + std::function& delayed_cb_; + }; +}; + class CacheFilterTest : public ::testing::Test { protected: - CacheFilter makeFilter() { + CacheFilter makeFilter(HttpCache& cache) { CacheFilter filter(config_, /*stats_prefix=*/"", context_.scope(), context_.timeSource(), - cache_); + cache); filter.setDecoderFilterCallbacks(decoder_callbacks_); filter.setEncoderFilterCallbacks(encoder_callbacks_); return filter; } - SimpleHttpCache cache_; + SimpleHttpCache simple_cache_; + DelayedCache delayed_cache_; envoy::extensions::filters::http::cache::v3alpha::CacheConfig config_; NiceMock context_; Event::SimulatedTimeSystem time_source_; @@ -44,7 +80,7 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { { // Create filter for request 1 - CacheFilter filter = makeFilter(); + CacheFilter filter = makeFilter(simple_cache_); // Decode request 1 header EXPECT_EQ(filter.decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue); @@ -55,7 +91,7 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { } { // Create filter for request 2 - CacheFilter filter = makeFilter(); + CacheFilter filter = makeFilter(simple_cache_); // Decode request 2 header EXPECT_CALL(decoder_callbacks_, @@ -69,6 +105,43 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { } } +TEST_F(CacheFilterTest, DelayedHitNoBody) { + request_headers_.setHost("ImmediateHitNoBody"); + ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); + ON_CALL(context_.dispatcher_, post(_)).WillByDefault(::testing::InvokeArgument<0>()); + + { + // Create filter for request 1 + CacheFilter filter = makeFilter(delayed_cache_); + + // Decode request 1 header + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + EXPECT_CALL(decoder_callbacks_, continueDecoding); + delayed_cache_.delayed_cb_(); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + + // Encode response header + EXPECT_EQ(filter.encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); + filter.onDestroy(); + } + { + // Create filter for request 2 + CacheFilter filter = makeFilter(delayed_cache_); + + // Decode request 2 header + EXPECT_EQ(filter.decodeHeaders(request_headers_, true), + Http::FilterHeadersStatus::StopIteration); + EXPECT_CALL(decoder_callbacks_, + encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), + HeaderHasValueRef("age", "0")), + true)); + delayed_cache_.delayed_cb_(); + ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); + filter.onDestroy(); + } +} + TEST_F(CacheFilterTest, ImmediateHitBody) { request_headers_.setHost("ImmediateHitBody"); ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(ReturnRef(context_.dispatcher_)); @@ -77,7 +150,7 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { { // Create filter for request 1 - CacheFilter filter = makeFilter(); + CacheFilter filter = makeFilter(simple_cache_); // Decode request 1 header EXPECT_EQ(filter.decodeHeaders(request_headers_, true), Http::FilterHeadersStatus::Continue); @@ -91,7 +164,7 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { } { // Create filter for request 2 - CacheFilter filter = makeFilter(); + CacheFilter filter = makeFilter(simple_cache_); // Decode request 2 header EXPECT_CALL(decoder_callbacks_, From ee9920461fb2364db72f67bc0db057ae3c19486e Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 20 Feb 2020 10:55:33 -0800 Subject: [PATCH 14/17] Use StopIterationAndWatermark instead of StopIteration, per review comment. Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/cache_filter.cc | 4 ++-- test/extensions/filters/http/cache/cache_filter_test.cc | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index 1beb5538667c3..d4f7e6aae0ab5 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -62,7 +62,7 @@ Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& hea // onHeaders hasn't been called yet--stop iteration to wait for it, and tell it that we stopped // iteration. state_ = GetHeadersState::FinishedGetHeadersCall; - return Http::FilterHeadersStatus::StopIteration; + return Http::FilterHeadersStatus::StopAllIterationAndWatermark; } Http::FilterHeadersStatus CacheFilter::encodeHeaders(Http::ResponseHeaderMap& headers, @@ -93,7 +93,7 @@ void CacheFilter::onHeaders(LookupResult&& result) { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; // We don't yet return or support these codes. case CacheEntryStatus::Unusable: if (state_ == GetHeadersState::FinishedGetHeadersCall) { - // decodeHeader returned Http::FilterHeadersStatus::StopIteration--restart it + // decodeHeader returned Http::FilterHeadersStatus::StopAllIterationAndWatermark--restart it decoder_callbacks_->continueDecoding(); } else { // decodeHeader hasn't yet returned--tell it to return Http::FilterHeadersStatus::Continue. diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 675aad4d344c6..94c9bee4e38a1 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -99,7 +99,7 @@ TEST_F(CacheFilterTest, ImmediateHitNoBody) { HeaderHasValueRef("age", "0")), true)); EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); + Http::FilterHeadersStatus::StopAllIterationAndWatermark); ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); filter.onDestroy(); } @@ -116,7 +116,7 @@ TEST_F(CacheFilterTest, DelayedHitNoBody) { // Decode request 1 header EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); + Http::FilterHeadersStatus::StopAllIterationAndWatermark); EXPECT_CALL(decoder_callbacks_, continueDecoding); delayed_cache_.delayed_cb_(); ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); @@ -131,7 +131,7 @@ TEST_F(CacheFilterTest, DelayedHitNoBody) { // Decode request 2 header EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); + Http::FilterHeadersStatus::StopAllIterationAndWatermark); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_), HeaderHasValueRef("age", "0")), @@ -175,7 +175,7 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { decoder_callbacks_, encodeData(testing::Property(&Buffer::Instance::toString, testing::Eq(body)), true)); EXPECT_EQ(filter.decodeHeaders(request_headers_, true), - Http::FilterHeadersStatus::StopIteration); + Http::FilterHeadersStatus::StopAllIterationAndWatermark); ::testing::Mock::VerifyAndClearExpectations(&decoder_callbacks_); filter.onDestroy(); } From 1b9a17f239b0a659eb1b60944455cfb4a5086713 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 20 Feb 2020 11:36:36 -0800 Subject: [PATCH 15/17] Add factory and integration test. Signed-off-by: Todd Greer --- source/extensions/filters/http/cache/BUILD | 9 ++ .../extensions/filters/http/cache/config.cc | 34 ++++++++ source/extensions/filters/http/cache/config.h | 28 +++++++ test/extensions/filters/http/cache/BUILD | 29 +++++++ .../cache/cache_filter_integration_test.cc | 84 +++++++++++++++++++ .../filters/http/cache/config_test.cc | 49 +++++++++++ 6 files changed, 233 insertions(+) create mode 100644 source/extensions/filters/http/cache/config.cc create mode 100644 source/extensions/filters/http/cache/config.h create mode 100644 test/extensions/filters/http/cache/cache_filter_integration_test.cc create mode 100644 test/extensions/filters/http/cache/config_test.cc diff --git a/source/extensions/filters/http/cache/BUILD b/source/extensions/filters/http/cache/BUILD index 4be1e0c676428..03c1c4932fefb 100644 --- a/source/extensions/filters/http/cache/BUILD +++ b/source/extensions/filters/http/cache/BUILD @@ -20,6 +20,7 @@ envoy_cc_library( ":http_cache_lib", "//source/common/common:logger_lib", "//source/common/common:macros", + "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//envoy/extensions/filters/http/cache/v3alpha:pkg_cc_proto", @@ -62,6 +63,14 @@ envoy_cc_library( envoy_cc_extension( name = "config", + srcs = ["config.cc"], + hdrs = ["config.h"], security_posture = "robust_to_untrusted_downstream_and_upstream", status = "wip", + deps = [ + ":cache_filter_lib", + "//source/extensions/filters/http:well_known_names", + "//source/extensions/filters/http/common:factory_base_lib", + "@envoy_api//envoy/extensions/filters/http/cache/v3alpha:pkg_cc_proto", + ], ) diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc new file mode 100644 index 0000000000000..86277189a789b --- /dev/null +++ b/source/extensions/filters/http/cache/config.cc @@ -0,0 +1,34 @@ +#include "extensions/filters/http/cache/config.h" + +#include "extensions/filters/http/cache/cache_filter.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { + +Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( + const envoy::extensions::filters::http::cache::v3alpha::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)); + } + + return [config, stats_prefix, &context, + http_cache_factory](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared(config, stats_prefix, context.scope(), + context.timeSource(), + http_cache_factory->getCache(config))); + }; +} + +REGISTER_FACTORY(CacheFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory); + +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/cache/config.h b/source/extensions/filters/http/cache/config.h new file mode 100644 index 0000000000000..656a1d68ca959 --- /dev/null +++ b/source/extensions/filters/http/cache/config.h @@ -0,0 +1,28 @@ +#pragma once + +#include "envoy/extensions/filters/http/cache/v3alpha/cache.pb.h" +#include "envoy/extensions/filters/http/cache/v3alpha/cache.pb.validate.h" + +#include "extensions/filters/http/common/factory_base.h" +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { + +class CacheFilterFactory + : public Common::FactoryBase { +public: + CacheFilterFactory() : FactoryBase(HttpFilterNames::get().Cache) {} + +private: + Http::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, + const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override; +}; + +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/cache/BUILD b/test/extensions/filters/http/cache/BUILD index 3a53a12ac9eb6..faa47807b25f7 100644 --- a/test/extensions/filters/http/cache/BUILD +++ b/test/extensions/filters/http/cache/BUILD @@ -26,6 +26,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.http.cache", deps = [ "//source/extensions/filters/http/cache:http_cache_lib", + "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib", "//test/mocks/http:http_mocks", "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", @@ -44,3 +45,31 @@ envoy_extension_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_extension_cc_test( + name = "config_test", + srcs = ["config_test.cc"], + extension_name = "envoy.filters.http.cache", + deps = [ + "//source/extensions/filters/http/cache:config", + "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib", + "//test/mocks/http:http_mocks", + "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", + ], +) + +envoy_extension_cc_test( + name = "cache_filter_integration_test", + srcs = [ + "cache_filter_integration_test.cc", + ], + extension_name = "envoy.filters.http.cache", + deps = [ + "//source/extensions/filters/http/cache:config", + "//source/extensions/filters/http/cache:http_cache_lib", + "//source/extensions/filters/http/cache/simple_http_cache:simple_http_cache_lib", + "//test/integration:http_protocol_integration_lib", + "//test/test_common:simulated_time_system_lib", + ], +) diff --git a/test/extensions/filters/http/cache/cache_filter_integration_test.cc b/test/extensions/filters/http/cache/cache_filter_integration_test.cc new file mode 100644 index 0000000000000..e5ba09e88332b --- /dev/null +++ b/test/extensions/filters/http/cache/cache_filter_integration_test.cc @@ -0,0 +1,84 @@ +#include "test/integration/http_protocol_integration.h" +#include "test/test_common/simulated_time_system.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { + +// TODO(toddmgreer): Expand integration test to include age header values, +// expiration, range headers, HEAD requests, trailers, config customizations, +// cache-control headers, and conditional header fields, as they are +// implemented. + +class CacheIntegrationTest : public Event::TestUsingSimulatedTime, + public HttpProtocolIntegrationTest { +public: + void TearDown() override { + cleanupUpstreamAndDownstream(); + HttpProtocolIntegrationTest::TearDown(); + } + + void initializeFilter(const std::string& config) { + config_helper_.addFilter(config); + initialize(); + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + } + + const std::string default_config{R"EOF( + name: "envoy.filters.http.cache" + typed_config: + "@type": "type.googleapis.com/envoy.extensions.filters.http.cache.v3alpha.CacheConfig" + typed_config: + "@type": "type.googleapis.com/envoy.source.extensions.filters.http.cache.SimpleHttpCacheConfig" + )EOF"}; + DateFormatter formatter_{"%a, %d %b %Y %H:%M:%S GMT"}; +}; + +INSTANTIATE_TEST_SUITE_P(Protocols, CacheIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +TEST_P(CacheIntegrationTest, MissInsertHit) { + // Set system time to cause Envoy's cached formatted time to match time on this thread. + simTime().setSystemTime(std::chrono::hours(1)); + initializeFilter(default_config); + + // Include test name and params in URL to make each test's requests unique. + const Http::TestRequestHeaderMapImpl request_headers = { + {":method", "GET"}, + {":path", absl::StrCat("/", protocolTestParamsToString({GetParam(), 0}))}, + {":scheme", "http"}, + {":authority", "MissInsertHit"}}; + Http::TestResponseHeaderMapImpl response_headers = {{":status", "200"}, + {"date", formatter_.now(simTime())}, + {"cache-control", "public,max-age=3600"}, + {"content-length", "42"}}; + + // Send first request, and get response from upstream. + { + IntegrationStreamDecoderPtr request = codec_client_->makeHeaderOnlyRequest(request_headers); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(response_headers, /*end_stream=*/false); + // send 42 'a's + upstream_request_->encodeData(42, true); + // Wait for the response to be read by the codec client. + request->waitForEndStream(); + EXPECT_TRUE(request->complete()); + EXPECT_THAT(request->headers(), IsSupersetOfHeaders(response_headers)); + EXPECT_EQ(request->headers().get(Http::Headers::get().Age), nullptr); + EXPECT_EQ(request->body(), std::string(42, 'a')); + } + + // Send second request, and get response from cache. + IntegrationStreamDecoderPtr request = codec_client_->makeHeaderOnlyRequest(request_headers); + request->waitForEndStream(); + EXPECT_TRUE(request->complete()); + EXPECT_THAT(request->headers(), IsSupersetOfHeaders(response_headers)); + EXPECT_EQ(request->body(), std::string(42, 'a')); + EXPECT_NE(request->headers().get(Http::Headers::get().Age), nullptr); +} +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/cache/config_test.cc b/test/extensions/filters/http/cache/config_test.cc new file mode 100644 index 0000000000000..c314897c33a4e --- /dev/null +++ b/test/extensions/filters/http/cache/config_test.cc @@ -0,0 +1,49 @@ +#include "source/extensions/filters/http/cache/simple_http_cache/config.pb.h" + +#include "extensions/filters/http/cache/cache_filter.h" +#include "extensions/filters/http/cache/config.h" + +#include "test/mocks/server/mocks.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Cache { +namespace { + +class CacheFilterFactoryTest : public ::testing::Test { +protected: + envoy::extensions::filters::http::cache::v3alpha::CacheConfig config_; + NiceMock context_; + CacheFilterFactory factory_; + Http::MockFilterChainFactoryCallbacks filter_callback_; +}; + +TEST_F(CacheFilterFactoryTest, Basic) { + config_.mutable_typed_config()->PackFrom( + envoy::source::extensions::filters::http::cache::SimpleHttpCacheConfig()); + 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); +} + +TEST_F(CacheFilterFactoryTest, UnregisteredTypedConfig) { + config_.mutable_typed_config()->PackFrom( + envoy::extensions::filters::http::cache::v3alpha::CacheConfig()); + EXPECT_THROW(factory_.createFilterFactoryFromProto(config_, "stats", context_), EnvoyException); +} +} // namespace +} // namespace Cache +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy From 590a34a83a8b8bf6502fa96a2350bb06b95412fc Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 20 Feb 2020 19:17:33 -0800 Subject: [PATCH 16/17] Refuse to cache requests with bodies, because they're used for a request smuggling, and the spec doesn't define any meaning for GET/HEAD requests with bodies. Signed-off-by: Todd Greer --- .../filters/http/cache/cache_filter.cc | 10 ++++- .../cache/cache_filter_integration_test.cc | 39 +++++++++++++++++++ .../filters/http/cache/cache_filter_test.cc | 23 ++++++++++- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/http/cache/cache_filter.cc b/source/extensions/filters/http/cache/cache_filter.cc index d4f7e6aae0ab5..ad43c577034e4 100644 --- a/source/extensions/filters/http/cache/cache_filter.cc +++ b/source/extensions/filters/http/cache/cache_filter.cc @@ -42,8 +42,16 @@ void CacheFilter::onDestroy() { insert_ = nullptr; } -Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { +Http::FilterHeadersStatus CacheFilter::decodeHeaders(Http::RequestHeaderMap& headers, + bool end_stream) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders: {}", *decoder_callbacks_, headers); + if (!end_stream) { + ENVOY_STREAM_LOG( + debug, + "CacheFilter::decodeHeaders ignoring request because it has body and/or trailers: {}", + *decoder_callbacks_, headers); + return Http::FilterHeadersStatus::Continue; + } if (!isCacheableRequest(headers)) { ENVOY_STREAM_LOG(debug, "CacheFilter::decodeHeaders ignoring uncacheable request: {}", *decoder_callbacks_, headers); diff --git a/test/extensions/filters/http/cache/cache_filter_integration_test.cc b/test/extensions/filters/http/cache/cache_filter_integration_test.cc index e5ba09e88332b..a645734486467 100644 --- a/test/extensions/filters/http/cache/cache_filter_integration_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_integration_test.cc @@ -78,6 +78,45 @@ TEST_P(CacheIntegrationTest, MissInsertHit) { EXPECT_EQ(request->body(), std::string(42, 'a')); EXPECT_NE(request->headers().get(Http::Headers::get().Age), nullptr); } + +// Send the same GET request twice with body and trailers twice, then check that the response +// doesn't have an age header, to confirm that it wasn't served from cache. +TEST_P(CacheIntegrationTest, GetRequestWithBodyAndTrailers) { + // Set system time to cause Envoy's cached formatted time to match time on this thread. + simTime().setSystemTime(std::chrono::hours(1)); + initializeFilter(default_config); + + // Include test name and params in URL to make each test's requests unique. + const Http::TestRequestHeaderMapImpl request_headers = { + {":method", "GET"}, + {":path", absl::StrCat("/", protocolTestParamsToString({GetParam(), 0}))}, + {":scheme", "http"}, + {":authority", "MissInsertHit"}}; + Http::TestRequestTrailerMapImpl request_trailers{{"request1", "trailer1"}, + {"request2", "trailer2"}}; + Http::TestResponseHeaderMapImpl response_headers = {{":status", "200"}, + {"date", formatter_.now(simTime())}, + {"cache-control", "public,max-age=3600"}, + {"content-length", "42"}}; + + for (int i = 0; i < 2; ++i) { + auto encoder_decoder = codec_client_->startRequest(request_headers); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 13, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(response_headers, /*end_stream=*/false); + // send 42 'a's + upstream_request_->encodeData(42, true); + // Wait for the response to be read by the codec client. + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_THAT(response->headers(), IsSupersetOfHeaders(response_headers)); + EXPECT_EQ(response->headers().get(Http::Headers::get().Age), nullptr); + EXPECT_EQ(response->body(), std::string(42, 'a')); + } +} } // namespace Cache } // namespace HttpFilters } // namespace Extensions diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index 94c9bee4e38a1..fd3aeb0b62195 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -69,8 +69,8 @@ class CacheFilterTest : public ::testing::Test { Http::TestResponseHeaderMapImpl response_headers_{{":status", "200"}, {"date", formatter_.now(time_source_)}, {"cache-control", "public,max-age=3600"}}; - NiceMock decoder_callbacks_; - NiceMock encoder_callbacks_; + Http::MockStreamDecoderFilterCallbacks decoder_callbacks_; + Http::MockStreamEncoderFilterCallbacks encoder_callbacks_; }; TEST_F(CacheFilterTest, ImmediateHitNoBody) { @@ -181,6 +181,25 @@ TEST_F(CacheFilterTest, ImmediateHitBody) { } } +// Send two identical GET requests with bodies. The CacheFilter will just pass everything through. +TEST_F(CacheFilterTest, GetRequestWithBodyAndTrailers) { + request_headers_.setHost("GetRequestWithBodyAndTrailers"); + const std::string body = "abc"; + Buffer::OwnedImpl request_buffer(body); + Http::TestRequestTrailerMapImpl request_trailers; + + for (int i = 0; i < 2; ++i) { + CacheFilter filter = makeFilter(simple_cache_); + + EXPECT_EQ(filter.decodeHeaders(request_headers_, false), Http::FilterHeadersStatus::Continue); + EXPECT_EQ(filter.decodeData(request_buffer, false), Http::FilterDataStatus::Continue); + EXPECT_EQ(filter.decodeTrailers(request_trailers), Http::FilterTrailersStatus::Continue); + + EXPECT_EQ(filter.encodeHeaders(response_headers_, true), Http::FilterHeadersStatus::Continue); + filter.onDestroy(); + } +} + } // namespace } // namespace Cache } // namespace HttpFilters From b9ad06b65d55b8d20f2e994d282821108dea8c63 Mon Sep 17 00:00:00 2001 From: Todd Greer Date: Thu, 20 Feb 2020 23:20:17 -0800 Subject: [PATCH 17/17] Restore NiceMocks Signed-off-by: Todd Greer --- test/extensions/filters/http/cache/cache_filter_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index fd3aeb0b62195..99d79a1e478a0 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -69,8 +69,8 @@ class CacheFilterTest : public ::testing::Test { Http::TestResponseHeaderMapImpl response_headers_{{":status", "200"}, {"date", formatter_.now(time_source_)}, {"cache-control", "public,max-age=3600"}}; - Http::MockStreamDecoderFilterCallbacks decoder_callbacks_; - Http::MockStreamEncoderFilterCallbacks encoder_callbacks_; + NiceMock decoder_callbacks_; + NiceMock encoder_callbacks_; }; TEST_F(CacheFilterTest, ImmediateHitNoBody) {