Skip to content

dfp: Insert correct preresolved hostname key in DNS cache #30784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abeyad merged 5 commits into from
Nov 10, 2023
Merged

dfp: Insert correct preresolved hostname key in DNS cache #30784

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f426016
dfp: Insert correct preresolved hostname key in DNS cache
abeyad Nov 8, 2023
921fd80
strcat
abeyad Nov 9, 2023
4b98e17
add runtime guard
abeyad Nov 9, 2023
26b0978
lizan feedback
abeyad Nov 10, 2023
97da9ef
Merge remote-tracking branch 'upstream/main' into dfp_dns_cache
abeyad Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ DnsCacheImpl::DnsCacheImpl(
// cache to load an entry. Further if this particular resolution fails all the is lost is the

@RyanTheOptimist RyanTheOptimist Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm trying to comment on line 56)

Wow, is this the correct type for hostname?

repeated config.core.v3.SocketAddress preresolve_hostnames = 10;

Hostname is a bizarre term for this as it seems to be a protocol (tcp/udp), an IP address, a port, and a resolver name. Am I reading that correctly? bizarre

@abeyad abeyad Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also surprised SocketAddress was used for preresolved hostnames config, and doesn't seem to be the right structure to represent a DNS cache entry. Changing it is a good idea but probably outside the scope of the PR. I do share your concern though..

// 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());

@RyanTheOptimist RyanTheOptimist Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a runtime guard for this, since it looks like a behavior change? (Or is there some reason we're safe here)

@abeyad abeyad Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was debating this myself, and could be convinced of adding a runtime guard. I just figured that no one is using this currently (preresolved hostnames config), and in any case, all that will happen if the cache key change causes a cache miss is that the DNS record will have to be fetched (which is actually what was happening previously on "production" use cases, i.e. my traffic director integration test, without us knowing about it, you can just tell from the logs).

@RyanTheOptimist RyanTheOptimist Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd be inclined to add a guard (which will default to true) since it's easy to add and better safe than sorry. I think we know that nobody is using the mobile APIs for this config, but I'm not sure if we know about DFP users in the wild?

@abeyad abeyad Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, i had assumed DFP is only used for mobile but didn't consider the possibility it's used outside of mobile. runtime guard has been added!

ENVOY_LOG(debug, "DNS pre-resolve starting for host {}", host);
startCacheLoad(host, hostname.port_value(), false);
}
}

Expand Down
37 changes: 24 additions & 13 deletions test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,17 @@ static const absl::optional<std::chrono::seconds> kNoTtl = absl::nullopt;
class DnsCacheImplTest : public testing::Test, public Event::TestUsingSimulatedTime {
public:
DnsCacheImplTest() : registered_dns_factory_(dns_resolver_factory_) {}
void initialize(std::vector<std::string> preresolve_hostnames = {}, uint32_t max_hosts = 1024) {
void initialize(
std::vector<std::pair<std::string /*host*/, uint32_t /*port*/>> 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) {

@lizan lizan Nov 10, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
for (const auto& host_pair : preresolve_hostnames) {
for (const auto& [host, port] : preresolve_hostnames) {

@abeyad abeyad Nov 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the recommendation, done!

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);
}
}

Expand Down Expand Up @@ -129,34 +131,43 @@ 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);

@RyanTheOptimist RyanTheOptimist Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think absl::StrCat() is the canonical way to do this sort of concatenation.

@abeyad abeyad Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i was debating whether to do that and add the new include or just use + and to_string() (since I see test code that use these quite liberally), but i just changed it to StrCat

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"}));
checkStats(1 /* attempt */, 1 /* success */, 0 /* failure */, 1 /* address changed */,
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_);
}

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");
}

Expand Down