envoyproxy · yanavlasov · Sep 19, 2022 · Sep 15, 2022 · Sep 19, 2022 · Sep 19, 2022
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -38,6 +38,9 @@ behavior_changes:
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
+- area: logging
+  change: |
+    changed the ``UPSTREAM_REMOTE_ADDRESS``, ``UPSTREAM_REMOTE_ADDRESS_WITHOUT_PORT``, and ``UPSTREAM_REMOTE_PORT`` fields to log based on the actual upstream connection rather than the upstream host. This fixes a bug where the address components were not consistently correct for Happy Eyeballs connections and proxied connections, but also means in cases where the host was selected but a connection was not established, the fields will be absent. This change can be temporarily reverted by setting the runtime guard ``envoy.reloadable_features.correct_remote_address`` to false.
 - area: resource_monitors
   change: |
     changed behavior of the fixed heap monitor to count pages allocated to TCMalloc as free memory if it's not used by Envoy. This change can be reverted temporarily by setting the runtime guard ``envoy.reloadable_features.do_not_count_mapped_pages_as_free`` to true.
@@ -59,6 +62,8 @@ minor_behavior_changes:
 - area: http
   change: |
     changed the filter callback interfaces to make sure that downstream-only functionality is explicit.
+  change: |
+    the upstream remote address is now available to downstream filters via the ``upstreamRemoteAddress`` function.
 - area: stats
   change: |
     Default tag extraction rules were changed for ``worker_id`` extraction. Previously, ``worker_`` was removed from the original name during the extraction. This

diff --git a/envoy/http/codec.h b/envoy/http/codec.h
@@ -341,10 +341,10 @@ class Stream : public StreamResetHandler {
   virtual absl::string_view responseDetails() { return ""; }
 
   /**
-   * @return const Address::InstanceConstSharedPtr& the local address of the connection associated
-   * with the stream.
+   * @return const Network::ConnectionInfoProvider& the adderess provider  of the connection
+   * associated with the stream.
    */
-  virtual const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() PURE;
+  virtual const Network::ConnectionInfoProvider& connectionInfoProvider() PURE;
 
   /**
    * Set the flush timeout for the stream. At the codec level this is used to bound the amount of

diff --git a/envoy/router/router.h b/envoy/router/router.h
@@ -1400,13 +1400,13 @@ class GenericConnectionPoolCallbacks {
    * @param upstream supplies the generic upstream for the stream.
    * @param host supplies the description of the host that will carry the request. For logical
    *             connection pools the description may be different each time this is called.
-   * @param upstream_local_address supplies the local address of the upstream connection.
+   * @param connection_info_provider, supplies the address provider of the upstream connection.
    * @param info supplies the stream info object associated with the upstream connection.
    * @param protocol supplies the protocol associated with the upstream connection.
    */
   virtual void onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
                            Upstream::HostDescriptionConstSharedPtr host,
-                           const Network::Address::InstanceConstSharedPtr& upstream_local_address,
+                           const Network::ConnectionInfoProvider& connection_info_provider,
                            StreamInfo::StreamInfo& info,
                            absl::optional<Http::Protocol> protocol) PURE;
 

diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h
@@ -402,6 +402,17 @@ class UpstreamInfo {
    */
   virtual const Network::Address::InstanceConstSharedPtr& upstreamLocalAddress() const PURE;
 
+  /**
+   * @param upstream_remote_address sets the remote address of the upstream connection.
+   */
+  virtual void setUpstreamRemoteAddress(
+      const Network::Address::InstanceConstSharedPtr& upstream_remote_address) PURE;
+
+  /**
+   * @return the upstream remote address.
+   */
+  virtual const Network::Address::InstanceConstSharedPtr& upstreamRemoteAddress() const PURE;
+
   /**
    * @param failure_reason the upstream transport failure reason.
    */

diff --git a/envoy/tcp/upstream.h b/envoy/tcp/upstream.h
@@ -67,14 +67,14 @@ class GenericConnectionPoolCallbacks {
    * @param info supplies the stream info object associated with the upstream connection.
    * @param upstream supplies the generic upstream for the stream.
    * @param host supplies the description of the host that will carry the request.
-   * @param upstream_local_address supplies the local address of the upstream connection.
+   * @param address_provider supplies the address provider of the upstream connection.
    * @param ssl_info supplies the ssl information of the upstream connection.
    */
-  virtual void
-  onGenericPoolReady(StreamInfo::StreamInfo* info, std::unique_ptr<GenericUpstream>&& upstream,
-                     Upstream::HostDescriptionConstSharedPtr& host,
-                     const Network::Address::InstanceConstSharedPtr& upstream_local_address,
-                     Ssl::ConnectionInfoConstSharedPtr ssl_info) PURE;
+  virtual void onGenericPoolReady(StreamInfo::StreamInfo* info,
+                                  std::unique_ptr<GenericUpstream>&& upstream,
+                                  Upstream::HostDescriptionConstSharedPtr& host,
+                                  const Network::ConnectionInfoProvider& address_provider,
+                                  Ssl::ConnectionInfoConstSharedPtr ssl_info) PURE;
 
   /**
    * Called to indicate a failure for GenericConnPool::newStream to establish a stream.

diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -375,8 +375,8 @@ void StreamEncoderImpl::readDisable(bool disable) {
 
 uint32_t StreamEncoderImpl::bufferLimit() const { return connection_.bufferLimit(); }
 
-const Network::Address::InstanceConstSharedPtr& StreamEncoderImpl::connectionLocalAddress() {
-  return connection_.connection().connectionInfoProvider().localAddress();
+const Network::ConnectionInfoProvider& StreamEncoderImpl::connectionInfoProvider() {
+  return connection_.connection().connectionInfoProvider();
 }
 
 static constexpr absl::string_view RESPONSE_PREFIX = "HTTP/1.1 ";

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -62,7 +62,7 @@ class StreamEncoderImpl : public virtual StreamEncoder,
   void readDisable(bool disable) override;
   uint32_t bufferLimit() const override;
   absl::string_view responseDetails() override { return details_; }
-  const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override;
+  const Network::ConnectionInfoProvider& connectionInfoProvider() override;
   void setFlushTimeout(std::chrono::milliseconds) override {
     // HTTP/1 has one stream per connection, thus any data encoded is immediately written to the
     // connection, invoking any watermarks as necessary. There is no internal buffering that would

diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h
@@ -266,8 +266,8 @@ class ConnectionImpl : public virtual Connection,
     void resetStream(StreamResetReason reason) override;
     void readDisable(bool disable) override;
     uint32_t bufferLimit() const override { return pending_recv_data_->highWatermark(); }
-    const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override {
-      return parent_.connection_.connectionInfoProvider().localAddress();
+    const Network::ConnectionInfoProvider& connectionInfoProvider() override {
+      return parent_.connection_.connectionInfoProvider();
     }
     absl::string_view responseDetails() override { return details_; }
     void setAccount(Buffer::BufferMemoryAccountSharedPtr account) override;

diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h
@@ -78,8 +78,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder,
     removeCallbacksHelper(callbacks);
   }
   uint32_t bufferLimit() const override { return send_buffer_simulation_.highWatermark(); }
-  const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override {
-    return connection()->connectionInfoProvider().localAddress();
+  const Network::ConnectionInfoProvider& connectionInfoProvider() override {
+    return connection()->connectionInfoProvider();
   }
 
   void setAccount(Buffer::BufferMemoryAccountSharedPtr account) override {

diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc
@@ -354,6 +354,13 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam
     field_extractor_ = parseSubstitutionFormatField(field_name, formatter_map_);
   } else if (field_name == "UPSTREAM_LOCAL_ADDRESS") {
     field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
+      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correct_remote_address")) {
+        if (stream_info.upstreamInfo().has_value() &&
+            stream_info.upstreamInfo()->upstreamHost()->address()) {
+          return stream_info.upstreamInfo()->upstreamHost()->address()->asString();
+        }
+        return "";
+      }
       if (stream_info.upstreamInfo().has_value() &&
           stream_info.upstreamInfo()->upstreamLocalAddress()) {
         return stream_info.upstreamInfo()->upstreamLocalAddress()->asString();
@@ -362,6 +369,14 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam
     };
   } else if (field_name == "UPSTREAM_LOCAL_ADDRESS_WITHOUT_PORT") {
     field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
+      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correct_remote_address")) {
+        if (stream_info.upstreamInfo().has_value() &&
+            stream_info.upstreamInfo()->upstreamHost()->address()) {
+          return StreamInfo::Utility::formatDownstreamAddressNoPort(
+              *stream_info.upstreamInfo()->upstreamHost()->address());
+        }
+        return "";
+      }
       if (stream_info.upstreamInfo().has_value() &&
           stream_info.upstreamInfo()->upstreamLocalAddress()) {
         return StreamInfo::Utility::formatDownstreamAddressNoPort(
@@ -371,6 +386,14 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam
     };
   } else if (field_name == "UPSTREAM_LOCAL_PORT") {
     field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
+      if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correct_remote_address")) {
+        if (stream_info.upstreamInfo().has_value() &&
+            stream_info.upstreamInfo()->upstreamHost()->address()) {
+          return StreamInfo::Utility::formatDownstreamAddressJustPort(
+              *stream_info.upstreamInfo()->upstreamHost()->address());
+        }
+        return "";
+      }
       if (stream_info.upstreamInfo().has_value() &&
           stream_info.upstreamInfo()->upstreamLocalAddress()) {
         return StreamInfo::Utility::formatDownstreamAddressJustPort(
@@ -380,16 +403,16 @@ StreamInfoHeaderFormatter::StreamInfoHeaderFormatter(absl::string_view field_nam
     };
   } else if (field_name == "UPSTREAM_REMOTE_ADDRESS") {
     field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
-      if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamHost()) {
-        return stream_info.upstreamInfo()->upstreamHost()->address()->asString();
+      if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamRemoteAddress()) {
+        return stream_info.upstreamInfo()->upstreamRemoteAddress()->asString();
       }
       return "";
     };
   } else if (field_name == "UPSTREAM_REMOTE_ADDRESS_WITHOUT_PORT") {
     field_extractor_ = [](const Envoy::StreamInfo::StreamInfo& stream_info) -> std::string {
-      if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamHost()) {
+      if (stream_info.upstreamInfo() && stream_info.upstreamInfo()->upstreamRemoteAddress()) {
         return StreamInfo::Utility::formatDownstreamAddressNoPort(
-            *stream_info.upstreamInfo()->upstreamHost()->address());
+            *stream_info.upstreamInfo()->upstreamRemoteAddress());
       }
       return "";
     };

diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc
@@ -710,10 +710,11 @@ void UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
   onResetStream(reset_reason, transport_failure_reason);
 }
 
-void UpstreamRequest::onPoolReady(
-    std::unique_ptr<GenericUpstream>&& upstream, Upstream::HostDescriptionConstSharedPtr host,
-    const Network::Address::InstanceConstSharedPtr& upstream_local_address,
-    StreamInfo::StreamInfo& info, absl::optional<Http::Protocol> protocol) {
+void UpstreamRequest::onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
+                                  Upstream::HostDescriptionConstSharedPtr host,
+                                  const Network::ConnectionInfoProvider& address_provider,
+                                  StreamInfo::StreamInfo& info,
+                                  absl::optional<Http::Protocol> protocol) {
   // This may be called under an existing ScopeTrackerScopeState but it will unwind correctly.
   ScopeTrackerScopeState scope(&parent_.callbacks()->scope(), parent_.callbacks()->dispatcher());
   ENVOY_STREAM_LOG(debug, "pool ready", *parent_.callbacks());
@@ -763,7 +764,8 @@ void UpstreamRequest::onPoolReady(
   } else {
     upstream_info.setUpstreamFilterState(filter_state);
   }
-  upstream_info.setUpstreamLocalAddress(upstream_local_address);
+  upstream_info.setUpstreamLocalAddress(address_provider.localAddress());
+  upstream_info.setUpstreamRemoteAddress(address_provider.remoteAddress());
   upstream_info.setUpstreamSslConnection(info.downstreamAddressProvider().sslConnection());
 
   if (info.downstreamAddressProvider().connectionID().has_value()) {

diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h
@@ -137,7 +137,7 @@ class UpstreamRequest : public Logger::Loggable<Logger::Id::router>,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
                    Upstream::HostDescriptionConstSharedPtr host,
-                   const Network::Address::InstanceConstSharedPtr& upstream_local_address,
+                   const Network::ConnectionInfoProvider& address_provider,
                    StreamInfo::StreamInfo& info, absl::optional<Http::Protocol> protocol) override;
   UpstreamToDownstream& upstreamToDownstream() override;
 

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -41,6 +41,7 @@ RUNTIME_GUARD(envoy_reloadable_features_cares_accept_nodata);
 RUNTIME_GUARD(envoy_reloadable_features_combine_sds_requests);
 RUNTIME_GUARD(envoy_reloadable_features_conn_pool_delete_when_idle);
 RUNTIME_GUARD(envoy_reloadable_features_conn_pool_new_stream_with_early_data_and_http3);
+RUNTIME_GUARD(envoy_reloadable_features_correct_remote_address);
 RUNTIME_GUARD(envoy_reloadable_features_deprecate_global_ints);
 RUNTIME_GUARD(envoy_reloadable_features_do_not_await_headers_on_upstream_timeout_to_emit_stats);
 RUNTIME_GUARD(envoy_reloadable_features_do_not_count_mapped_pages_as_free);

diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h
@@ -49,10 +49,17 @@ struct UpstreamInfoImpl : public UpstreamInfo {
   const Network::Address::InstanceConstSharedPtr& upstreamLocalAddress() const override {
     return upstream_local_address_;
   }
+  const Network::Address::InstanceConstSharedPtr& upstreamRemoteAddress() const override {
+    return upstream_remote_address_;
+  }
   void setUpstreamLocalAddress(
       const Network::Address::InstanceConstSharedPtr& upstream_local_address) override {
     upstream_local_address_ = upstream_local_address;
   }
+  void setUpstreamRemoteAddress(
+      const Network::Address::InstanceConstSharedPtr& upstream_remote_address) override {
+    upstream_remote_address_ = upstream_remote_address;
+  }
   void setUpstreamTransportFailureReason(absl::string_view failure_reason) override {
     upstream_transport_failure_reason_ = std::string(failure_reason);
   }
@@ -84,6 +91,7 @@ struct UpstreamInfoImpl : public UpstreamInfo {
 
   Upstream::HostDescriptionConstSharedPtr upstream_host_{};
   Network::Address::InstanceConstSharedPtr upstream_local_address_;
+  Network::Address::InstanceConstSharedPtr upstream_remote_address_;
   UpstreamTiming upstream_timing_;
   Ssl::ConnectionInfoConstSharedPtr upstream_ssl_info_;
   absl::optional<uint64_t> upstream_connection_id_;

diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -506,14 +506,15 @@ void Filter::onGenericPoolFailure(ConnectionPool::PoolFailureReason reason,
 void Filter::onGenericPoolReady(StreamInfo::StreamInfo* info,
                                 std::unique_ptr<GenericUpstream>&& upstream,
                                 Upstream::HostDescriptionConstSharedPtr& host,
-                                const Network::Address::InstanceConstSharedPtr& local_address,
+                                const Network::ConnectionInfoProvider& address_provider,
                                 Ssl::ConnectionInfoConstSharedPtr ssl_info) {
   upstream_ = std::move(upstream);
   generic_conn_pool_.reset();
   read_callbacks_->upstreamHost(host);
   StreamInfo::UpstreamInfo& upstream_info = *getStreamInfo().upstreamInfo();
   upstream_info.setUpstreamHost(host);
-  upstream_info.setUpstreamLocalAddress(local_address);
+  upstream_info.setUpstreamLocalAddress(address_provider.localAddress());
+  upstream_info.setUpstreamRemoteAddress(address_provider.remoteAddress());
   upstream_info.setUpstreamSslConnection(ssl_info);
   onUpstreamConnection();
   read_callbacks_->continueReading();

diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h
@@ -341,7 +341,7 @@ class Filter : public Network::ReadFilter,
   // GenericConnectionPoolCallbacks
   void onGenericPoolReady(StreamInfo::StreamInfo* info, std::unique_ptr<GenericUpstream>&& upstream,
                           Upstream::HostDescriptionConstSharedPtr& host,
-                          const Network::Address::InstanceConstSharedPtr& local_address,
+                          const Network::ConnectionInfoProvider& address_provider,
                           Ssl::ConnectionInfoConstSharedPtr ssl_info) override;
   void onGenericPoolFailure(ConnectionPool::PoolFailureReason reason,
                             absl::string_view failure_reason,

diff --git a/source/common/tcp_proxy/upstream.cc b/source/common/tcp_proxy/upstream.cc
@@ -189,7 +189,7 @@ void TcpConnPool::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data
   auto upstream = std::make_unique<TcpUpstream>(std::move(conn_data), upstream_callbacks_);
   callbacks_->onGenericPoolReady(
       &connection.streamInfo(), std::move(upstream), host,
-      latched_data->connection().connectionInfoProvider().localAddress(),
+      latched_data->connection().connectionInfoProvider(),
       latched_data->connection().streamInfo().downstreamAddressProvider().sslConnection());
 }
 
@@ -252,9 +252,9 @@ void HttpConnPool::onPoolReady(Http::RequestEncoder& request_encoder,
 }
 
 void HttpConnPool::onGenericPoolReady(Upstream::HostDescriptionConstSharedPtr& host,
-                                      const Network::Address::InstanceConstSharedPtr& local_address,
+                                      const Network::ConnectionInfoProvider& address_provider,
                                       Ssl::ConnectionInfoConstSharedPtr ssl_info) {
-  callbacks_->onGenericPoolReady(nullptr, std::move(upstream_), host, local_address, ssl_info);
+  callbacks_->onGenericPoolReady(nullptr, std::move(upstream_), host, address_provider, ssl_info);
 }
 
 Http2Upstream::Http2Upstream(Tcp::ConnectionPool::UpstreamCallbacks& callbacks,

diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h
@@ -71,7 +71,7 @@ class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callba
     virtual ~Callbacks() = default;
     virtual void onSuccess(Http::RequestEncoder& request_encoder) {
       ASSERT(conn_pool_ != nullptr);
-      conn_pool_->onGenericPoolReady(host_, request_encoder.getStream().connectionLocalAddress(),
+      conn_pool_->onGenericPoolReady(host_, request_encoder.getStream().connectionInfoProvider(),
                                      ssl_info_);
     }
     virtual void onFailure() {
@@ -91,7 +91,7 @@ class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callba
 
 private:
   void onGenericPoolReady(Upstream::HostDescriptionConstSharedPtr& host,
-                          const Network::Address::InstanceConstSharedPtr& local_address,
+                          const Network::ConnectionInfoProvider& address_provider,
                           Ssl::ConnectionInfoConstSharedPtr ssl_info);
   const TunnelingConfigHelper& config_;
   Http::CodecType type_;