SNI dynamic forward proxy: Support saving resolved upstream address (e…

…nvoyproxy#37099)

Commit Message: Save resolved upstream address in filter state in SNI
dynamic forward proxy
Additional Description:
Risk Level: Low
Testing: Added unit tests and am also consuming this change from filter
state in a subsequent filter
Docs Changes: Fixed a typo in doc and added new field to proto
Release Notes: Added a description in change log
Platform Specific Features:

Signed-off-by: Santosh Rao <santosh.bl@gmail.com>

api/envoy/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto

@@ -33,4 +33,10 @@ message FilterConfig {
     // The port number to connect to the upstream.
     uint32 port_value = 2 [(validate.rules).uint32 = {lte: 65535 gt: 0}];
   }
+
+  // When this flag is set, the filter will add the resolved upstream address in the filter
+  // state. The state should be saved with key
+  // ``envoy.stream.upstream_address`` (See
+  // :repo:`upstream_address.h<source/common/stream_info/upstream_address.h>`).
+  bool save_upstream_address = 3;
 }

changelogs/current.yaml

@@ -243,6 +243,10 @@ new_features:
 - area: ext_authz
   change: |
     added filter state field latency_us, bytesSent and bytesReceived access for CEL and logging.
+- area: sni_dynamic_forward_proxy
+  change: |
+    Added support in SNI dynamic forward proxy for saving the resolved upstream address in the filter state.
+    The state is saved with the key ``envoy.stream.upstream_address``.
 
 deprecated:
 - area: rbac

docs/root/configuration/listeners/network_filters/sni_dynamic_forward_proxy_filter.rst

@@ -36,6 +36,6 @@ dynamically set by other network filters on a per-connection basis by setting a
 state object under the key ``envoy.upstream.dynamic_host``. Additionally, the SNI dynamic forward
 proxy uses the default port filter configuration as target port, but it can by dynamically set,
 by setting a per-connection state object under the key ``envoy.upstream.dynamic_port``. If these
-objects are set, they take precedence over the SNI value and default port. In case that the overrided
+objects are set, they take precedence over the SNI value and default port. In case that the overridden
 port is out of the valid port range, the overriding value will be ignored and the default port
 configured will be used. See the implementation for the details.

source/extensions/filters/network/sni_dynamic_forward_proxy/BUILD

@@ -21,7 +21,9 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/common:minimal_logger_lib",
         "//source/common/stream_info:uint32_accessor_lib",
+        "//source/common/stream_info:upstream_address_lib",
         "//source/common/tcp_proxy",
+        "//source/common/upstream:upstream_includes",
         "//source/extensions/common/dynamic_forward_proxy:dns_cache_interface",
         "@envoy_api//envoy/extensions/filters/network/sni_dynamic_forward_proxy/v3:pkg_cc_proto",
     ],

source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc

@@ -8,6 +8,7 @@
 
 #include "source/common/common/assert.h"
 #include "source/common/stream_info/uint32_accessor_impl.h"
