access log: make %DOWNSTREAM_DIRECT_REMOTE_ADDRESS% work correctly with PROXY protocol

docs/root/intro/version_history.rst

@@ -5,6 +5,7 @@ Version history
 ================
 * access loggers: access logger extensions use the "envoy.access_loggers" name space. A mapping
   of extension names is available in the :ref:`deprecated <deprecated>` documentation.
+* access log: fix %DOWSTREAM_DIRECT_REMOTE_ADDRESS% when used with PROXY protocol listener filter
 * adaptive concurrency: fixed bug that allowed concurrency limits to drop below the configured
   minimum.
 * aws_request_signing: a few fixes so that it works with S3.

include/envoy/network/connection.h

@@ -172,6 +172,13 @@ class Connection : public Event::DeferredDeletable, public FilterManager {
    */
   virtual const Network::Address::InstanceConstSharedPtr& remoteAddress() const PURE;
 
+  /**
+   * @return The address of the remote directly connected peer. Note that this method
+   * will never return nullptr. This address is not affected or modified by PROXY protocol
+   * or any other listener filter.
+   */
+  virtual const Network::Address::InstanceConstSharedPtr& directRemoteAddress() const PURE;
+
   /**
    * Credentials of the peer of a socket as decided by SO_PEERCRED.
    */

include/envoy/network/listen_socket.h

@@ -191,6 +191,12 @@ class ConnectionSocket : public virtual Socket {
    */
   virtual const Address::InstanceConstSharedPtr& remoteAddress() const PURE;
 
+  /**
+   * @return the direct remote address of the socket. This is the address of the directly
+   *         connected peer, and cannot be modified by listener filters.
+   */
+  virtual const Address::InstanceConstSharedPtr& directRemoteAddress() const PURE;
+
   /**
    * Restores the local address of the socket. On accepted sockets the local address defaults to the
    * one at which the connection was received at, which is the same as the listener's address, if

source/common/http/conn_manager_impl.cc

@@ -578,7 +578,7 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
   stream_info_.setDownstreamLocalAddress(
       connection_manager_.read_callbacks_->connection().localAddress());
   stream_info_.setDownstreamDirectRemoteAddress(
-      connection_manager_.read_callbacks_->connection().remoteAddress());
+      connection_manager_.read_callbacks_->connection().directRemoteAddress());
   // Initially, the downstream remote address is the source address of the
   // downstream connection. That can change later in the request's lifecycle,
   // based on XFF processing, but setting the downstream remote address here

source/common/network/connection_impl.h

@@ -68,6 +68,9 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback
   const Address::InstanceConstSharedPtr& remoteAddress() const override {
     return socket_->remoteAddress();
   }
+  const Address::InstanceConstSharedPtr& directRemoteAddress() const override {
+    return socket_->directRemoteAddress();
+  }
   const Address::InstanceConstSharedPtr& localAddress() const override {
     return socket_->localAddress();
   }

source/common/network/listen_socket_impl.h

@@ -120,13 +120,17 @@ class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket {
   ConnectionSocketImpl(IoHandlePtr&& io_handle,
                        const Address::InstanceConstSharedPtr& local_address,
                        const Address::InstanceConstSharedPtr& remote_address)
-      : SocketImpl(std::move(io_handle), local_address), remote_address_(remote_address) {}
+      : SocketImpl(std::move(io_handle), local_address), remote_address_(remote_address),
+        direct_remote_address_(remote_address) {}
 
   // Network::Socket
   Address::SocketType socketType() const override { return Address::SocketType::Stream; }
 
   // Network::ConnectionSocket
   const Address::InstanceConstSharedPtr& remoteAddress() const override { return remote_address_; }
+  const Address::InstanceConstSharedPtr& directRemoteAddress() const override {
+    return direct_remote_address_;
+  }
   void restoreLocalAddress(const Address::InstanceConstSharedPtr& local_address) override {
     setLocalAddress(local_address);
     local_address_restored_ = true;
@@ -158,6 +162,7 @@ class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket {
 
 protected:
   Address::InstanceConstSharedPtr remote_address_;
+  const Address::InstanceConstSharedPtr direct_remote_address_;
   bool local_address_restored_{false};
   std::string transport_protocol_;
   std::vector<std::string> application_protocols_;

source/common/tcp_proxy/tcp_proxy.cc

@@ -255,6 +255,8 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec
   read_callbacks_->connection().enableHalfClose(true);
   getStreamInfo().setDownstreamLocalAddress(read_callbacks_->connection().localAddress());
   getStreamInfo().setDownstreamRemoteAddress(read_callbacks_->connection().remoteAddress());
+  getStreamInfo().setDownstreamDirectRemoteAddress(
+      read_callbacks_->connection().directRemoteAddress());
   getStreamInfo().setDownstreamSslConnection(read_callbacks_->connection().ssl());
 
   // Need to disable reads so that we don't write to an upstream that might fail

source/extensions/filters/network/dubbo_proxy/active_message.cc

@@ -192,6 +192,7 @@ ActiveMessage::ActiveMessage(ConnectionManager& parent)
   parent_.stats().request_active_.inc();
   stream_info_.setDownstreamLocalAddress(parent_.connection().localAddress());
   stream_info_.setDownstreamRemoteAddress(parent_.connection().remoteAddress());
+  stream_info_.setDownstreamDirectRemoteAddress(parent_.connection().directRemoteAddress());
 }
 
 ActiveMessage::~ActiveMessage() {

source/extensions/filters/network/thrift_proxy/conn_manager.h

@@ -164,6 +164,8 @@ class ConnectionManager : public Network::ReadFilter,
       stream_info_.setDownstreamLocalAddress(parent_.read_callbacks_->connection().localAddress());
       stream_info_.setDownstreamRemoteAddress(
           parent_.read_callbacks_->connection().remoteAddress());
+      stream_info_.setDownstreamDirectRemoteAddress(
+          parent_.read_callbacks_->connection().directRemoteAddress());
     }
     ~ActiveRpc() override {
       request_timer_->complete();

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

@@ -105,6 +105,13 @@ QuicFilterManagerConnectionImpl::remoteAddress() const {
   return quic_connection_->connectionSocket()->remoteAddress();
 }
 
+const Network::Address::InstanceConstSharedPtr&
+QuicFilterManagerConnectionImpl::directRemoteAddress() const {
+  ASSERT(quic_connection_->connectionSocket() != nullptr,
+         "directRemoteAddress() should only be called after OnPacketHeader");
+  return quic_connection_->connectionSocket()->directRemoteAddress();
+}
+
 const Network::Address::InstanceConstSharedPtr&
 QuicFilterManagerConnectionImpl::localAddress() const {
   ASSERT(quic_connection_->connectionSocket() != nullptr,

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

@@ -44,6 +44,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase {
   void detectEarlyCloseWhenReadDisabled(bool /*value*/) override { NOT_REACHED_GCOVR_EXCL_LINE; }
   bool readEnabled() const override { return true; }
   const Network::Address::InstanceConstSharedPtr& remoteAddress() const override;
+  const Network::Address::InstanceConstSharedPtr& directRemoteAddress() const override;
   const Network::Address::InstanceConstSharedPtr& localAddress() const override;
   absl::optional<Network::Connection::UnixDomainSocketPeerCredentials>
   unixSocketPeerCredentials() const override {

source/server/api_listener_impl.h

@@ -106,6 +106,9 @@ class ApiListenerImplBase : public ApiListener,
       const Network::Address::InstanceConstSharedPtr& remoteAddress() const override {
         return parent_.parent_.address();
       }
+      const Network::Address::InstanceConstSharedPtr& directRemoteAddress() const override {
+        return parent_.parent_.address();
+      }
       absl::optional<Network::Connection::UnixDomainSocketPeerCredentials>
       unixSocketPeerCredentials() const override {
         return absl::nullopt;

test/extensions/access_loggers/grpc/tcp_grpc_access_log_integration_test.cc

@@ -98,6 +98,7 @@ class TcpGrpcAccessLogIntegrationTest : public Grpc::GrpcClientIntegrationParamT
     // Clear fields which are not deterministic.
     auto* log_entry = request_msg.mutable_tcp_logs()->mutable_log_entry(0);
     clearPort(*log_entry->mutable_common_properties()->mutable_downstream_remote_address());
+    clearPort(*log_entry->mutable_common_properties()->mutable_downstream_direct_remote_address());
     clearPort(*log_entry->mutable_common_properties()->mutable_downstream_local_address());
     clearPort(*log_entry->mutable_common_properties()->mutable_upstream_remote_address());
     clearPort(*log_entry->mutable_common_properties()->mutable_upstream_local_address());
@@ -181,13 +182,17 @@ TEST_P(TcpGrpcAccessLogIntegrationTest, BasicAccessLogFlow) {
         socket_address:
           address: {}
       upstream_cluster: cluster_0
+      downstream_direct_remote_address:
+        socket_address:
+          address: {}
     connection_properties:
       received_bytes: 3
       sent_bytes: 5
 )EOF",
                   VersionInfo::version(), Network::Test::getLoopbackAddressString(ipVersion()),
                   Network::Test::getLoopbackAddressString(ipVersion()),
                   Network::Test::getLoopbackAddressString(ipVersion()),
+                  Network::Test::getLoopbackAddressString(ipVersion()),
                   Network::Test::getLoopbackAddressString(ipVersion()))));
 
   cleanup();

