From 7975a2b760c55dba724bd6c5dba2909e86aa309c Mon Sep 17 00:00:00 2001 From: Santosh Rao Date: Wed, 20 Nov 2024 18:14:15 -0800 Subject: [PATCH] SNI dynamic forward proxy: Support saving resolved upstream address (#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 --- .../v3/sni_dynamic_forward_proxy.proto | 6 + changelogs/current.yaml | 4 + .../sni_dynamic_forward_proxy_filter.rst | 2 +- .../network/sni_dynamic_forward_proxy/BUILD | 2 + .../sni_dynamic_forward_proxy/proxy_filter.cc | 38 ++++++- .../sni_dynamic_forward_proxy/proxy_filter.h | 5 + .../proxy_filter_test.cc | 106 ++++++++++++++++-- 7 files changed, 152 insertions(+), 11 deletions(-) diff --git a/api/envoy/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto b/api/envoy/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto index 1ab471e0d772..0f01889c402d 100644 --- a/api/envoy/extensions/filters/network/sni_dynamic_forward_proxy/v3/sni_dynamic_forward_proxy.proto +++ b/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`). + bool save_upstream_address = 3; } diff --git a/changelogs/current.yaml b/changelogs/current.yaml index fc0eb42b3e03..442a77d31d08 100644 --- a/changelogs/current.yaml +++ b/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 diff --git a/docs/root/configuration/listeners/network_filters/sni_dynamic_forward_proxy_filter.rst b/docs/root/configuration/listeners/network_filters/sni_dynamic_forward_proxy_filter.rst index 1caa881e3a4e..4d64134b3c8e 100644 --- a/docs/root/configuration/listeners/network_filters/sni_dynamic_forward_proxy_filter.rst +++ b/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. diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/BUILD b/source/extensions/filters/network/sni_dynamic_forward_proxy/BUILD index 1f523fd0e360..749f18bdaacf 100644 --- a/source/extensions/filters/network/sni_dynamic_forward_proxy/BUILD +++ b/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", ], diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc index 3a45c764bebf..44758ecd4a1a 100644 --- a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.cc +++ b/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(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(); + 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 diff --git a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h b/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h index ccba394462ce..02fa055c435e 100644 --- a/source/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter.h +++ b/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; @@ -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_; diff --git a/test/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter_test.cc b/test/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter_test.cc index 59a824c553a2..a3fff449dd39 100644 --- a/test/extensions/filters/network/sni_dynamic_forward_proxy/proxy_filter_test.cc +++ b/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()); + std::make_shared>()); EXPECT_CALL(*handle, onDestroy()); } @@ -119,7 +123,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithHostFromFilterState) { EXPECT_CALL(callbacks_, continueReading()); filter_->onLoadDnsCacheComplete( - std::make_shared()); + std::make_shared>()); EXPECT_CALL(*handle, onDestroy()); } @@ -140,7 +144,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPortFromFilterState) { EXPECT_CALL(callbacks_, continueReading()); filter_->onLoadDnsCacheComplete( - std::make_shared()); + std::make_shared>()); EXPECT_CALL(*handle, onDestroy()); } @@ -162,7 +166,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithHostAndPortFromFilterState) { EXPECT_CALL(callbacks_, continueReading()); filter_->onLoadDnsCacheComplete( - std::make_shared()); + std::make_shared>()); EXPECT_CALL(*handle, onDestroy()); } @@ -183,7 +187,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPort0FromFilterState) { EXPECT_CALL(callbacks_, continueReading()); filter_->onLoadDnsCacheComplete( - std::make_shared()); + std::make_shared>()); EXPECT_CALL(*handle, onDestroy()); } @@ -204,7 +208,7 @@ TEST_F(SniDynamicProxyFilterTest, LoadDnsCacheWithPortAboveLimitFromFilterState) EXPECT_CALL(callbacks_, continueReading()); filter_->onLoadDnsCacheComplete( - std::make_shared()); + std::make_shared>()); 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(proto_config, *this, cm_); + filter_ = std::make_unique(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(); + 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(); + 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::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(); + 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