+#include "source/common/stream_info/upstream_address.h"
 #include "source/common/tcp_proxy/tcp_proxy.h"
 
 namespace Envoy {
@@ -20,7 +21,8 @@ ProxyFilterConfig::ProxyFilterConfig(
     Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactory& cache_manager_factory,
     Upstream::ClusterManager&)
     : port_(static_cast<uint16_t>(proto_config.port_value())),
-      dns_cache_manager_(cache_manager_factory.get()) {
+      dns_cache_manager_(cache_manager_factory.get()),
+      save_upstream_address_(proto_config.save_upstream_address()) {
   auto cache_or_error = dns_cache_manager_->getCache(proto_config.dns_cache_config());
   THROW_IF_NOT_OK_REF(cache_or_error.status());
   dns_cache_ = std::move(cache_or_error.value());
@@ -84,11 +86,16 @@ Network::FilterStatus ProxyFilter::onNewConnection() {
   }
 
   switch (result.status_) {
-  case LoadDnsCacheEntryStatus::InCache:
+  case LoadDnsCacheEntryStatus::InCache: {
     ASSERT(cache_load_handle_ == nullptr);
     ENVOY_CONN_LOG(debug, "DNS cache entry already loaded, continuing",
                    read_callbacks_->connection());
+    auto const& host_info = result.host_info_;
+    if (host_info.has_value() && host_info.value()->address()) {
+      addHostAddressToFilterState(host_info.value()->address());
+    }
     return Network::FilterStatus::Continue;
+  }
   case LoadDnsCacheEntryStatus::Loading:
     ASSERT(cache_load_handle_ != nullptr);
     ENVOY_CONN_LOG(debug, "waiting to load DNS cache entry", read_callbacks_->connection());
@@ -104,14 +111,39 @@ Network::FilterStatus ProxyFilter::onNewConnection() {
   PANIC_DUE_TO_CORRUPT_ENUM;
 }
 
-void ProxyFilter::onLoadDnsCacheComplete(const Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) {
+void ProxyFilter::onLoadDnsCacheComplete(
+    const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) {
   ENVOY_CONN_LOG(debug, "load DNS cache complete, continuing", read_callbacks_->connection());
   ASSERT(circuit_breaker_ != nullptr);
   circuit_breaker_.reset();
+
+  if (host_info && host_info->address()) {
+    addHostAddressToFilterState(host_info->address());
+  }
+
   read_callbacks_->connection().readDisable(false);
   read_callbacks_->continueReading();
 }
 
+void ProxyFilter::addHostAddressToFilterState(
+    const Network::Address::InstanceConstSharedPtr& address) {
+  ASSERT(address); // null pointer checks must be done before calling this function.
+
+  if (!config_->saveUpstreamAddress()) {
+    return;
+  }
+
+  ENVOY_CONN_LOG(trace, "Adding resolved host {} to filter state", read_callbacks_->connection(),
+                 address->asString());
+
+  auto address_obj = std::make_unique<StreamInfo::UpstreamAddress>();
+  address_obj->address_ = address;
+
+  read_callbacks_->connection().streamInfo().filterState()->setData(
+      StreamInfo::UpstreamAddress::key(), std::move(address_obj),
+      StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection);
+}
+
 } // namespace SniDynamicForwardProxy
 } // namespace NetworkFilters
 } // namespace Extensions

source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h

@@ -5,6 +5,7 @@
 #include "envoy/upstream/cluster_manager.h"
 
 #include "source/common/common/logger.h"
+#include "source/common/upstream/upstream_impl.h"
 #include "source/extensions/common/dynamic_forward_proxy/dns_cache.h"
 
 namespace Envoy {
@@ -24,11 +25,13 @@ class ProxyFilterConfig {
 
   Extensions::Common::DynamicForwardProxy::DnsCache& cache() { return *dns_cache_; }
   uint32_t port() { return port_; }
+  bool saveUpstreamAddress() const { return save_upstream_address_; };
 
 private:
   const uint32_t port_;
   const Extensions::Common::DynamicForwardProxy::DnsCacheManagerSharedPtr dns_cache_manager_;
   Extensions::Common::DynamicForwardProxy::DnsCacheSharedPtr dns_cache_;
+  const bool save_upstream_address_;
 };
 
 using ProxyFilterConfigSharedPtr = std::shared_ptr<ProxyFilterConfig>;
@@ -54,6 +57,8 @@ class ProxyFilter
       const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr&) override;
 
 private:
+  void addHostAddressToFilterState(const Network::Address::InstanceConstSharedPtr& address);
+
   const ProxyFilterConfigSharedPtr config_;
   Upstream::ResourceAutoIncDecPtr circuit_breaker_;
   Extensions::Common::DynamicForwardProxy::DnsCache::LoadDnsCacheEntryHandlePtr cache_load_handle_;

test/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter_test.cc

@@ -3,6 +3,7 @@
 
 #include "source/common/router/string_accessor_impl.h"
 #include "source/common/stream_info/uint32_accessor_impl.h"
+#include "source/common/stream_info/upstream_address.h"
 #include "source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h"
 #include "source/extensions/filters/network/well_known_names.h"
 
@@ -14,6 +15,7 @@
 
 using testing::AtLeast;
 using testing::Eq;
+using testing::InSequence;
 using testing::NiceMock;
 using testing::Return;
 using testing::ReturnRef;
@@ -33,7 +35,9 @@ class SniDynamicProxyFilterTest
     : public testing::Test,
       public Extensions::Common::DynamicForwardProxy::DnsCacheManagerFactory {
 public:
-  SniDynamicProxyFilterTest() {
+  void SetUp() override { setupFilter(); }
+
+  virtual void setupFilter() {
     FilterConfig proto_config;
     proto_config.set_port_value(443);
     EXPECT_CALL(*dns_cache_manager_, getCache(_));
@@ -98,7 +102,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCache) {
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -119,7 +123,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithHostFromFilterState) {
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -140,7 +144,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPortFromFilterState) {
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -162,7 +166,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithHostAndPortFromFilterState) {
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -183,7 +187,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPort0FromFilterState) {
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -204,7 +208,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPortAboveLimitFromFilterState)
 
   EXPECT_CALL(callbacks_, continueReading());
   filter_->onLoadDnsCacheComplete(
-      std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>());
+      std::make_shared<NiceMock<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>>());
 
   EXPECT_CALL(*handle, onDestroy());
 }
@@ -314,6 +318,94 @@ TEST_F(SniDynamicProxyFilterTest, CircuitBreakerInvoked) {
   EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection());
 }
 
+class UpstreamResolvedHostFilterStateHelper : public SniDynamicProxyFilterTest {
+public:
+  void setupFilter() override {
+    FilterConfig proto_config;
+    proto_config.set_port_value(443);
+    proto_config.set_save_upstream_address(true);
+
+    EXPECT_CALL(*dns_cache_manager_, getCache(_));
+    filter_config_ = std::make_shared<ProxyFilterConfig>(proto_config, *this, cm_);
+    filter_ = std::make_unique<ProxyFilter>(filter_config_);
+    filter_->initializeReadFilterCallbacks(callbacks_);
+
+    // Allow for an otherwise strict mock.
+    ON_CALL(callbacks_, connection()).WillByDefault(ReturnRef(connection_));
+    EXPECT_CALL(callbacks_, connection()).Times(AtLeast(0));
+  }
+};
+
+// Tests if an already existing address set in filter state is updated when upstream host is
+// resolved successfully.
+TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetadata) {
+  // Pre-populate the filter state with an address.
+  const auto pre_address = Network::Utility::parseInternetAddressNoThrow("1.2.3.3", 443);
+  auto address_obj = std::make_unique<StreamInfo::UpstreamAddress>();
+  address_obj->address_ = pre_address;
+  connection_.streamInfo().filterState()->setData(
+      StreamInfo::UpstreamAddress::key(), std::move(address_obj),
+      StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection);
+
+  InSequence s;
+
+  // Setup test host
+  auto host_info = std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>();
+  host_info->address_ = Network::Utility::parseInternetAddressNoThrow("1.2.3.4", 443);
+
+  EXPECT_CALL(connection_, requestedServerName()).WillRepeatedly(Return(""));
+  setFilterStateHost("foo");
+  Upstream::ResourceAutoIncDec* circuit_breakers_{
+      new Upstream::ResourceAutoIncDec(pending_requests_)};
+  EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_())
+      .WillOnce(Return(circuit_breakers_));
+  EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _, _))
+      .WillOnce(Invoke([&](absl::string_view, uint16_t, bool,
+                           ProxyFilter::LoadDnsCacheEntryCallbacks&) {
+        return MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::InCache, nullptr, host_info};
+      }));
+
+  EXPECT_CALL(*host_info, address()).Times(2).WillRepeatedly(Return(host_info->address_));
+
+  // Host was resolved successfully, so continue filter iteration.
+  EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection());
+
+  const StreamInfo::UpstreamAddress* updated_address_obj =
+      connection_.streamInfo().filterState()->getDataReadOnly<StreamInfo::UpstreamAddress>(
+          StreamInfo::UpstreamAddress::key());
+
+  // Verify the data
+  EXPECT_TRUE(updated_address_obj->address_);
+  EXPECT_EQ(updated_address_obj->address_->asStringView(), host_info->address_->asStringView());
+}
+
+// Tests if address set is populated in the filter state when an upstream host is resolved
+// successfully but is null.
+TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddress) {
+  InSequence s;
+
+  // Setup test host
+  auto host_info = std::make_shared<Extensions::Common::DynamicForwardProxy::MockDnsHostInfo>();
+  host_info->address_ = nullptr;
+
+  EXPECT_CALL(connection_, requestedServerName()).WillRepeatedly(Return(""));
+  setFilterStateHost("foo");
+  Upstream::ResourceAutoIncDec* circuit_breakers_{
+      new Upstream::ResourceAutoIncDec(pending_requests_)};
+  EXPECT_CALL(*dns_cache_manager_->dns_cache_, canCreateDnsRequest_())
+      .WillOnce(Return(circuit_breakers_));
+  EXPECT_CALL(*dns_cache_manager_->dns_cache_, loadDnsCacheEntry_(Eq("foo"), 443, _, _))
+      .WillOnce(Invoke([&](absl::string_view, uint16_t, bool,
+                           ProxyFilter::LoadDnsCacheEntryCallbacks&) {
+        return MockLoadDnsCacheEntryResult{LoadDnsCacheEntryStatus::InCache, nullptr, host_info};
+      }));
+
+  EXPECT_CALL(*host_info, address());
+
+  // Host was not resolved, still continue filter iteration.
+  EXPECT_EQ(Network::FilterStatus::Continue, filter_->onNewConnection());
+}
+
 } // namespace
 
 } // namespace SniDynamicForwardProxy