test/integration/proxy_proto_integration_test.cc

@@ -92,6 +92,33 @@ TEST_P(ProxyProtoIntegrationTest, RouterProxyUnknownLongRequestAndResponseWithBo
   testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
 }
 
+// Test that %DOWNSTREAM_DIRECT_REMOTE_ADDRESS%/%DOWNSTREAM_DIRECT_REMOTE_ADDRESS_WITHOUT_PORT%
+// returns the direct address, and %DOWSTREAM_REMOTE_ADDRESS% returns the proxy-protocol-provided
+// address.
+TEST_P(ProxyProtoIntegrationTest, AccessLog) {
+  useAccessLog("%DOWNSTREAM_DIRECT_REMOTE_ADDRESS_WITHOUT_PORT% %DOWNSTREAM_REMOTE_ADDRESS%");
+
+  // Tell HCM to ignore x-forwarded-for so that the proxy-proto address is used.
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) -> void { hcm.mutable_use_remote_address()->set_value(true); });
+
+  ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr {
+    Network::ClientConnectionPtr conn = makeClientConnection(lookupPort("http"));
+    Buffer::OwnedImpl buf("PROXY TCP4 1.2.3.4 254.254.254.254 12345 1234\r\n");
+    conn->write(buf, false);
+    return conn;
+  };
+
+  testRouterRequestAndResponseWithBody(1024, 512, false, &creator);
+  const std::string log_line = waitForAccessLog(access_log_name_);
+  const std::vector<absl::string_view> tokens = StringUtil::splitToken(log_line, " ");
+
+  ASSERT_EQ(2, tokens.size());
+  EXPECT_EQ(tokens[0], Network::Test::getLoopbackAddressString(GetParam()));
+  EXPECT_EQ(tokens[1], "1.2.3.4:12345");
+}
+
 TEST_P(ProxyProtoIntegrationTest, DEPRECATED_FEATURE_TEST(OriginalDst)) {
   // Change the cluster to an original destination cluster. An original destination cluster
   // ignores the configured hosts, and instead uses the restored destination address from the

test/mocks/network/connection.cc

@@ -62,6 +62,7 @@ template <class T> static void initializeMockConnection(T& connection) {
     connection.raiseEvent(Network::ConnectionEvent::LocalClose);
   }));
   ON_CALL(connection, remoteAddress()).WillByDefault(ReturnRef(connection.remote_address_));
