diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9acc3bf57f14..5e3489b53212 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -81,6 +81,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 ea7a92e65f54..5b3f34c34de4 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 2b02e5ca39f3..02afd3a48161 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 { @@ -58,7 +59,13 @@ 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 = + (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 ea7ed5952f45..8625b7c3bb7c 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 37c798db5b1e..283496e84369 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,8 +18,11 @@ #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" + using testing::AtLeast; using testing::DoAll; using testing::InSequence; @@ -37,15 +40,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, port] : 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); + address->set_port_value(port); } } @@ -127,20 +132,35 @@ 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 hostname = "bar.baz.com:443"; - EXPECT_CALL(*resolver_, resolve("bar.baz.com", _, _)) - .WillOnce(DoAll(SaveArg<2>(&resolve_cb), Return(&resolver_->active_query_))); - EXPECT_CALL(update_callbacks_, - onDnsHostAddOrUpdate("bar.baz.com:443", - DnsHostInfoEquals("10.0.0.1:443", "bar.baz.com", false))); + std::string host = "bar.baz.com"; + uint32_t port = 443; + std::string authority = absl::StrCat(host, ":", port); + EXPECT_CALL(*resolver_, resolve(host, _, _)) + .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))); 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({{normalizeDfpHost() ? host : authority, port}} /* preresolve_hostnames */); resolve_cb(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"10.0.0.1"})); @@ -148,15 +168,24 @@ TEST_F(DnsCacheImplTest, PreresolveSuccess) { 1 /* added */, 0 /* removed */, 1 /* num hosts */); MockLoadDnsCacheEntryCallbacks callbacks; - auto result = dns_cache_->loadDnsCacheEntry("bar.baz.com", 443, false, callbacks); + 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". + 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"} /* 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"); }