From f426016ba1108a1145c9ac302685cff4112c5f38 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 8 Nov 2023 15:50:34 -0500 Subject: [PATCH 1/4] dfp: Insert correct preresolved hostname key in DNS cache Previously, preresolved hostnames didn't include the port in the `host` that is used as a key in the DNS cache. This behavior differed from the regular resolution path which always normalized the host (via DnsHostInfo::normalizeHostForDfp). There was also a bug in the DnsCacheImplTest.PreresolveSuccess test, where the preresolve hostnames config included the port in the socket address, which is not how it would normally be specified and masked this issue. This PR fixes the problem by ensuring the preresolved hostnames have the host normalized (via DnsHostInfo::normalizeHostForDfp) before inserting into the DNS cache, just like the host is normalized before retrieving from the DNS cache. The test issue is also fixed so we can now verify that the correct cache key is being used for insertion, retrieval, and removal, and test coverage is added to ensure we can fetch the cache entry correctly whether the host includes the port or not. Signed-off-by: Ali Beyad --- .../dynamic_forward_proxy/dns_cache_impl.cc | 5 ++- .../dns_cache_impl_test.cc | 37 ++++++++++++------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 2b02e5ca39f32..9d98f5929cb33 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -58,7 +58,10 @@ DnsCacheImpl::DnsCacheImpl( // cache to load an entry. Further if this particular resolution fails all the is lost is the // potential optimization of having the entry be preresolved the first time a true consumer of // this DNS cache asks for it. - startCacheLoad(hostname.address(), hostname.port_value(), false); + const std::string host = + DnsHostInfo::normalizeHostForDfp(hostname.address(), hostname.port_value()); + ENVOY_LOG(debug, "DNS pre-resolve starting for host {}", host); + startCacheLoad(host, hostname.port_value(), false); } } diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 37c798db5b1ee..1e4b3a0058d6f 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -37,15 +37,17 @@ static const absl::optional kNoTtl = absl::nullopt; class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedTime { public: DnsCacheImplTest() : registered_dns_factory_(dns_resolver_factory_) {} - void initialize(std::vector preresolve_hostnames = {}, uint32_t max_hosts = 1024) { + void initialize( + std::vector> preresolve_hostnames = {}, + uint32_t max_hosts = 1024) { config_.set_name("foo"); config_.set_dns_lookup_family(envoy::config::cluster::v3::Cluster::V4_ONLY); config_.mutable_max_hosts()->set_value(max_hosts); if (!preresolve_hostnames.empty()) { - for (const auto& hostname : preresolve_hostnames) { + for (const auto& host_pair : preresolve_hostnames) { envoy::config::core::v3::SocketAddress* address = config_.add_preresolve_hostnames(); - address->set_address(hostname); - address->set_port_value(443); + address->set_address(host_pair.first); + address->set_port_value(host_pair.second); } } @@ -129,18 +131,20 @@ void verifyCaresDnsConfigAndUnpack( TEST_F(DnsCacheImplTest, PreresolveSuccess) { Network::DnsResolver::ResolveCb resolve_cb; - std::string hostname = "bar.baz.com:443"; - EXPECT_CALL(*resolver_, resolve("bar.baz.com", _, _)) + std::string host = "bar.baz.com"; + uint32_t port = 443; + std::string authority = host + ":" + std::to_string(port); + EXPECT_CALL(*resolver_, resolve(host, _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); + EXPECT_CALL( + update_callbacks_, + onDnsHostAddOrUpdate(authority, DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false))); EXPECT_CALL(update_callbacks_, - onDnsHostAddOrUpdate("bar.baz.com:443", - DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false))); - EXPECT_CALL(update_callbacks_, - onDnsResolutionComplete("bar.baz.com:443", + onDnsResolutionComplete(authority, DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false), Network::DnsResolver::ResolutionStatus::Success)); - initialize({"bar.baz.com:443"} /* preresolve_hostnames */); + initialize({{host, port}} /* preresolve_hostnames */); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -148,7 +152,13 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); MockLoadDnsCacheEntryCallbacks callbacks; - auto result = dns_cache_->loadDnsCacheEntry("bar.baz.com", 443, false, callbacks); + // Retrieve with the hostname and port in the "host". + auto result = dns_cache_->loadDnsCacheEntry(authority, port, false, callbacks); + EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_); + EXPECT_EQ(result.handle_, nullptr); + EXPECT_NE(absl::nullopt, result.host_info_); + // Retrieve with the hostname only in the "host". + result = dns_cache_->loadDnsCacheEntry(host, port, false, callbacks); EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_); EXPECT_EQ(result.handle_, nullptr); EXPECT_NE(absl::nullopt, result.host_info_); @@ -156,7 +166,8 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { TEST_F(DnsCacheImplTest, PreresolveFailure) { EXPECT_THROW_WITH_MESSAGE( - initialize({"bar.baz.com"} /* preresolve_hostnames */, 0 /* max_hosts */), EnvoyException, + initialize({{"bar.baz.com", 443}} /* preresolve_hostnames */, 0 /* max_hosts */), + EnvoyException, "DNS Cache [foo] configured with preresolve_hostnames=1 larger than max_hosts=0"); } From 921fd807f0f697dec27ba75166f18575ef27e022 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Wed, 8 Nov 2023 21:48:21 -0500 Subject: [PATCH 2/4] strcat Signed-off-by: Ali Beyad --- .../common/dynamic_forward_proxy/dns_cache_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 1e4b3a0058d6f..b4a7231de7f7e 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -20,6 +20,8 @@ #include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" +#include "absl/strings/str_cat.h" + using testing::AtLeast; using testing::DoAll; using testing::InSequence; @@ -133,7 +135,7 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { Network::DnsResolver::ResolveCb resolve_cb; std::string host = "bar.baz.com"; uint32_t port = 443; - std::string authority = host + ":" + std::to_string(port); + std::string authority = absl::StrCat(host, ":", port); EXPECT_CALL(*resolver_, resolve(host, _, _)) .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); EXPECT_CALL( From 4b98e171e67e701c37ebcbfbaea1048eb4328087 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Thu, 9 Nov 2023 12:22:45 -0500 Subject: [PATCH 3/4] add runtime guard Signed-off-by: Ali Beyad --- changelogs/current.yaml | 7 ++++ source/common/runtime/runtime_features.cc | 1 + .../dynamic_forward_proxy/dns_cache_impl.cc | 6 +++- .../common/dynamic_forward_proxy/BUILD | 1 + .../dns_cache_impl_test.cc | 36 +++++++++++++------ 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 75d150ee9f81c..fa27d72275765 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -60,6 +60,13 @@ bug_fixes: in BUFFERED BodySendMode + SEND HeaderSendMode. It is now external processor's responsibility to set the content length correctly matched to the mutated body. if those two doesn't match, the mutation will be rejected and local reply with error status will be returned. +- area: dynamic_forward_proxy + change: | + Fixed a bug where the preresolved hostnames specified in the Dynamic Forward Proxy cluster + config would not use the normalized hostname as the DNS cache key, which is the same key + used for retrieval. This caused cache misses on initial use, even though the host DNS entry + was pre-resolved. The fix is guarded by runtime guard ``envoy.reloadable_features.normalize_host_for_preresolve_dfp_dns``, + which defaults to true. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index c572b3d2820ac..4dfd3b07cd560 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -65,6 +65,7 @@ RUNTIME_GUARD(envoy_reloadable_features_lowercase_scheme); RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name); RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name); RUNTIME_GUARD(envoy_reloadable_features_no_full_scan_certs_on_sni_mismatch); +RUNTIME_GUARD(envoy_reloadable_features_normalize_host_for_preresolve_dfp_dns); RUNTIME_GUARD(envoy_reloadable_features_oauth_make_token_cookie_httponly); RUNTIME_GUARD(envoy_reloadable_features_oauth_use_standard_max_age_value); RUNTIME_GUARD(envoy_reloadable_features_oauth_use_url_encoding); diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 9d98f5929cb33..02afd3a48161f 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -9,6 +9,7 @@ #include "source/common/network/dns_resolver/dns_factory_util.h" #include "source/common/network/resolver_impl.h" #include "source/common/network/utility.h" +#include "source/common/runtime/runtime_features.h" namespace Envoy { namespace Extensions { @@ -59,7 +60,10 @@ DnsCacheImpl::DnsCacheImpl( // potential optimization of having the entry be preresolved the first time a true consumer of // this DNS cache asks for it. const std::string host = - DnsHostInfo::normalizeHostForDfp(hostname.address(), hostname.port_value()); + (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.normalize_host_for_preresolve_dfp_dns")) + ? DnsHostInfo::normalizeHostForDfp(hostname.address(), hostname.port_value()) + : hostname.address(); ENVOY_LOG(debug, "DNS pre-resolve starting for host {}", host); startCacheLoad(host, hostname.port_value(), false); } diff --git a/test/extensions/common/dynamic_forward_proxy/BUILD b/test/extensions/common/dynamic_forward_proxy/BUILD index ea7ed5952f453..8625b7c3bb7ca 100644 --- a/test/extensions/common/dynamic_forward_proxy/BUILD +++ b/test/extensions/common/dynamic_forward_proxy/BUILD @@ -24,6 +24,7 @@ envoy_cc_test( "//test/mocks/thread_local:thread_local_mocks", "//test/test_common:registry_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/common/dynamic_forward_proxy/v3:pkg_cc_proto", diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index b4a7231de7f7e..6b70a21a1c126 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/thread_local/mocks.h" #include "test/test_common/registry.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "absl/strings/str_cat.h" @@ -131,13 +132,26 @@ void verifyCaresDnsConfigAndUnpack( typed_dns_resolver_config.typed_config().UnpackTo(&cares); } -TEST_F(DnsCacheImplTest, PreresolveSuccess) { +class DnsCacheImplPreresolveTest : public DnsCacheImplTest, + public testing::WithParamInterface { +public: + bool normalizeDfpHost() { return GetParam(); } +}; + +INSTANTIATE_TEST_SUITE_P(DnsCachePreresolveNormalizedDfpHost, DnsCacheImplPreresolveTest, + testing::Bool()); + +TEST_P(DnsCacheImplPreresolveTest, PreresolveSuccess) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.normalize_host_for_preresolve_dfp_dns", + absl::StrCat(normalizeDfpHost())}}); + Network::DnsResolver::ResolveCb resolve_cb; std::string host = "bar.baz.com"; uint32_t port = 443; std::string authority = absl::StrCat(host, ":", port); EXPECT_CALL(*resolver_, resolve(host, _, _)) - .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); + .WillRepeatedly(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); EXPECT_CALL( update_callbacks_, onDnsHostAddOrUpdate(authority, DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false))); @@ -146,7 +160,7 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false), Network::DnsResolver::ResolutionStatus::Success)); - initialize({{host, port}} /* preresolve_hostnames */); + initialize({{normalizeDfpHost() ? host : authority, port}} /* preresolve_hostnames */); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -154,19 +168,21 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); MockLoadDnsCacheEntryCallbacks callbacks; - // Retrieve with the hostname and port in the "host". - auto result = dns_cache_->loadDnsCacheEntry(authority, port, false, callbacks); - EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_); - EXPECT_EQ(result.handle_, nullptr); - EXPECT_NE(absl::nullopt, result.host_info_); + if (normalizeDfpHost()) { + // Retrieve with the hostname and port in the "host". + auto result = dns_cache_->loadDnsCacheEntry(authority, port, false, callbacks); + EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_); + EXPECT_EQ(result.handle_, nullptr); + EXPECT_NE(absl::nullopt, result.host_info_); + } // Retrieve with the hostname only in the "host". - result = dns_cache_->loadDnsCacheEntry(host, port, false, callbacks); + auto result = dns_cache_->loadDnsCacheEntry(host, port, false, callbacks); EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::InCache, result.status_); EXPECT_EQ(result.handle_, nullptr); EXPECT_NE(absl::nullopt, result.host_info_); } -TEST_F(DnsCacheImplTest, PreresolveFailure) { +TEST_P(DnsCacheImplPreresolveTest, PreresolveFailure) { EXPECT_THROW_WITH_MESSAGE( initialize({{"bar.baz.com", 443}} /* preresolve_hostnames */, 0 /* max_hosts */), EnvoyException, From 26b097881d6118401cc8750ffc5a7d33a40dcfb1 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Fri, 10 Nov 2023 10:42:30 -0500 Subject: [PATCH 4/4] lizan feedback Signed-off-by: Ali Beyad --- .../common/dynamic_forward_proxy/dns_cache_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 6b70a21a1c126..283496e843698 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -47,10 +47,10 @@ class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedT config_.set_dns_lookup_family(envoy::config::cluster::v3::Cluster::V4_ONLY); config_.mutable_max_hosts()->set_value(max_hosts); if (!preresolve_hostnames.empty()) { - for (const auto& host_pair : preresolve_hostnames) { + for (const auto& [host, port] : preresolve_hostnames) { envoy::config::core::v3::SocketAddress* address = config_.add_preresolve_hostnames(); - address->set_address(host_pair.first); - address->set_port_value(host_pair.second); + address->set_address(host); + address->set_port_value(port); } }