From 6bd5c2d0ebd07019618c3f80ea24b86de31c115a Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 8 Nov 2021 10:31:40 -0500 Subject: [PATCH 1/6] dfp: adding timing information about DNS resolution Signed-off-by: Alyssa Wilk --- source/common/stream_info/BUILD | 8 +++ .../common/stream_info/uint64_accessor_impl.h | 28 ++++++++++ .../filters/http/dynamic_forward_proxy/BUILD | 1 + .../dynamic_forward_proxy/proxy_filter.cc | 18 +++++++ .../http/dynamic_forward_proxy/proxy_filter.h | 7 +++ .../filters/http/dynamic_forward_proxy/BUILD | 1 + .../proxy_filter_integration_test.cc | 10 ++++ .../proxy_filter_test.cc | 46 ++++++++++++++++- test/integration/filters/BUILD | 16 ++++++ .../filters/stream_info_to_headers_filter.cc | 51 +++++++++++++++++++ 10 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 source/common/stream_info/uint64_accessor_impl.h create mode 100644 test/integration/filters/stream_info_to_headers_filter.cc diff --git a/source/common/stream_info/BUILD b/source/common/stream_info/BUILD index 60f7ac62f3c02..a48d79a708bfe 100644 --- a/source/common/stream_info/BUILD +++ b/source/common/stream_info/BUILD @@ -50,6 +50,14 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "uint64_accessor_lib", + hdrs = ["uint64_accessor_impl.h"], + deps = [ + "//envoy/stream_info:stream_info_interface", + ], +) + envoy_cc_library( name = "upstream_address_lib", hdrs = ["upstream_address.h"], diff --git a/source/common/stream_info/uint64_accessor_impl.h b/source/common/stream_info/uint64_accessor_impl.h new file mode 100644 index 0000000000000..639193e26b2eb --- /dev/null +++ b/source/common/stream_info/uint64_accessor_impl.h @@ -0,0 +1,28 @@ +#pragma once + +#include "envoy/stream_info/filter_state.h" + +namespace Envoy { +namespace StreamInfo { + +/* + * A FilterState object that tracks a single uint64_t value. + */ +class UInt64AccessorImpl : public FilterState::Object { +public: + UInt64AccessorImpl(uint32_t value) : value_(value) {} + + // From FilterState::Object + ProtobufTypes::MessagePtr serializeAsProto() const override { + auto message = std::make_unique(); + message->set_value(value_); + return message; + } + uint64_t value() const { return value_; } + +private: + uint64_t value_; +}; + +} // namespace StreamInfo +} // namespace Envoy diff --git a/source/extensions/filters/http/dynamic_forward_proxy/BUILD b/source/extensions/filters/http/dynamic_forward_proxy/BUILD index 1bc118a341f8b..8c7daccc62cdf 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/BUILD +++ b/source/extensions/filters/http/dynamic_forward_proxy/BUILD @@ -16,6 +16,7 @@ envoy_cc_library( deps = [ "//envoy/http:filter_interface", "//source/common/http:header_utility_lib", + "//source/common/stream_info:uint64_accessor_lib", "//source/common/stream_info:upstream_address_lib", "//source/extensions/common/dynamic_forward_proxy:dns_cache_interface", "//source/extensions/filters/http/common:pass_through_filter_lib", diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index a25f157ea9eae..ad0190189ed9f 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -5,6 +5,7 @@ #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h" #include "source/common/http/utility.h" +#include "source/common/stream_info/uint64_accessor_impl.h" #include "source/common/stream_info/upstream_address.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache.h" @@ -12,7 +13,20 @@ namespace Envoy { namespace Extensions { namespace HttpFilters { namespace DynamicForwardProxy { +namespace { +void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, const std::string& key) { + const Envoy::StreamInfo::FilterStateSharedPtr& filter_state = + decoder_callbacks->streamInfo().filterState(); + auto timing = std::make_unique( + std::chrono::duration_cast( + decoder_callbacks->dispatcher().timeSource().monotonicTime().time_since_epoch()) + .count()); + filter_state->setData(key, std::move(timing), StreamInfo::FilterState::StateType::Mutable, + StreamInfo::FilterState::LifeSpan::Request); +} + +} // namespace struct ResponseStringValues { const std::string DnsCacheOverflow = "DNS cache overflow"; const std::string PendingRequestOverflow = "Dynamic forward proxy pending request overflow"; @@ -46,6 +60,8 @@ void ProxyFilter::onDestroy() { } Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { + latchTime(decoder_callbacks_, dnsStart()); + Router::RouteConstSharedPtr route = decoder_callbacks_->route(); const Router::RouteEntry* route_entry; if (!route || !(route_entry = route->routeEntry())) { @@ -132,6 +148,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea addHostAddressToFilterState(host.value()->address()); } + latchTime(decoder_callbacks_, dnsEnd()); return Http::FilterHeadersStatus::Continue; } case LoadDnsCacheEntryStatus::Loading: @@ -183,6 +200,7 @@ void ProxyFilter::onLoadDnsCacheComplete( const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { ENVOY_STREAM_LOG(debug, "load DNS cache complete, continuing after adding resolved host: {}", *decoder_callbacks_, host_info->resolvedHost()); + latchTime(decoder_callbacks_, dnsEnd()); ASSERT(circuit_breaker_ != nullptr); circuit_breaker_.reset(); diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h index a0920e553c704..effe11e5aa679 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h @@ -51,6 +51,13 @@ class ProxyFilter public: ProxyFilter(const ProxyFilterConfigSharedPtr& config) : config_(config) {} + static const std::string& dnsStart() { + CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_start_ms"); + } + static const std::string& dnsEnd() { + CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_end_ms"); + } + // Http::PassThroughDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) override; diff --git a/test/extensions/filters/http/dynamic_forward_proxy/BUILD b/test/extensions/filters/http/dynamic_forward_proxy/BUILD index ec2a0c9534eda..1dcafd08831a5 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/BUILD +++ b/test/extensions/filters/http/dynamic_forward_proxy/BUILD @@ -57,6 +57,7 @@ envoy_extension_cc_test( "//source/extensions/filters/http/dynamic_forward_proxy:config", "//source/extensions/key_value/file_based:config_lib", "//test/integration:http_integration_lib", + "//test/integration/filters:stream_info_to_headers_filter_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc index c138d28fbb59b..555a419b208b4 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc @@ -43,6 +43,10 @@ name: dynamic_forward_proxy max_hosts, max_pending_requests, filename); config_helper_.prependFilter(filter); + config_helper_.prependFilter(fmt::format(R"EOF( +name: stream-info-to-headers-filter +typed_config: + "@type": type.googleapis.com/google.protobuf.Empty)EOF")); config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { // Switch predefined cluster_0 to CDS filesystem sourcing. bootstrap.mutable_dynamic_resources()->mutable_cds_config()->set_resource_api_version( @@ -174,12 +178,18 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithBody) { checkSimpleRequestSuccess(1024, 1024, response.get()); EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value()); EXPECT_EQ(1, test_server_->counter("dns_cache.foo.host_added")->value()); + // Make sure dns timings are tracked for cache-misses. + ASSERT_FALSE(response->headers().get(Http::LowerCaseString("dns_start")).empty()); + ASSERT_FALSE(response->headers().get(Http::LowerCaseString("dns_end")).empty()); // Now send another request. This should hit the DNS cache. response = sendRequestAndWaitForResponse(request_headers, 512, default_response_headers_, 512); checkSimpleRequestSuccess(512, 512, response.get()); EXPECT_EQ(1, test_server_->counter("dns_cache.foo.dns_query_attempt")->value()); EXPECT_EQ(1, test_server_->counter("dns_cache.foo.host_added")->value()); + // Make sure dns timings are tracked for cache-hits. + ASSERT_FALSE(response->headers().get(Http::LowerCaseString("dns_start")).empty()); + ASSERT_FALSE(response->headers().get(Http::LowerCaseString("dns_end")).empty()); } // Currently if the first DNS resolution fails, the filter will continue with diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc index c2150e500dd91..82772c157d43d 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc @@ -1,6 +1,7 @@ #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h" +#include "source/common/stream_info/uint64_accessor_impl.h" #include "source/common/stream_info/upstream_address.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h" #include "source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h" @@ -98,11 +99,14 @@ TEST_F(ProxyFilterTest, HttpDefaultPort) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false)); + Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle* handle = new Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle(); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 80, _)) @@ -121,6 +125,8 @@ TEST_F(ProxyFilterTest, HttpsDefaultPort) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -144,6 +150,8 @@ TEST_F(ProxyFilterTest, CacheOverflow) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -168,6 +176,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflow) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -184,6 +194,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflow) { // Create a second filter for a 2nd request. auto filter2 = std::make_unique(filter_config_); filter2->setDecoderFilterCallbacks(callbacks_); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()); @@ -206,6 +218,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -222,6 +236,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { // Create a second filter for a 2nd request. auto filter2 = std::make_unique(filter_config_); filter2->setDecoderFilterCallbacks(callbacks_); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()); @@ -245,6 +261,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { TEST_F(ProxyFilterTest, NoRoute) { InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()).WillOnce(Return(nullptr)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); } @@ -253,6 +271,8 @@ TEST_F(ProxyFilterTest, NoRoute) { TEST_F(ProxyFilterTest, NoCluster) { InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)).WillOnce(Return(nullptr)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -264,6 +284,8 @@ TEST_F(ProxyFilterTest, NoClusterType) { InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -277,6 +299,8 @@ TEST_F(ProxyFilterTest, NonDynamicForwardProxy) { InSequence s; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -291,6 +315,8 @@ TEST_F(ProxyFilterTest, HostRewrite) { proto_config.set_host_rewrite_literal("bar"); ProxyPerRouteConfig config(proto_config); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -320,6 +346,8 @@ TEST_F(ProxyFilterTest, HostRewriteViaHeader) { proto_config.set_host_rewrite_header("x-set-header"); ProxyPerRouteConfig config(proto_config); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -372,6 +400,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata auto host_info = std::make_shared(); host_info->address_ = Network::Utility::parseInternetAddress("1.2.3.4", 80); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -393,6 +423,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata EXPECT_CALL(*host_info, address()); EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); // Host was resolved successfully, so continue filter iteration. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -427,6 +459,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad auto host_info = std::make_shared(); host_info->address_ = Network::Utility::parseInternetAddress("1.2.3.4", 80); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -448,13 +482,19 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad EXPECT_CALL(*host_info, address()); EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); // Host was resolved successfully, so continue filter iteration. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); - // We expect FilterState to be populated + // We expect FilterState and resolution times to be populated EXPECT_TRUE( filter_state->hasData(StreamInfo::UpstreamAddress::key())); + EXPECT_TRUE(filter_state->hasData( + "envoy.dynamic_forward_proxy.dns_start_ms")); + EXPECT_TRUE(filter_state->hasData( + "envoy.dynamic_forward_proxy.dns_end_ms")); const StreamInfo::UpstreamAddress& updated_address_obj = filter_state->getDataReadOnly( @@ -482,6 +522,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre auto host_info = std::make_shared(); host_info->address_ = nullptr; + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -501,6 +543,8 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre })); EXPECT_CALL(*host_info, address()); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 1011e3c74776e..c9f922fa7aec4 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -635,3 +635,19 @@ envoy_cc_test_library( "//test/extensions/filters/http/common:empty_http_filter_config_lib", ], ) + +envoy_cc_test_library( + name = "stream_info_to_headers_filter_lib", + srcs = [ + "stream_info_to_headers_filter.cc", + ], + deps = [ + ":common_lib", + "//envoy/http:filter_interface", + "//envoy/registry", + "//envoy/server:filter_config_interface", + "//source/common/stream_info:uint64_accessor_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) diff --git a/test/integration/filters/stream_info_to_headers_filter.cc b/test/integration/filters/stream_info_to_headers_filter.cc new file mode 100644 index 0000000000000..723e2fa468caf --- /dev/null +++ b/test/integration/filters/stream_info_to_headers_filter.cc @@ -0,0 +1,51 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/common/stream_info/uint64_accessor_impl.h" +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "test/integration/filters/common.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +using StreamInfo::UInt64AccessorImpl; + +// A filter that sticks stream info into headers for integration testing. +class StreamInfoToHeadersFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "stream-info-to-headers-filter"; + + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { + return Http::FilterHeadersStatus::Continue; + } + + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override { + const std::string dns_start = "envoy.dynamic_forward_proxy.dns_start_ms"; + const std::string dns_end = "envoy.dynamic_forward_proxy.dns_end_ms"; + StreamInfo::StreamInfo& stream_info = decoder_callbacks_->streamInfo(); + + if (stream_info.filterState()->hasData(dns_start)) { + headers.addCopy( + Http::LowerCaseString("dns_start"), + absl::StrCat( + stream_info.filterState()->getDataReadOnly(dns_start).value())); + } + if (stream_info.filterState()->hasData(dns_end)) { + headers.addCopy( + Http::LowerCaseString("dns_end"), + absl::StrCat( + stream_info.filterState()->getDataReadOnly(dns_end).value())); + } + return Http::FilterHeadersStatus::Continue; + } +}; + +constexpr char StreamInfoToHeadersFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy From 18e4576e77c6c2d1dabdc52538765deb9b09eb46 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 8 Nov 2021 13:36:33 -0500 Subject: [PATCH 2/6] coverage Signed-off-by: Alyssa Wilk --- test/common/stream_info/BUILD | 8 +++++++ .../stream_info/uint64_accessor_impl_test.cc | 23 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 test/common/stream_info/uint64_accessor_impl_test.cc diff --git a/test/common/stream_info/BUILD b/test/common/stream_info/BUILD index 92d905fa1c828..77315beda13e2 100644 --- a/test/common/stream_info/BUILD +++ b/test/common/stream_info/BUILD @@ -74,3 +74,11 @@ envoy_cc_test( "//source/common/stream_info:uint32_accessor_lib", ], ) + +envoy_cc_test( + name = "uint64_accessor_impl_test", + srcs = ["uint64_accessor_impl_test.cc"], + deps = [ + "//source/common/stream_info:uint64_accessor_lib", + ], +) diff --git a/test/common/stream_info/uint64_accessor_impl_test.cc b/test/common/stream_info/uint64_accessor_impl_test.cc new file mode 100644 index 0000000000000..a9e6e868c1e8d --- /dev/null +++ b/test/common/stream_info/uint64_accessor_impl_test.cc @@ -0,0 +1,23 @@ +#include "source/common/stream_info/uint64_accessor_impl.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace StreamInfo { +namespace { + +TEST(UInt64AccessorImplTest, ConstructorInitsValue) { + uint64_t init_value = 0xdeadbeef; + UInt64AccessorImpl accessor(init_value); + EXPECT_EQ(init_value, accessor.value()); +} + +TEST(UInt64AccessorImplTest, DebugString) { + uint64_t init_value = 123; + UInt64AccessorImpl accessor(init_value); + EXPECT_EQ("value: 123\n", accessor.serializeAsProto()->DebugString()); +} + +} // namespace +} // namespace StreamInfo +} // namespace Envoy From b930d827c4249077d4cc948dade09f57ef0ca9d8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 9 Nov 2021 14:46:09 -0500 Subject: [PATCH 3/6] rework Signed-off-by: Alyssa Wilk --- envoy/stream_info/stream_info.h | 23 +++++++++++++++ source/common/stream_info/BUILD | 8 ------ source/common/stream_info/stream_info_impl.h | 8 ++++++ .../common/stream_info/uint64_accessor_impl.h | 28 ------------------- .../filters/http/dynamic_forward_proxy/BUILD | 1 - .../dynamic_forward_proxy/proxy_filter.cc | 12 ++------ test/common/stream_info/BUILD | 8 ------ test/common/stream_info/test_util.h | 3 ++ .../stream_info/uint64_accessor_impl_test.cc | 23 --------------- .../proxy_filter_test.cc | 11 +++----- test/integration/filters/BUILD | 1 - .../filters/stream_info_to_headers_filter.cc | 21 +++++++------- test/mocks/stream_info/mocks.cc | 1 + test/mocks/stream_info/mocks.h | 2 ++ 14 files changed, 55 insertions(+), 95 deletions(-) delete mode 100644 source/common/stream_info/uint64_accessor_impl.h delete mode 100644 test/common/stream_info/uint64_accessor_impl_test.cc diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h index 5c4c2b473b611..190b7053c99fb 100644 --- a/envoy/stream_info/stream_info.h +++ b/envoy/stream_info/stream_info.h @@ -238,6 +238,22 @@ struct UpstreamTiming { absl::optional last_upstream_rx_byte_received_; }; +class DownstreamTiming { +public: + void setValue(absl::string_view key, MonotonicTime value) { timings_[key] = value; } + + absl::optional getValue(absl::string_view value) const { + auto ret = timings_.find(value); + if (ret == timings_.end()) { + return {}; + } + return ret->second; + } + +private: + absl::flat_hash_map timings_; +}; + // Measure the number of bytes sent and received for a stream. struct BytesMeter { uint64_t wireBytesSent() const { return wire_bytes_sent_; } @@ -406,6 +422,8 @@ class StreamInfo { */ virtual absl::optional firstDownstreamTxByteSent() const PURE; + // TODO(alyssawilk) move the downstream timing calls into DownstreamTiming in + // a follow-up. /** * Sets the time when the first byte of the response is sent downstream. */ @@ -434,6 +452,11 @@ class StreamInfo { */ virtual void onRequestComplete() PURE; + /** + * @return the downstream timing information. + */ + virtual DownstreamTiming& downstreamTiming() PURE; + /** * @param bytes_sent denotes the number of bytes to add to total sent bytes. */ diff --git a/source/common/stream_info/BUILD b/source/common/stream_info/BUILD index a48d79a708bfe..60f7ac62f3c02 100644 --- a/source/common/stream_info/BUILD +++ b/source/common/stream_info/BUILD @@ -50,14 +50,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "uint64_accessor_lib", - hdrs = ["uint64_accessor_impl.h"], - deps = [ - "//envoy/stream_info:stream_info_interface", - ], -) - envoy_cc_library( name = "upstream_address_lib", hdrs = ["upstream_address.h"], diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 403ab403154bf..d0ee1a3ab1cc0 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -131,6 +131,13 @@ struct StreamInfoImpl : public StreamInfo { final_time_ = time_source_.monotonicTime(); } + DownstreamTiming& downstreamTiming() override { + if (!downstream_timing_.has_value()) { + downstream_timing_ = DownstreamTiming(); + } + return downstream_timing_.value(); + } + void addBytesReceived(uint64_t bytes_received) override { bytes_received_ += bytes_received; } uint64_t bytesReceived() const override { return bytes_received_; } @@ -360,6 +367,7 @@ struct StreamInfoImpl : public StreamInfo { std::string requested_server_name_; const Http::RequestHeaderMap* request_headers_{}; Http::RequestIdStreamInfoProviderSharedPtr request_id_provider_; + absl::optional downstream_timing_; UpstreamTiming upstream_timing_; std::string upstream_transport_failure_reason_; absl::optional upstream_cluster_info_; diff --git a/source/common/stream_info/uint64_accessor_impl.h b/source/common/stream_info/uint64_accessor_impl.h deleted file mode 100644 index 639193e26b2eb..0000000000000 --- a/source/common/stream_info/uint64_accessor_impl.h +++ /dev/null @@ -1,28 +0,0 @@ -#pragma once - -#include "envoy/stream_info/filter_state.h" - -namespace Envoy { -namespace StreamInfo { - -/* - * A FilterState object that tracks a single uint64_t value. - */ -class UInt64AccessorImpl : public FilterState::Object { -public: - UInt64AccessorImpl(uint32_t value) : value_(value) {} - - // From FilterState::Object - ProtobufTypes::MessagePtr serializeAsProto() const override { - auto message = std::make_unique(); - message->set_value(value_); - return message; - } - uint64_t value() const { return value_; } - -private: - uint64_t value_; -}; - -} // namespace StreamInfo -} // namespace Envoy diff --git a/source/extensions/filters/http/dynamic_forward_proxy/BUILD b/source/extensions/filters/http/dynamic_forward_proxy/BUILD index 8c7daccc62cdf..1bc118a341f8b 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/BUILD +++ b/source/extensions/filters/http/dynamic_forward_proxy/BUILD @@ -16,7 +16,6 @@ envoy_cc_library( deps = [ "//envoy/http:filter_interface", "//source/common/http:header_utility_lib", - "//source/common/stream_info:uint64_accessor_lib", "//source/common/stream_info:upstream_address_lib", "//source/extensions/common/dynamic_forward_proxy:dns_cache_interface", "//source/extensions/filters/http/common:pass_through_filter_lib", diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index ad0190189ed9f..b73f78151dc03 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -5,7 +5,6 @@ #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h" #include "source/common/http/utility.h" -#include "source/common/stream_info/uint64_accessor_impl.h" #include "source/common/stream_info/upstream_address.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache.h" @@ -16,14 +15,9 @@ namespace DynamicForwardProxy { namespace { void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, const std::string& key) { - const Envoy::StreamInfo::FilterStateSharedPtr& filter_state = - decoder_callbacks->streamInfo().filterState(); - auto timing = std::make_unique( - std::chrono::duration_cast( - decoder_callbacks->dispatcher().timeSource().monotonicTime().time_since_epoch()) - .count()); - filter_state->setData(key, std::move(timing), StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::Request); + StreamInfo::DownstreamTiming& downstream_timing = + decoder_callbacks->streamInfo().downstreamTiming(); + downstream_timing.setValue(key, decoder_callbacks->dispatcher().timeSource().monotonicTime()); } } // namespace diff --git a/test/common/stream_info/BUILD b/test/common/stream_info/BUILD index 77315beda13e2..92d905fa1c828 100644 --- a/test/common/stream_info/BUILD +++ b/test/common/stream_info/BUILD @@ -74,11 +74,3 @@ envoy_cc_test( "//source/common/stream_info:uint32_accessor_lib", ], ) - -envoy_cc_test( - name = "uint64_accessor_impl_test", - srcs = ["uint64_accessor_impl_test.cc"], - deps = [ - "//source/common/stream_info:uint64_accessor_lib", - ], -) diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 2cebca68cd1a1..a9cfaa47feb33 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -144,6 +144,8 @@ class TestStreamInfo : public StreamInfo::StreamInfo { void onRequestComplete() override { end_time_ = timeSystem().monotonicTime(); } + Envoy::StreamInfo::DownstreamTiming& downstreamTiming() override { return downstream_timing_; } + void setUpstreamTiming(const Envoy::StreamInfo::UpstreamTiming& upstream_timing) override { upstream_timing_ = upstream_timing; } @@ -241,6 +243,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { SystemTime start_time_; MonotonicTime start_time_monotonic_; + Envoy::StreamInfo::DownstreamTiming downstream_timing_; absl::optional last_rx_byte_received_; absl::optional first_upstream_tx_byte_sent_; absl::optional last_upstream_tx_byte_sent_; diff --git a/test/common/stream_info/uint64_accessor_impl_test.cc b/test/common/stream_info/uint64_accessor_impl_test.cc deleted file mode 100644 index a9e6e868c1e8d..0000000000000 --- a/test/common/stream_info/uint64_accessor_impl_test.cc +++ /dev/null @@ -1,23 +0,0 @@ -#include "source/common/stream_info/uint64_accessor_impl.h" - -#include "gtest/gtest.h" - -namespace Envoy { -namespace StreamInfo { -namespace { - -TEST(UInt64AccessorImplTest, ConstructorInitsValue) { - uint64_t init_value = 0xdeadbeef; - UInt64AccessorImpl accessor(init_value); - EXPECT_EQ(init_value, accessor.value()); -} - -TEST(UInt64AccessorImplTest, DebugString) { - uint64_t init_value = 123; - UInt64AccessorImpl accessor(init_value); - EXPECT_EQ("value: 123\n", accessor.serializeAsProto()->DebugString()); -} - -} // namespace -} // namespace StreamInfo -} // namespace Envoy diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc index 82772c157d43d..322b0a70283c1 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc @@ -1,7 +1,6 @@ #include "envoy/config/cluster/v3/cluster.pb.h" #include "envoy/extensions/filters/http/dynamic_forward_proxy/v3/dynamic_forward_proxy.pb.h" -#include "source/common/stream_info/uint64_accessor_impl.h" #include "source/common/stream_info/upstream_address.h" #include "source/extensions/common/dynamic_forward_proxy/dns_cache_impl.h" #include "source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h" @@ -442,7 +441,7 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad Upstream::ResourceAutoIncDec* circuit_breakers_( new Upstream::ResourceAutoIncDec(pending_requests_)); - EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, streamInfo()).Times(testing::AnyNumber()); // Pre-populate the filter state with an address. auto& filter_state = callbacks_.streamInfo().filterState(); @@ -490,11 +489,9 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad // We expect FilterState and resolution times to be populated EXPECT_TRUE( - filter_state->hasData(StreamInfo::UpstreamAddress::key())); - EXPECT_TRUE(filter_state->hasData( - "envoy.dynamic_forward_proxy.dns_start_ms")); - EXPECT_TRUE(filter_state->hasData( - "envoy.dynamic_forward_proxy.dns_end_ms")); + callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::dnsStart()).has_value()); + EXPECT_TRUE( + callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::dnsEnd()).has_value()); const StreamInfo::UpstreamAddress& updated_address_obj = filter_state->getDataReadOnly( diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index c9f922fa7aec4..9ebfa7cb928aa 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -646,7 +646,6 @@ envoy_cc_test_library( "//envoy/http:filter_interface", "//envoy/registry", "//envoy/server:filter_config_interface", - "//source/common/stream_info:uint64_accessor_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "//test/extensions/filters/http/common:empty_http_filter_config_lib", ], diff --git a/test/integration/filters/stream_info_to_headers_filter.cc b/test/integration/filters/stream_info_to_headers_filter.cc index 723e2fa468caf..a29fefcc46c96 100644 --- a/test/integration/filters/stream_info_to_headers_filter.cc +++ b/test/integration/filters/stream_info_to_headers_filter.cc @@ -1,7 +1,6 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" -#include "source/common/stream_info/uint64_accessor_impl.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "test/extensions/filters/http/common/empty_http_filter_config.h" @@ -10,8 +9,13 @@ #include "gtest/gtest.h" namespace Envoy { +namespace { -using StreamInfo::UInt64AccessorImpl; +uint64_t toMs(MonotonicTime time) { + return std::chrono::duration_cast(time.time_since_epoch()).count(); +} + +} // namespace // A filter that sticks stream info into headers for integration testing. class StreamInfoToHeadersFilter : public Http::PassThroughFilter { @@ -27,17 +31,14 @@ class StreamInfoToHeadersFilter : public Http::PassThroughFilter { const std::string dns_end = "envoy.dynamic_forward_proxy.dns_end_ms"; StreamInfo::StreamInfo& stream_info = decoder_callbacks_->streamInfo(); - if (stream_info.filterState()->hasData(dns_start)) { + if (stream_info.downstreamTiming().getValue(dns_start).has_value()) { headers.addCopy( Http::LowerCaseString("dns_start"), - absl::StrCat( - stream_info.filterState()->getDataReadOnly(dns_start).value())); + absl::StrCat(toMs(stream_info.downstreamTiming().getValue(dns_start).value()))); } - if (stream_info.filterState()->hasData(dns_end)) { - headers.addCopy( - Http::LowerCaseString("dns_end"), - absl::StrCat( - stream_info.filterState()->getDataReadOnly(dns_end).value())); + if (stream_info.downstreamTiming().getValue(dns_end).has_value()) { + headers.addCopy(Http::LowerCaseString("dns_end"), + absl::StrCat(toMs(stream_info.downstreamTiming().getValue(dns_end).value()))); } return Http::FilterHeadersStatus::Continue; } diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 2eaad512dcd7e..5f651e4130e5a 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -56,6 +56,7 @@ MockStreamInfo::MockStreamInfo() std::chrono::duration_cast(ts_.systemTime() - start_time_) .count()); })); + ON_CALL(*this, downstreamTiming()).WillByDefault(ReturnRef(downstream_timing_)); ON_CALL(*this, setUpstreamLocalAddress(_)) .WillByDefault( Invoke([this](const Network::Address::InstanceConstSharedPtr& upstream_local_address) { diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index be4c47a77eadf..045a70243f3b6 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -46,6 +46,7 @@ class MockStreamInfo : public StreamInfo { MOCK_METHOD(void, onLastDownstreamTxByteSent, ()); MOCK_METHOD(void, onRequestComplete, ()); MOCK_METHOD(absl::optional, requestComplete, (), (const)); + MOCK_METHOD(DownstreamTiming&, downstreamTiming, ()); MOCK_METHOD(void, addBytesReceived, (uint64_t)); MOCK_METHOD(uint64_t, bytesReceived, (), (const)); MOCK_METHOD(void, addWireBytesReceived, (uint64_t)); @@ -141,6 +142,7 @@ class MockStreamInfo : public StreamInfo { std::string filter_chain_name_; absl::optional upstream_connection_id_; absl::optional attempt_count_; + DownstreamTiming downstream_timing_; }; } // namespace StreamInfo From 1cbb49d062c9aa2ce4d86326ee5a9af0d1082602 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 11 Nov 2021 13:03:18 -0500 Subject: [PATCH 4/6] comments Signed-off-by: Alyssa Wilk --- .../filters/http/dynamic_forward_proxy/proxy_filter.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index b73f78151dc03..bd486627e5409 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -54,8 +54,6 @@ void ProxyFilter::onDestroy() { } Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { - latchTime(decoder_callbacks_, dnsStart()); - Router::RouteConstSharedPtr route = decoder_callbacks_->route(); const Router::RouteEntry* route_entry; if (!route || !(route_entry = route->routeEntry())) { @@ -120,6 +118,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea } } + latchTime(decoder_callbacks_, dnsStart()); // See the comments in dns_cache.h for how loadDnsCacheEntry() handles hosts with embedded ports. // TODO(mattklein123): Because the filter and cluster have independent configuration, it is // not obvious to the user if something is misconfigured. We should see if From 3af98723c7505ec8573be29dbf983c8e95d50342 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 11 Nov 2021 17:00:09 -0500 Subject: [PATCH 5/6] fix all the mocks Signed-off-by: Alyssa Wilk --- .../proxy_filter_test.cc | 57 ++++++++----------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc index 322b0a70283c1..2031c9e8a9473 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc @@ -98,13 +98,13 @@ TEST_F(ProxyFilterTest, HttpDefaultPort) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle* handle = new Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle(); @@ -124,13 +124,14 @@ TEST_F(ProxyFilterTest, HttpsDefaultPort) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(true)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); + Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle* handle = new Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle(); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _)) @@ -149,13 +150,14 @@ TEST_F(ProxyFilterTest, CacheOverflow) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(true)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); + EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _)) .WillOnce(Return( MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::Overflow, nullptr, absl::nullopt})); @@ -175,13 +177,13 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflow) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(true)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle* handle = new Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle(); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _)) @@ -193,8 +195,6 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflow) { // Create a second filter for a 2nd request. auto filter2 = std::make_unique(filter_config_); filter2->setDecoderFilterCallbacks(callbacks_); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()); @@ -217,8 +217,6 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { new Upstream::ResourceAutoIncDec(pending_requests_)); InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -226,6 +224,8 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(true)); Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle* handle = new Extensions::Common::DynamicForwardProxy::MockLoadDnsCacheEntryHandle(); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _)) .WillOnce(Return( MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::Loading, handle, absl::nullopt})); @@ -235,8 +235,6 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { // Create a second filter for a 2nd request. auto filter2 = std::make_unique(filter_config_); filter2->setDecoderFilterCallbacks(callbacks_); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()); @@ -260,8 +258,6 @@ TEST_F(ProxyFilterTest, CircuitBreakerOverflowWithDnsCacheResourceManager) { TEST_F(ProxyFilterTest, NoRoute) { InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()).WillOnce(Return(nullptr)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); } @@ -270,8 +266,6 @@ TEST_F(ProxyFilterTest, NoRoute) { TEST_F(ProxyFilterTest, NoCluster) { InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)).WillOnce(Return(nullptr)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -283,8 +277,6 @@ TEST_F(ProxyFilterTest, NoClusterType) { InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -298,8 +290,6 @@ TEST_F(ProxyFilterTest, NonDynamicForwardProxy) { InSequence s; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -314,8 +304,6 @@ TEST_F(ProxyFilterTest, HostRewrite) { proto_config.set_host_rewrite_literal("bar"); ProxyPerRouteConfig config(proto_config); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -326,6 +314,8 @@ TEST_F(ProxyFilterTest, HostRewrite) { EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig("envoy.filters.http.dynamic_forward_proxy")) .WillOnce(Return(&config)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("bar"), 80, _)) .WillOnce(Return( MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::Loading, handle, absl::nullopt})); @@ -345,8 +335,6 @@ TEST_F(ProxyFilterTest, HostRewriteViaHeader) { proto_config.set_host_rewrite_header("x-set-header"); ProxyPerRouteConfig config(proto_config); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) @@ -357,6 +345,8 @@ TEST_F(ProxyFilterTest, HostRewriteViaHeader) { EXPECT_CALL(*callbacks_.route_, mostSpecificPerFilterConfig("envoy.filters.http.dynamic_forward_proxy")) .WillOnce(Return(&config)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("bar:82"), 80, _)) .WillOnce(Return( MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::Loading, handle, absl::nullopt})); @@ -399,13 +389,13 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata auto host_info = std::make_shared(); host_info->address_ = Network::Utility::parseInternetAddress("1.2.3.4", 80); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 80, _)) .WillOnce(Invoke([&](absl::string_view, uint16_t, ProxyFilter::LoadDnsCacheEntryCallbacks&) { @@ -458,13 +448,13 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad auto host_info = std::make_shared(); host_info->address_ = Network::Utility::parseInternetAddress("1.2.3.4", 80); - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false)); + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 80, _)) .WillOnce(Invoke([&](absl::string_view, uint16_t, ProxyFilter::LoadDnsCacheEntryCallbacks&) { @@ -480,9 +470,9 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad EXPECT_CALL(*host_info, address()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, streamInfo()); EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); // Host was resolved successfully, so continue filter iteration. EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); @@ -519,14 +509,13 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre auto host_info = std::make_shared(); host_info->address_ = nullptr; - EXPECT_CALL(callbacks_, streamInfo()); - EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(callbacks_, route()); EXPECT_CALL(cm_, getThreadLocalCluster(_)); EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_()) .WillOnce(Return(circuit_breakers_)); EXPECT_CALL(*transport_socket_factory_, implementsSecureTransport()).WillOnce(Return(false)); - + EXPECT_CALL(callbacks_, streamInfo()); + EXPECT_CALL(callbacks_, dispatcher()); EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 80, _)) .WillOnce(Invoke([&](absl::string_view, uint16_t, ProxyFilter::LoadDnsCacheEntryCallbacks&) { return MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::InCache, nullptr, host_info}; From efc9af1d63da5cf9756cc21f1a3b442f387c6504 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 16 Nov 2021 13:22:01 -0500 Subject: [PATCH 6/6] comment Signed-off-by: Alyssa Wilk --- .../filters/http/dynamic_forward_proxy/proxy_filter.cc | 8 ++++---- .../filters/http/dynamic_forward_proxy/proxy_filter.h | 8 ++------ .../http/dynamic_forward_proxy/proxy_filter_test.cc | 4 ++-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc index bd486627e5409..15d817c9b05bc 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc @@ -14,7 +14,7 @@ namespace HttpFilters { namespace DynamicForwardProxy { namespace { -void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, const std::string& key) { +void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, absl::string_view key) { StreamInfo::DownstreamTiming& downstream_timing = decoder_callbacks->streamInfo().downstreamTiming(); downstream_timing.setValue(key, decoder_callbacks->dispatcher().timeSource().monotonicTime()); @@ -118,7 +118,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea } } - latchTime(decoder_callbacks_, dnsStart()); + latchTime(decoder_callbacks_, DNS_START); // See the comments in dns_cache.h for how loadDnsCacheEntry() handles hosts with embedded ports. // TODO(mattklein123): Because the filter and cluster have independent configuration, it is // not obvious to the user if something is misconfigured. We should see if @@ -141,7 +141,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea addHostAddressToFilterState(host.value()->address()); } - latchTime(decoder_callbacks_, dnsEnd()); + latchTime(decoder_callbacks_, DNS_END); return Http::FilterHeadersStatus::Continue; } case LoadDnsCacheEntryStatus::Loading: @@ -193,7 +193,7 @@ void ProxyFilter::onLoadDnsCacheComplete( const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { ENVOY_STREAM_LOG(debug, "load DNS cache complete, continuing after adding resolved host: {}", *decoder_callbacks_, host_info->resolvedHost()); - latchTime(decoder_callbacks_, dnsEnd()); + latchTime(decoder_callbacks_, DNS_END); ASSERT(circuit_breaker_ != nullptr); circuit_breaker_.reset(); diff --git a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h index effe11e5aa679..0bc97e646d328 100644 --- a/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h +++ b/source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.h @@ -51,12 +51,8 @@ class ProxyFilter public: ProxyFilter(const ProxyFilterConfigSharedPtr& config) : config_(config) {} - static const std::string& dnsStart() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_start_ms"); - } - static const std::string& dnsEnd() { - CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_end_ms"); - } + static constexpr absl::string_view DNS_START = "envoy.dynamic_forward_proxy.dns_start_ms"; + static constexpr absl::string_view DNS_END = "envoy.dynamic_forward_proxy.dns_end_ms"; // Http::PassThroughDecoderFilter Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc index 2031c9e8a9473..6c6413066ceb8 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc @@ -479,9 +479,9 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad // We expect FilterState and resolution times to be populated EXPECT_TRUE( - callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::dnsStart()).has_value()); + callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::DNS_START).has_value()); EXPECT_TRUE( - callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::dnsEnd()).has_value()); + callbacks_.streamInfo().downstreamTiming().getValue(ProxyFilter::DNS_END).has_value()); const StreamInfo::UpstreamAddress& updated_address_obj = filter_state->getDataReadOnly(