envoyproxy · mattklein123 · Jun 30, 2021 · May 19, 2021 · May 20, 2021 · May 21, 2021
diff --git a/envoy/network/socket.h b/envoy/network/socket.h
@@ -46,6 +46,8 @@ struct SocketOptionName {
  * Interfaces for providing a socket's various addresses. This is split into a getters interface
  * and a getters + setters interface. This is so that only the getters portion can be overridden
  * in certain cases.
+ * TODO(soulxu): Since there are more than address information inside the provider, this will be
+ * renamed as ConnectionInfoProvider. Ref https://github.com/envoyproxy/envoy/issues/17168
  */
 class SocketAddressProvider {
 public:
@@ -73,6 +75,11 @@ class SocketAddressProvider {
    */
   virtual const Address::InstanceConstSharedPtr& directRemoteAddress() const PURE;
 
+  /**
+   * @return SNI value for downstream host.
+   */
+  virtual absl::string_view requestedServerName() const PURE;
+
   /**
    * Dumps the state of the SocketAddressProvider to the given ostream.
    *
@@ -109,6 +116,11 @@ class SocketAddressSetter : public SocketAddressProvider {
    * Set the remote address of the socket.
    */
   virtual void setRemoteAddress(const Address::InstanceConstSharedPtr& remote_address) PURE;
+
+  /**
+   * @param SNI value requested.
+   */
+  virtual void setRequestedServerName(const absl::string_view requested_server_name) PURE;
 };
 
 using SocketAddressSetterSharedPtr = std::shared_ptr<SocketAddressSetter>;

diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h
@@ -529,16 +529,6 @@ class StreamInfo {
   virtual const FilterStateSharedPtr& upstreamFilterState() const PURE;
   virtual void setUpstreamFilterState(const FilterStateSharedPtr& filter_state) PURE;
 
-  /**
-   * @param SNI value requested.
-   */
-  virtual void setRequestedServerName(const absl::string_view requested_server_name) PURE;
-
-  /**
-   * @return SNI value for downstream host.
-   */
-  virtual const std::string& requestedServerName() const PURE;
-
   /**
    * @param failure_reason the upstream transport failure reason.
    */

diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc
@@ -820,8 +820,8 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
     field_extractor_ = std::make_unique<StreamInfoStringFieldExtractor>(
         [](const StreamInfo::StreamInfo& stream_info) {
           absl::optional<std::string> result;
-          if (!stream_info.requestedServerName().empty()) {
-            result = stream_info.requestedServerName();
+          if (!stream_info.downstreamAddressProvider().requestedServerName().empty()) {
+            result = std::string(stream_info.downstreamAddressProvider().requestedServerName());
           }
           return result;
         });

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -687,9 +687,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
     max_stream_duration_timer_->enableTimer(connection_manager_.config_.maxStreamDuration().value(),
                                             this);
   }
-
-  filter_manager_.streamInfo().setRequestedServerName(
-      connection_manager_.read_callbacks_->connection().requestedServerName());
 }
 
 void ConnectionManagerImpl::ActiveStream::completeRequest() {

diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h
@@ -618,7 +618,9 @@ class OverridableRemoteSocketAddressSetterStreamInfo : public StreamInfo::Stream
   const Network::Address::InstanceConstSharedPtr& directRemoteAddress() const override {
     return StreamInfoImpl::downstreamAddressProvider().directRemoteAddress();
   }
-
+  absl::string_view requestedServerName() const override {
+    return StreamInfoImpl::downstreamAddressProvider().requestedServerName();
+  }
   void dumpState(std::ostream& os, int indent_level) const override {
     StreamInfoImpl::dumpState(os, indent_level);
 

diff --git a/source/common/network/listen_socket_impl.h b/source/common/network/listen_socket_impl.h
@@ -152,25 +152,25 @@ class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket {
 
   void setRequestedServerName(absl::string_view server_name) override {
     // Always keep the server_name_ as lower case.
-    server_name_ = absl::AsciiStrToLower(server_name);
+    addressProvider().setRequestedServerName(absl::AsciiStrToLower(server_name));
+  }
+  absl::string_view requestedServerName() const override {
+    return addressProvider().requestedServerName();
   }
-  absl::string_view requestedServerName() const override { return server_name_; }
 
   absl::optional<std::chrono::milliseconds> lastRoundTripTime() override {
     return ioHandle().lastRoundTripTime();
   }
 
   void dumpState(std::ostream& os, int indent_level) const override {
     const char* spaces = spacesForLevel(indent_level);
-    os << spaces << "ListenSocketImpl " << this << DUMP_MEMBER(transport_protocol_)
-       << DUMP_MEMBER(server_name_) << "\n";
+    os << spaces << "ListenSocketImpl " << this << DUMP_MEMBER(transport_protocol_) << "\n";
     DUMP_DETAILS(address_provider_);
   }
 
 protected:
   std::string transport_protocol_;
   std::vector<std::string> application_protocols_;
-  std::string server_name_;
 };
 
 // ConnectionSocket used with server connections.

diff --git a/source/common/network/socket_impl.h b/source/common/network/socket_impl.h
@@ -24,7 +24,8 @@ class SocketAddressSetterImpl : public SocketAddressSetter {
     os << spaces << "SocketAddressSetterImpl " << this
        << DUMP_NULLABLE_MEMBER(remote_address_, remote_address_->asStringView())
        << DUMP_NULLABLE_MEMBER(direct_remote_address_, direct_remote_address_->asStringView())
-       << DUMP_NULLABLE_MEMBER(local_address_, local_address_->asStringView()) << "\n";
+       << DUMP_NULLABLE_MEMBER(local_address_, local_address_->asStringView())
+       << DUMP_MEMBER(server_name_) << "\n";
   }
 
   // SocketAddressSetter
@@ -44,12 +45,17 @@ class SocketAddressSetterImpl : public SocketAddressSetter {
   const Address::InstanceConstSharedPtr& directRemoteAddress() const override {
     return direct_remote_address_;
   }
+  absl::string_view requestedServerName() const override { return server_name_; }
+  void setRequestedServerName(const absl::string_view requested_server_name) override {
+    server_name_ = std::string(requested_server_name);
+  }
 
 private:
   Address::InstanceConstSharedPtr local_address_;
   bool local_address_restored_{false};
   Address::InstanceConstSharedPtr remote_address_;
   Address::InstanceConstSharedPtr direct_remote_address_;
+  std::string server_name_;
 };
 
 class SocketImpl : public virtual Socket {

diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h
@@ -233,12 +233,6 @@ struct StreamInfoImpl : public StreamInfo {
     upstream_filter_state_ = filter_state;
   }
 
-  void setRequestedServerName(absl::string_view requested_server_name) override {
-    requested_server_name_ = std::string(requested_server_name);
-  }
-
-  const std::string& requestedServerName() const override { return requested_server_name_; }
-
   void setUpstreamTransportFailureReason(absl::string_view failure_reason) override {
     upstream_transport_failure_reason_ = std::string(failure_reason);
   }

diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -655,9 +655,9 @@ void Filter::onUpstreamConnection() {
   read_callbacks_->upstreamHost()->outlierDetector().putResult(
       Upstream::Outlier::Result::LocalOriginConnectSuccessFinal);
 
-  getStreamInfo().setRequestedServerName(read_callbacks_->connection().requestedServerName());
   ENVOY_CONN_LOG(debug, "TCP:onUpstreamEvent(), requestedServerName: {}",
-                 read_callbacks_->connection(), getStreamInfo().requestedServerName());
+                 read_callbacks_->connection(),
+                 getStreamInfo().downstreamAddressProvider().requestedServerName());
 
   if (config_->idleTimeout()) {
     // The idle_timer_ can be moved to a Drainer, so related callbacks call into

@@ -171,7 +171,8 @@ void Utility::extractCommonAccessLogProperties(
     const Ssl::ConnectionInfoConstSharedPtr downstream_ssl_connection =
         stream_info.downstreamSslConnection();
 
-    tls_properties->set_tls_sni_hostname(stream_info.requestedServerName());
+    tls_properties->set_tls_sni_hostname(
+        std::string(stream_info.downstreamAddressProvider().requestedServerName()));
 
     auto* local_properties = tls_properties->mutable_local_certificate_properties();
     for (const auto& uri_san : downstream_ssl_connection->uriSanLocalCertificate()) {

diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc
@@ -184,7 +184,7 @@ absl::optional<CelValue> ConnectionWrapper::operator[](CelValue key) const {
     return CelValue::CreateBool(info_.downstreamSslConnection() != nullptr &&
                                 info_.downstreamSslConnection()->peerCertificatePresented());
   } else if (value == RequestedServerName) {
-    return CelValue::CreateString(&info_.requestedServerName());
+    return CelValue::CreateStringView(info_.downstreamAddressProvider().requestedServerName());
   } else if (value == ID) {
     auto id = info_.connectionID();
     if (id.has_value()) {

@@ -141,7 +141,7 @@ int StreamInfoWrapper::luaDownstreamDirectRemoteAddress(lua_State* state) {
 }
 
 int StreamInfoWrapper::luaRequestedServerName(lua_State* state) {
-  lua_pushstring(state, stream_info_.requestedServerName().c_str());
+  lua_pushstring(state, stream_info_.downstreamAddressProvider().requestedServerName().data());
   return 1;
 }
 

diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc
@@ -674,8 +674,7 @@ TEST(SubstitutionFormatterTest, streamInfoFormatter) {
   {
     StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
     std::string requested_server_name = "stub_server";
-    EXPECT_CALL(stream_info, requestedServerName())
-        .WillRepeatedly(ReturnRef(requested_server_name));
+    stream_info.downstream_address_provider_->setRequestedServerName(requested_server_name);
     EXPECT_EQ("stub_server", upstream_format.format(request_headers, response_headers,
                                                     response_trailers, stream_info, body));
     EXPECT_THAT(upstream_format.formatValue(request_headers, response_headers, response_trailers,
@@ -686,8 +685,7 @@ TEST(SubstitutionFormatterTest, streamInfoFormatter) {
   {
     StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
     std::string requested_server_name;
-    EXPECT_CALL(stream_info, requestedServerName())
-        .WillRepeatedly(ReturnRef(requested_server_name));
+    stream_info.downstream_address_provider_->setRequestedServerName(requested_server_name);
     EXPECT_EQ(absl::nullopt, upstream_format.format(request_headers, response_headers,
                                                     response_trailers, stream_info, body));
     EXPECT_THAT(upstream_format.formatValue(request_headers, response_headers, response_trailers,

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -293,11 +293,7 @@ TEST_F(HttpConnectionManagerImplTest, 100ContinueResponseWithDecoderPause) {
 // When create new stream, the stream info will be populated from the connection.
 TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {
   setup(true, "", false);
-
-  absl::string_view server_name = "fake-server";
   EXPECT_CALL(filter_callbacks_.connection_, id()).WillRepeatedly(Return(1234));
-  EXPECT_CALL(filter_callbacks_.connection_, requestedServerName())
-      .WillRepeatedly(Return(server_name));
 
   // Set up the codec.
   Buffer::OwnedImpl fake_input("input");
@@ -308,7 +304,7 @@ TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {
   EXPECT_EQ(requestIDExtension().get(), decoder_->streamInfo().getRequestIDProvider());
   EXPECT_EQ(ssl_connection_, decoder_->streamInfo().downstreamSslConnection());
   EXPECT_EQ(1234U, decoder_->streamInfo().connectionID());
-  EXPECT_EQ(server_name, decoder_->streamInfo().requestedServerName());
+  EXPECT_EQ(server_name_, decoder_->streamInfo().downstreamAddressProvider().requestedServerName());
 
   // Clean up.
   filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);

diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc
@@ -70,6 +70,8 @@ void HttpConnectionManagerImplTest::setup(bool ssl, const std::string& server_na
       std::make_shared<Network::Address::Ipv4Instance>("0.0.0.0"));
   filter_callbacks_.connection_.stream_info_.downstream_address_provider_
       ->setDirectRemoteAddressForTest(std::make_shared<Network::Address::Ipv4Instance>("0.0.0.0"));
+  filter_callbacks_.connection_.stream_info_.downstream_address_provider_->setRequestedServerName(
+      server_name_);
   conn_manager_ = std::make_unique<ConnectionManagerImpl>(
       *this, drain_close_, random_, http_context_, runtime_, local_info_, cluster_manager_,
       overload_manager_, test_time_.timeSystem());

diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc
@@ -1913,19 +1913,19 @@ TEST_P(ConnectionImplTest, NetworkSocketDumpsWithoutAllocatingMemory) {
   // Check socket dump
   const auto contents = ostream.contents();
   EXPECT_THAT(contents, HasSubstr("ListenSocketImpl"));
-  EXPECT_THAT(contents, HasSubstr("transport_protocol_: , server_name_: envoyproxy.io"));
+  EXPECT_THAT(contents, HasSubstr("transport_protocol_: "));
   EXPECT_THAT(contents, HasSubstr("SocketAddressSetterImpl"));
   if (GetParam() == Network::Address::IpVersion::v4) {
     EXPECT_THAT(
         contents,
         HasSubstr(
             "remote_address_: 1.1.1.1:80, direct_remote_address_: 1.1.1.1:80, local_address_: "
-            "1.2.3.4:56789"));
+            "1.2.3.4:56789, server_name_: envoyproxy.io"));
   } else {
     EXPECT_THAT(
         contents,
         HasSubstr("remote_address_: [::1]:80, direct_remote_address_: [::1]:80, local_address_: "
-                  "[::1:2:3:4]:56789"));
+                  "[::1:2:3:4]:56789, server_name_: envoyproxy.io"));
   }
 }
 

diff --git a/test/common/stream_info/stream_info_impl_test.cc b/test/common/stream_info/stream_info_impl_test.cc
@@ -180,11 +180,6 @@ TEST_F(StreamInfoImplTest, MiscSettersAndGetters) {
     EXPECT_EQ(1,
               stream_info.upstreamFilterState()->getDataReadOnly<TestIntAccessor>("test").access());
 
-    EXPECT_EQ("", stream_info.requestedServerName());
-    absl::string_view sni_name = "stubserver.org";
-    stream_info.setRequestedServerName(sni_name);
-    EXPECT_EQ(std::string(sni_name), stream_info.requestedServerName());
-
     EXPECT_EQ(absl::nullopt, stream_info.upstreamClusterInfo());
     Upstream::ClusterInfoConstSharedPtr cluster_info(new NiceMock<Upstream::MockClusterInfo>());
     stream_info.setUpstreamClusterInfo(cluster_info);

diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h
@@ -178,12 +178,6 @@ class TestStreamInfo : public StreamInfo::StreamInfo {
     upstream_filter_state_ = filter_state;
   }
 
-  void setRequestedServerName(const absl::string_view requested_server_name) override {
-    requested_server_name_ = std::string(requested_server_name);
-  }
-
-  const std::string& requestedServerName() const override { return requested_server_name_; }
-
   void setUpstreamTransportFailureReason(absl::string_view failure_reason) override {
     upstream_transport_failure_reason_ = std::string(failure_reason);
   }

@@ -389,7 +389,7 @@ response: {}
     ON_CALL(*connection_info, tlsVersion()).WillByDefault(ReturnRef(tlsVersion));
     ON_CALL(*connection_info, ciphersuiteId()).WillByDefault(Return(0x2CC0));
     stream_info.setDownstreamSslConnection(connection_info);
-    stream_info.requested_server_name_ = "sni";
+    stream_info.downstream_address_provider_->setRequestedServerName("sni");
 
     Http::TestRequestHeaderMapImpl request_headers{
         {":method", "WHACKADOO"},
@@ -449,7 +449,7 @@ response: {}
     ON_CALL(*connection_info, tlsVersion()).WillByDefault(ReturnRef(tlsVersion));
     ON_CALL(*connection_info, ciphersuiteId()).WillByDefault(Return(0x2F));
     stream_info.setDownstreamSslConnection(connection_info);
-    stream_info.requested_server_name_ = "sni";
+    stream_info.downstream_address_provider_->setRequestedServerName("sni");
 
     Http::TestRequestHeaderMapImpl request_headers{
         {":method", "WHACKADOO"},
@@ -499,7 +499,7 @@ response: {}
     ON_CALL(*connection_info, tlsVersion()).WillByDefault(ReturnRef(tlsVersion));
     ON_CALL(*connection_info, ciphersuiteId()).WillByDefault(Return(0x2F));
     stream_info.setDownstreamSslConnection(connection_info);
-    stream_info.requested_server_name_ = "sni";
+    stream_info.downstream_address_provider_->setRequestedServerName("sni");
 
     Http::TestRequestHeaderMapImpl request_headers{
         {":method", "WHACKADOO"},
@@ -549,7 +549,7 @@ response: {}
     ON_CALL(*connection_info, tlsVersion()).WillByDefault(ReturnRef(tlsVersion));
     ON_CALL(*connection_info, ciphersuiteId()).WillByDefault(Return(0x2F));
     stream_info.setDownstreamSslConnection(connection_info);
-    stream_info.requested_server_name_ = "sni";
+    stream_info.downstream_address_provider_->setRequestedServerName("sni");
 
     Http::TestRequestHeaderMapImpl request_headers{
         {":method", "WHACKADOO"},
@@ -599,7 +599,7 @@ response: {}
     ON_CALL(*connection_info, tlsVersion()).WillByDefault(ReturnRef(tlsVersion));
     ON_CALL(*connection_info, ciphersuiteId()).WillByDefault(Return(0x2F));
     stream_info.setDownstreamSslConnection(connection_info);
-    stream_info.requested_server_name_ = "sni";
+    stream_info.downstream_address_provider_->setRequestedServerName("sni");
 
     Http::TestRequestHeaderMapImpl request_headers{
         {":method", "WHACKADOO"},

diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc
@@ -440,10 +440,10 @@ TEST(Context, ConnectionAttributes) {
   const std::string sni_name = "kittens.com";
   info.downstream_address_provider_->setLocalAddress(local);
   info.downstream_address_provider_->setRemoteAddress(remote);
+  info.downstream_address_provider_->setRequestedServerName(sni_name);
   EXPECT_CALL(info, downstreamSslConnection()).WillRepeatedly(Return(downstream_ssl_info));
   EXPECT_CALL(info, upstreamSslConnection()).WillRepeatedly(Return(upstream_ssl_info));
   EXPECT_CALL(info, upstreamHost()).WillRepeatedly(Return(upstream_host));
-  EXPECT_CALL(info, requestedServerName()).WillRepeatedly(ReturnRef(sni_name));
   EXPECT_CALL(info, upstreamLocalAddress()).WillRepeatedly(ReturnRef(upstream_local_address));
   const std::string upstream_transport_failure_reason = "ConnectionTermination";
   EXPECT_CALL(info, upstreamTransportFailureReason())

@@ -1794,8 +1794,8 @@ TEST_F(LuaHttpFilterTest, GetRequestedServerName) {
   setup(SCRIPT);
 
   EXPECT_CALL(decoder_callbacks_, streamInfo()).WillOnce(ReturnRef(stream_info_));
-  std::string server_name = "foo.example.com";
-  EXPECT_CALL(stream_info_, requestedServerName()).WillOnce(ReturnRef(server_name));
+  absl::string_view server_name = "foo.example.com";
+  stream_info_.downstream_address_provider_->setRequestedServerName(server_name);
 
   Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
   EXPECT_CALL(*filter_, scriptLog(spdlog::level::trace, StrEq("foo.example.com")));

@@ -310,7 +310,7 @@ TEST_F(LuaStreamInfoWrapperTest, ReturnRequestedServerName) {
   setup(SCRIPT);
 
   NiceMock<Envoy::StreamInfo::MockStreamInfo> stream_info;
-  stream_info.requested_server_name_ = "some.sni.io";
+  stream_info.downstream_address_provider_->setRequestedServerName("some.sni.io");
   Filters::Common::Lua::LuaDeathRef<StreamInfoWrapper> wrapper(
       StreamInfoWrapper::create(coroutine_->luaState(), stream_info), true);
   EXPECT_CALL(printer_, testPrint("some.sni.io"));

@@ -1682,7 +1682,7 @@ TEST_P(WasmHttpFilterTest, Property) {
   EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, true));
   StreamInfo::MockStreamInfo log_stream_info;
   request_stream_info_.route_name_ = "route12";
-  request_stream_info_.requested_server_name_ = "w3.org";
+  request_stream_info_.downstream_address_provider_->setRequestedServerName("w3.org");
   NiceMock<Network::MockConnection> connection;
   EXPECT_CALL(connection, id()).WillRepeatedly(Return(4));
   EXPECT_CALL(encoder_callbacks_, connection()).WillRepeatedly(Return(&connection));

diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h
@@ -152,7 +152,6 @@ inline std::unique_ptr<TestStreamInfo> fromStreamInfo(const test::fuzz::StreamIn
   if (stream_info.has_response_code()) {
     test_stream_info->response_code_ = stream_info.response_code().value();
   }
-  test_stream_info->setRequestedServerName(stream_info.requested_server_name());
   auto upstream_host = std::make_shared<NiceMock<Upstream::MockHostDescription>>();
   auto upstream_metadata = std::make_shared<envoy::config::core::v3::Metadata>(
       replaceInvalidStringValues(stream_info.upstream_metadata()));
@@ -168,6 +167,8 @@ inline std::unique_ptr<TestStreamInfo> fromStreamInfo(const test::fuzz::StreamIn
   test_stream_info->upstream_local_address_ = upstream_local_address;
   test_stream_info->downstream_address_provider_ =
       std::make_shared<Network::SocketAddressSetterImpl>(address, address);
+  test_stream_info->downstream_address_provider_->setRequestedServerName(
+      stream_info.requested_server_name());
   auto connection_info = std::make_shared<NiceMock<Ssl::MockConnectionInfo>>();
   ON_CALL(*connection_info, subjectPeerCertificate())
       .WillByDefault(testing::ReturnRef(TestSubjectPeer));