From 8bbdc82d8bb4d379e53d82c31fd0eedd1b132f05 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Mon, 6 Aug 2018 16:17:12 -0400 Subject: [PATCH 01/11] access_log: log requested_server_name in tcp proxy Signed-off-by: Christopher M. Luciano --- include/envoy/request_info/request_info.h | 10 ++++++++++ source/common/access_log/access_log_formatter.cc | 10 ++++++++++ source/common/request_info/request_info_impl.h | 8 ++++++++ source/common/tcp_proxy/tcp_proxy.cc | 3 +++ .../listener/tls_inspector/tls_inspector.cc | 2 ++ .../common/access_log/access_log_formatter_test.cc | 14 ++++++++++++++ test/common/access_log/test_util.h | 7 +++++++ test/common/request_info/request_info_impl_test.cc | 5 +++++ test/mocks/request_info/mocks.cc | 5 +++++ test/mocks/request_info/mocks.h | 3 +++ 10 files changed, 67 insertions(+) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index 8d8a01119674..dc7493e2dba3 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -301,6 +301,16 @@ class RequestInfo { * the same key overriding existing. */ virtual void setDynamicMetadata(const std::string& name, const ProtobufWkt::Struct& value) PURE; + + /** + * @param SNI value requested + */ + virtual void setRequestedServerName(const absl::string_view& requested_server_name) PURE; + + /** + * @return SNI value for downstream host + */ + virtual const absl::string_view& requestedServerName() const PURE; }; } // namespace RequestInfo diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 88ec956d9d55..f64018194ffa 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -301,6 +301,16 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { return RequestInfo::Utility::formatDownstreamAddressNoPort( *request_info.downstreamRemoteAddress()); }; + } else if (field_name == "REQUESTED_SERVER_NAME") { + field_extractor_ = [](const RequestInfo::RequestInfo& request_info) { + absl::string_view requested_server_name; + if (nullptr != request_info.requestedServerName()) { + return request_info.requestedServerName().data(); + } else { + requested_server_name = UnspecifiedValueString; + return requested_server_name.data(); + } + }; } else { throw EnvoyException(fmt::format("Not supported field in RequestInfo: {}", field_name)); } diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index f56f34ebc4e5..867095fd4b91 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -178,6 +178,12 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; + void setRequestedServerName(const absl::string_view& requested_server_name) override { + requested_server_name_ = requested_server_name; + } + + const absl::string_view& requestedServerName() const override { return requested_server_name_; } + const SystemTime start_time_; const MonotonicTime start_time_monotonic_; @@ -197,6 +203,7 @@ struct RequestInfoImpl : public RequestInfo { bool hc_request_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; + absl::string_view requestedServerName_; private: uint64_t bytes_received_{}; @@ -204,6 +211,7 @@ struct RequestInfoImpl : public RequestInfo { Network::Address::InstanceConstSharedPtr upstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_remote_address_; + absl::string_view requested_server_name_; }; } // namespace RequestInfo diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 43f60e791489..cce638934854 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -388,6 +388,9 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) { upstream_connection_->write(data, end_stream); ASSERT(0 == data.length()); resetIdleTimer(); // TODO(ggreenway) PERF: do we need to reset timer on both send and receive? + getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); + ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", + getRequestInfo().requestedServerName()); return Network::FilterStatus::StopIteration; } diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 06a2ef6a5dcc..9f853780492d 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -123,6 +123,8 @@ void Filter::onServername(absl::string_view name) { if (!name.empty()) { config_->stats().sni_found_.inc(); cb_->socket().setRequestedServerName(name); + ENVOY_LOG(debug, "tls:onServerName(), requestedServerName: {}", + cb_->socket().requestedServerName()); } else { config_->stats().sni_not_found_.inc(); } diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 44c0ddf9eda8..a3f63345327a 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -185,6 +185,20 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { RequestInfoFormatter upstream_format("DOWNSTREAM_REMOTE_ADDRESS"); EXPECT_EQ("127.0.0.1:0", upstream_format.format(header, header, header, request_info)); } + + // { + // RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); + // absl::string_view requested_server_name = "stub_server"; + // EXPECT_CALL(request_info, + // requestedServerName()).WillRepeatedly(ReturnRef(requested_server_name)); + // EXPECT_EQ("stub_server", upstream_format.format(header, header, header, request_info)); + // } + + { + RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); + // EXPECT_CALL(request_info, requestedServerName()).WillOnce(Return(nullptr)); + EXPECT_EQ("-", upstream_format.format(header, header, header, request_info)); + } } TEST(AccessLogFormatterTest, requestHeaderFormatter) { diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index dc750cc900d2..dd1a1db09be5 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -154,6 +154,12 @@ class TestRequestInfo : public RequestInfo::RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; + void setRequestedServerName(const absl::string_view& requested_server_name) override { + requested_server_name_ = requested_server_name; + } + + const absl::string_view& requestedServerName() const override { return requested_server_name_; } + SystemTime start_time_; MonotonicTime start_time_monotonic_; @@ -176,6 +182,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo { Network::Address::InstanceConstSharedPtr downstream_remote_address_; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; + absl::string_view requested_server_name_; }; } // namespace Envoy diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index 687a6264a3c8..ef1ba9433653 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -140,6 +140,11 @@ TEST(RequestInfoImplTest, MiscSettersAndGetters) { NiceMock route_entry; request_info.route_entry_ = &route_entry; EXPECT_EQ(&route_entry, request_info.routeEntry()); + + EXPECT_EQ(nullptr, request_info.requestedServerName()); + absl::string_view sni_name = "stubserver.org"; + request_info.setRequestedServerName(sni_name); + EXPECT_EQ(sni_name, request_info.requestedServerName()); } } diff --git a/test/mocks/request_info/mocks.cc b/test/mocks/request_info/mocks.cc index e034c1e82d5c..cb3663cf6275 100644 --- a/test/mocks/request_info/mocks.cc +++ b/test/mocks/request_info/mocks.cc @@ -64,6 +64,11 @@ MockRequestInfo::MockRequestInfo() })); ON_CALL(*this, bytesSent()).WillByDefault(ReturnPointee(&bytes_sent_)); ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); + ON_CALL(*this, setRequestedServerName(_)) + .WillByDefault(Invoke([this](const absl::string_view& requested_server_name) { + requested_server_name_ = requested_server_name; + })); + ON_CALL(*this, requestedServerName()).WillByDefault(ReturnRef(requested_server_name_)); } MockRequestInfo::~MockRequestInfo() {} diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index cafe6815e5bc..aa76c30ae744 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -60,6 +60,8 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD2(setDynamicMetadata, void(const std::string&, const ProtobufWkt::Struct&)); MOCK_METHOD3(setDynamicMetadata, void(const std::string&, const std::string&, const std::string&)); + MOCK_METHOD1(setRequestedServerName, void(const absl::string_view&)); + MOCK_CONST_METHOD0(requestedServerName, const absl::string_view&()); std::shared_ptr> host_{ new testing::NiceMock()}; @@ -81,6 +83,7 @@ class MockRequestInfo : public RequestInfo { Network::Address::InstanceConstSharedPtr upstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_remote_address_; + absl::string_view requested_server_name_; }; } // namespace RequestInfo From 6b04ff50f6b65b9710496a2ab5198fd96e96f985 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Thu, 16 Aug 2018 10:33:53 -0400 Subject: [PATCH 02/11] Address code comments and uncomment working access_log tests Signed-off-by: Christopher M. Luciano --- include/envoy/request_info/request_info.h | 2 +- .../common/access_log/access_log_formatter.cc | 2 +- source/common/request_info/request_info_impl.h | 4 ++-- .../listener/tls_inspector/tls_inspector.cc | 3 +-- .../access_log/access_log_formatter_test.cc | 18 ++++++++++-------- test/mocks/request_info/mocks.cc | 2 +- test/mocks/request_info/mocks.h | 2 +- 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index dc7493e2dba3..f3d4545eb551 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -305,7 +305,7 @@ class RequestInfo { /** * @param SNI value requested */ - virtual void setRequestedServerName(const absl::string_view& requested_server_name) PURE; + virtual void setRequestedServerName(const absl::string_view requested_server_name) PURE; /** * @return SNI value for downstream host diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index f64018194ffa..3faa74d9b3b4 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -304,7 +304,7 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { } else if (field_name == "REQUESTED_SERVER_NAME") { field_extractor_ = [](const RequestInfo::RequestInfo& request_info) { absl::string_view requested_server_name; - if (nullptr != request_info.requestedServerName()) { + if (!request_info.requestedServerName().empty()) { return request_info.requestedServerName().data(); } else { requested_server_name = UnspecifiedValueString; diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index 867095fd4b91..b54aedbcf1c0 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -178,7 +178,7 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - void setRequestedServerName(const absl::string_view& requested_server_name) override { + void setRequestedServerName(const absl::string_view requested_server_name) override { requested_server_name_ = requested_server_name; } @@ -203,7 +203,7 @@ struct RequestInfoImpl : public RequestInfo { bool hc_request_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; - absl::string_view requestedServerName_; + // absl::string_view requestedServerName_; private: uint64_t bytes_received_{}; diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 9f853780492d..393c88486987 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -123,8 +123,7 @@ void Filter::onServername(absl::string_view name) { if (!name.empty()) { config_->stats().sni_found_.inc(); cb_->socket().setRequestedServerName(name); - ENVOY_LOG(debug, "tls:onServerName(), requestedServerName: {}", - cb_->socket().requestedServerName()); + ENVOY_LOG(debug, "tls:onServerName(), requestedServerName: {}", name); } else { config_->stats().sni_not_found_.inc(); } diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index a3f63345327a..696a28e5f892 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -186,17 +186,19 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { EXPECT_EQ("127.0.0.1:0", upstream_format.format(header, header, header, request_info)); } - // { - // RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); - // absl::string_view requested_server_name = "stub_server"; - // EXPECT_CALL(request_info, - // requestedServerName()).WillRepeatedly(ReturnRef(requested_server_name)); - // EXPECT_EQ("stub_server", upstream_format.format(header, header, header, request_info)); - // } + { + RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); + absl::string_view requested_server_name = "stub_server"; + EXPECT_CALL(request_info, requestedServerName()) + .WillRepeatedly(ReturnRef(requested_server_name)); + EXPECT_EQ("stub_server", upstream_format.format(header, header, header, request_info)); + } { RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); - // EXPECT_CALL(request_info, requestedServerName()).WillOnce(Return(nullptr)); + absl::string_view requested_server_name; + EXPECT_CALL(request_info, requestedServerName()) + .WillRepeatedly(ReturnRef(requested_server_name)); EXPECT_EQ("-", upstream_format.format(header, header, header, request_info)); } } diff --git a/test/mocks/request_info/mocks.cc b/test/mocks/request_info/mocks.cc index cb3663cf6275..fd7875c12eea 100644 --- a/test/mocks/request_info/mocks.cc +++ b/test/mocks/request_info/mocks.cc @@ -65,7 +65,7 @@ MockRequestInfo::MockRequestInfo() ON_CALL(*this, bytesSent()).WillByDefault(ReturnPointee(&bytes_sent_)); ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, setRequestedServerName(_)) - .WillByDefault(Invoke([this](const absl::string_view& requested_server_name) { + .WillByDefault(Invoke([this](const absl::string_view requested_server_name) { requested_server_name_ = requested_server_name; })); ON_CALL(*this, requestedServerName()).WillByDefault(ReturnRef(requested_server_name_)); diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index aa76c30ae744..e07a0fe5b7d3 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -60,7 +60,7 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD2(setDynamicMetadata, void(const std::string&, const ProtobufWkt::Struct&)); MOCK_METHOD3(setDynamicMetadata, void(const std::string&, const std::string&, const std::string&)); - MOCK_METHOD1(setRequestedServerName, void(const absl::string_view&)); + MOCK_METHOD1(setRequestedServerName, void(const absl::string_view)); MOCK_CONST_METHOD0(requestedServerName, const absl::string_view&()); std::shared_ptr> host_{ From c416a0141ad17ee49e7d5c6408289a82a5481698 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Thu, 16 Aug 2018 13:12:51 -0400 Subject: [PATCH 03/11] log string_view data in tcp_proxy Signed-off-by: Christopher M. Luciano --- source/common/tcp_proxy/tcp_proxy.cc | 2 +- test/common/access_log/test_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index cce638934854..0630c79ea753 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -390,7 +390,7 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) { resetIdleTimer(); // TODO(ggreenway) PERF: do we need to reset timer on both send and receive? getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", - getRequestInfo().requestedServerName()); + getRequestInfo().requestedServerName().data()); return Network::FilterStatus::StopIteration; } diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index dd1a1db09be5..c690c9f62b6e 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -154,7 +154,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - void setRequestedServerName(const absl::string_view& requested_server_name) override { + void setRequestedServerName(const absl::string_view requested_server_name) override { requested_server_name_ = requested_server_name; } From 17cf3202f7b493308e207ff831c22d5072bca5b0 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Thu, 16 Aug 2018 15:39:16 -0400 Subject: [PATCH 04/11] Add requested_server_name to docs and release notes Signed-off-by: Christopher M. Luciano --- docs/root/configuration/access_log.rst | 4 ++++ docs/root/intro/version_history.rst | 1 + 2 files changed, 5 insertions(+) diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index 94392f404c69..eca982d73b50 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -227,6 +227,10 @@ The following command operators are supported: TCP Not implemented ("-"). +%REQUESTED_SERVER_NAME% + TCP + String value set on ssl connection socket for Server Name Indication (SNI) + .. _config_access_log_default_format: Default format diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 6474940705ce..95a487cc3743 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -6,6 +6,7 @@ Version history * access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. * access log: added RESPONSE_DURATION and RESPONSE_TX_DURATION. +* access log: added REQUESTED_SERVER_NAME for SNI * admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics through `Hystrix dashboard `_. * grpc-json: added support for building HTTP response from From d8369abeb4e996a7b30d75ec76116d54dcba6dc8 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Thu, 23 Aug 2018 14:33:48 -0400 Subject: [PATCH 05/11] remove unused requestedServerName Signed-off-by: Christopher M. Luciano --- source/common/request_info/request_info_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index b54aedbcf1c0..dc1af770a2d0 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -203,7 +203,6 @@ struct RequestInfoImpl : public RequestInfo { bool hc_request_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; - // absl::string_view requestedServerName_; private: uint64_t bytes_received_{}; From 5113cb239a14a2c30b45bdcc5973ccf794918eb1 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Fri, 24 Aug 2018 15:13:01 -0400 Subject: [PATCH 06/11] Store requestedServerName as string Signed-off-by: Christopher M. Luciano --- include/envoy/request_info/request_info.h | 2 +- source/common/access_log/access_log_formatter.cc | 6 ++---- source/common/request_info/request_info_impl.h | 8 ++++---- source/common/tcp_proxy/tcp_proxy.cc | 2 +- test/common/access_log/access_log_formatter_test.cc | 4 ++-- test/common/request_info/request_info_impl_test.cc | 4 ++-- test/mocks/request_info/mocks.cc | 2 +- test/mocks/request_info/mocks.h | 4 ++-- 8 files changed, 15 insertions(+), 17 deletions(-) diff --git a/include/envoy/request_info/request_info.h b/include/envoy/request_info/request_info.h index f3d4545eb551..dc4abffccb81 100644 --- a/include/envoy/request_info/request_info.h +++ b/include/envoy/request_info/request_info.h @@ -310,7 +310,7 @@ class RequestInfo { /** * @return SNI value for downstream host */ - virtual const absl::string_view& requestedServerName() const PURE; + virtual const std::string& requestedServerName() const PURE; }; } // namespace RequestInfo diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc index 3faa74d9b3b4..40247819f2cb 100644 --- a/source/common/access_log/access_log_formatter.cc +++ b/source/common/access_log/access_log_formatter.cc @@ -303,12 +303,10 @@ RequestInfoFormatter::RequestInfoFormatter(const std::string& field_name) { }; } else if (field_name == "REQUESTED_SERVER_NAME") { field_extractor_ = [](const RequestInfo::RequestInfo& request_info) { - absl::string_view requested_server_name; if (!request_info.requestedServerName().empty()) { - return request_info.requestedServerName().data(); + return request_info.requestedServerName(); } else { - requested_server_name = UnspecifiedValueString; - return requested_server_name.data(); + return UnspecifiedValueString; } }; } else { diff --git a/source/common/request_info/request_info_impl.h b/source/common/request_info/request_info_impl.h index dc1af770a2d0..4407cfa24fcb 100644 --- a/source/common/request_info/request_info_impl.h +++ b/source/common/request_info/request_info_impl.h @@ -178,11 +178,11 @@ struct RequestInfoImpl : public RequestInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - void setRequestedServerName(const absl::string_view requested_server_name) override { - requested_server_name_ = requested_server_name; + void setRequestedServerName(absl::string_view requested_server_name) override { + requested_server_name_ = std::string(requested_server_name); } - const absl::string_view& requestedServerName() const override { return requested_server_name_; } + const std::string& requestedServerName() const override { return requested_server_name_; } const SystemTime start_time_; const MonotonicTime start_time_monotonic_; @@ -210,7 +210,7 @@ struct RequestInfoImpl : public RequestInfo { Network::Address::InstanceConstSharedPtr upstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_remote_address_; - absl::string_view requested_server_name_; + std::string requested_server_name_; }; } // namespace RequestInfo diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 0630c79ea753..cce638934854 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -390,7 +390,7 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) { resetIdleTimer(); // TODO(ggreenway) PERF: do we need to reset timer on both send and receive? getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", - getRequestInfo().requestedServerName().data()); + getRequestInfo().requestedServerName()); return Network::FilterStatus::StopIteration; } diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 696a28e5f892..25915b721012 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -188,7 +188,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { { RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); - absl::string_view requested_server_name = "stub_server"; + std::string requested_server_name = "stub_server"; EXPECT_CALL(request_info, requestedServerName()) .WillRepeatedly(ReturnRef(requested_server_name)); EXPECT_EQ("stub_server", upstream_format.format(header, header, header, request_info)); @@ -196,7 +196,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { { RequestInfoFormatter upstream_format("REQUESTED_SERVER_NAME"); - absl::string_view requested_server_name; + std::string requested_server_name; EXPECT_CALL(request_info, requestedServerName()) .WillRepeatedly(ReturnRef(requested_server_name)); EXPECT_EQ("-", upstream_format.format(header, header, header, request_info)); diff --git a/test/common/request_info/request_info_impl_test.cc b/test/common/request_info/request_info_impl_test.cc index ef1ba9433653..b8896ba1fac7 100644 --- a/test/common/request_info/request_info_impl_test.cc +++ b/test/common/request_info/request_info_impl_test.cc @@ -141,10 +141,10 @@ TEST(RequestInfoImplTest, MiscSettersAndGetters) { request_info.route_entry_ = &route_entry; EXPECT_EQ(&route_entry, request_info.routeEntry()); - EXPECT_EQ(nullptr, request_info.requestedServerName()); + EXPECT_EQ("", request_info.requestedServerName()); absl::string_view sni_name = "stubserver.org"; request_info.setRequestedServerName(sni_name); - EXPECT_EQ(sni_name, request_info.requestedServerName()); + EXPECT_EQ(std::string(sni_name), request_info.requestedServerName()); } } diff --git a/test/mocks/request_info/mocks.cc b/test/mocks/request_info/mocks.cc index fd7875c12eea..96fa726d2ab6 100644 --- a/test/mocks/request_info/mocks.cc +++ b/test/mocks/request_info/mocks.cc @@ -66,7 +66,7 @@ MockRequestInfo::MockRequestInfo() ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, setRequestedServerName(_)) .WillByDefault(Invoke([this](const absl::string_view requested_server_name) { - requested_server_name_ = requested_server_name; + requested_server_name_ = std::string(requested_server_name); })); ON_CALL(*this, requestedServerName()).WillByDefault(ReturnRef(requested_server_name_)); } diff --git a/test/mocks/request_info/mocks.h b/test/mocks/request_info/mocks.h index e07a0fe5b7d3..8e0412e1a93b 100644 --- a/test/mocks/request_info/mocks.h +++ b/test/mocks/request_info/mocks.h @@ -61,7 +61,7 @@ class MockRequestInfo : public RequestInfo { MOCK_METHOD3(setDynamicMetadata, void(const std::string&, const std::string&, const std::string&)); MOCK_METHOD1(setRequestedServerName, void(const absl::string_view)); - MOCK_CONST_METHOD0(requestedServerName, const absl::string_view&()); + MOCK_CONST_METHOD0(requestedServerName, const std::string&()); std::shared_ptr> host_{ new testing::NiceMock()}; @@ -83,7 +83,7 @@ class MockRequestInfo : public RequestInfo { Network::Address::InstanceConstSharedPtr upstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_remote_address_; - absl::string_view requested_server_name_; + std::string requested_server_name_; }; } // namespace RequestInfo From a39f84bef8fa1b7cdb6ed9e7973620fabbe7cbba Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Fri, 24 Aug 2018 16:50:08 -0400 Subject: [PATCH 07/11] missing string conversion in accesslog testutil Signed-off-by: Christopher M. Luciano --- test/common/access_log/test_util.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/common/access_log/test_util.h b/test/common/access_log/test_util.h index c690c9f62b6e..c4d1ef49c5b6 100644 --- a/test/common/access_log/test_util.h +++ b/test/common/access_log/test_util.h @@ -155,10 +155,10 @@ class TestRequestInfo : public RequestInfo::RequestInfo { }; void setRequestedServerName(const absl::string_view requested_server_name) override { - requested_server_name_ = requested_server_name; + requested_server_name_ = std::string(requested_server_name); } - const absl::string_view& requestedServerName() const override { return requested_server_name_; } + const std::string& requestedServerName() const override { return requested_server_name_; } SystemTime start_time_; MonotonicTime start_time_monotonic_; @@ -182,7 +182,7 @@ class TestRequestInfo : public RequestInfo::RequestInfo { Network::Address::InstanceConstSharedPtr downstream_remote_address_; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; - absl::string_view requested_server_name_; + std::string requested_server_name_; }; } // namespace Envoy From b3274e5a2c5c694858354ea3aea294e0cdc7e6b3 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Mon, 27 Aug 2018 13:19:45 -0400 Subject: [PATCH 08/11] Log requested_server_name at connected in tcp_proxy Signed-off-by: Christopher M. Luciano --- source/common/tcp_proxy/tcp_proxy.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index cce638934854..cf0d3c2a816a 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -388,9 +388,6 @@ Network::FilterStatus Filter::onData(Buffer::Instance& data, bool end_stream) { upstream_connection_->write(data, end_stream); ASSERT(0 == data.length()); resetIdleTimer(); // TODO(ggreenway) PERF: do we need to reset timer on both send and receive? - getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); - ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", - getRequestInfo().requestedServerName()); return Network::FilterStatus::StopIteration; } @@ -472,6 +469,10 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { Upstream::Outlier::Result::SUCCESS); onConnectionSuccess(); + getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); + ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", + getRequestInfo().requestedServerName()); + if (config_->idleTimeout()) { // The idle_timer_ can be moved to a Drainer, so related callbacks call into // the UpstreamCallbacks, which has the same lifetime as the timer, and can dispatch From d2c6a3405fe67db6105a6b9752379846ebfc7190 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Tue, 28 Aug 2018 10:40:18 -0400 Subject: [PATCH 09/11] log correct message for requestedServerName Signed-off-by: Christopher M. Luciano --- source/common/tcp_proxy/tcp_proxy.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index cf0d3c2a816a..fa682b9a33a9 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -470,7 +470,7 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { onConnectionSuccess(); getRequestInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName()); - ENVOY_LOG(debug, "TCP:onData(), requestedServerName: {} ", + ENVOY_LOG(debug, "TCP:onUpstreamEvent(), requestedServerName: {}", getRequestInfo().requestedServerName()); if (config_->idleTimeout()) { From 7d02b33af5856ac70f06b0c0e2d58ed2c705394e Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Tue, 28 Aug 2018 12:19:05 -0400 Subject: [PATCH 10/11] Add SNI access_logging to http Signed-off-by: Christopher M. Luciano --- docs/root/configuration/access_log.rst | 2 ++ docs/root/intro/version_history.rst | 2 +- source/common/http/conn_manager_impl.cc | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index eca982d73b50..323b5f1b712d 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -228,6 +228,8 @@ The following command operators are supported: Not implemented ("-"). %REQUESTED_SERVER_NAME% + HTTP + String value set on ssl connection socket for Server Name Indication (SNI) TCP String value set on ssl connection socket for Server Name Indication (SNI) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 95a487cc3743..e88b5313d057 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -6,7 +6,7 @@ Version history * access log: added :ref:`response flag filter ` to filter based on the presence of Envoy response flags. * access log: added RESPONSE_DURATION and RESPONSE_TX_DURATION. -* access log: added REQUESTED_SERVER_NAME for SNI +* access log: added REQUESTED_SERVER_NAME for SNI to tcp_proxy and http * admin: added :http:get:`/hystrix_event_stream` as an endpoint for monitoring envoy's statistics through `Hystrix dashboard `_. * grpc-json: added support for building HTTP response from diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 6e27c7f6baaf..009b289aea71 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -380,6 +380,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect [this]() -> void { onIdleTimeout(); }); resetIdleTimer(); } + request_info_.setRequestedServerName( + connection_manager_.read_callbacks_->connection().requestedServerName()); } ConnectionManagerImpl::ActiveStream::~ActiveStream() { From 49f0ffe841668022e18147a70d45ba673f5bd648 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Tue, 28 Aug 2018 12:28:27 -0400 Subject: [PATCH 11/11] Kick CI Signed-off-by: Christopher M. Luciano