+  ON_CALL(connection, directRemoteAddress()).WillByDefault(ReturnRef(connection.remote_address_));
   ON_CALL(connection, localAddress()).WillByDefault(ReturnRef(connection.local_address_));
   ON_CALL(connection, id()).WillByDefault(Return(connection.next_id_));
   ON_CALL(connection, state()).WillByDefault(ReturnPointee(&connection.state_));

test/mocks/network/connection.h

@@ -67,6 +67,7 @@ class MockConnection : public Connection, public MockConnectionBase {
   MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool));
   MOCK_METHOD(bool, readEnabled, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const));
+  MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const));
   MOCK_METHOD(absl::optional<Connection::UnixDomainSocketPeerCredentials>,
               unixSocketPeerCredentials, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const));
@@ -112,6 +113,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase
   MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool));
   MOCK_METHOD(bool, readEnabled, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const));
+  MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const));
   MOCK_METHOD(absl::optional<Connection::UnixDomainSocketPeerCredentials>,
               unixSocketPeerCredentials, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const));
@@ -160,6 +162,7 @@ class MockFilterManagerConnection : public FilterManagerConnection, public MockC
   MOCK_METHOD(void, detectEarlyCloseWhenReadDisabled, (bool));
   MOCK_METHOD(bool, readEnabled, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const));
+  MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const));
   MOCK_METHOD(absl::optional<Connection::UnixDomainSocketPeerCredentials>,
               unixSocketPeerCredentials, (), (const));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, localAddress, (), (const));

test/mocks/network/mocks.cc

@@ -149,6 +149,7 @@ MockConnectionSocket::MockConnectionSocket()
       remote_address_(new Address::Ipv4Instance(80)) {
   ON_CALL(*this, localAddress()).WillByDefault(ReturnRef(local_address_));
   ON_CALL(*this, remoteAddress()).WillByDefault(ReturnRef(remote_address_));
+  ON_CALL(*this, directRemoteAddress()).WillByDefault(ReturnRef(remote_address_));
   ON_CALL(*this, ioHandle()).WillByDefault(ReturnRef(*io_handle_));
   ON_CALL(testing::Const(*this), ioHandle()).WillByDefault(ReturnRef(*io_handle_));
 }

test/mocks/network/mocks.h

@@ -253,6 +253,7 @@ class MockConnectionSocket : public ConnectionSocket {
   MOCK_METHOD(bool, localAddressRestored, (), (const));
   MOCK_METHOD(void, setRemoteAddress, (const Address::InstanceConstSharedPtr&));
   MOCK_METHOD(const Address::InstanceConstSharedPtr&, remoteAddress, (), (const));
+  MOCK_METHOD(const Address::InstanceConstSharedPtr&, directRemoteAddress, (), (const));
   MOCK_METHOD(void, setDetectedTransportProtocol, (absl::string_view));
   MOCK_METHOD(absl::string_view, detectedTransportProtocol, (), (const));
   MOCK_METHOD(void, setRequestedApplicationProtocols, (const std::vector<absl::string_view>&));

test/server/filter_chain_benchmark_test.cc

@@ -67,6 +67,10 @@ class MockConnectionSocket : public Network::ConnectionSocket {
     return remote_address_;
   }
 
+  const Network::Address::InstanceConstSharedPtr& directRemoteAddress() const override {
+    return remote_address_;
+  }
+
   const Network::Address::InstanceConstSharedPtr& localAddress() const override {
     return local_address_;
   }