Skip to content

DNS: officially (previously only unofficially) allowing null resolutions #19588

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
alyssawilk merged 6 commits into from
Jan 24, 2022
Merged

DNS: officially (previously only unofficially) allowing null resolutions #19588

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0c2594
DNS: officially (previously only unofficially) allowing null resolutions
alyssawilk Jan 18, 2022
c60a27d
adding flag
alyssawilk Jan 18, 2022
86b54b0
Merge branch 'main' into dns2
alyssawilk Jan 18, 2022
e6d7ef6
comment
alyssawilk Jan 18, 2022
0a21c0c
update mocks
alyssawilk Jan 19, 2022
9d32ce0
Merge branch 'main' into dns2
alyssawilk Jan 20, 2022
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
2 changes: 1 addition & 1 deletion source/extensions/common/dynamic_forward_proxy/dns_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class DnsHostInfo {

/**
* Returns the host's currently resolved address. This address may change periodically due to
* async re-resolution.
* async re-resolution. This address may be null in the case of failed resolution.
*/
virtual Network::Address::InstanceConstSharedPtr address() const PURE;

Expand Down
30 changes: 19 additions & 11 deletions source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, absl::stri
struct ResponseStringValues {
const std::string DnsCacheOverflow = "DNS cache overflow";
const std::string PendingRequestOverflow = "Dynamic forward proxy pending request overflow";
const std::string DnsResolutionFailure = "DNS resolution failure";
};

struct RcDetailsValues {
const std::string DnsCacheOverflow = "dns_cache_overflow";
const std::string PendingRequestOverflow = "dynamic_forward_proxy_pending_request_overflow";
const std::string DnsResolutionFailure = "dns_resolution_failure";
};

using CustomClusterType = envoy::config::cluster::v3::Cluster::CustomClusterType;
Expand Down Expand Up @@ -144,6 +146,12 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea

auto const& host = config_->cache().getHost(headers.Host()->value().getStringView());
if (host.has_value()) {
if (!host.value()->address()) {
decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable,
ResponseStrings::get().DnsResolutionFailure, nullptr,
absl::nullopt, RcDetails::get().DnsResolutionFailure);
return Http::FilterHeadersStatus::StopIteration;

@adisuissa adisuissa Jan 18, 2022

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.

Consider calling latchTime(decoder_callbacks_, DNS_END); before this line (or in onDnsResolutionFail().

}
addHostAddressToFilterState(host.value()->address());
}

Expand All @@ -165,22 +173,13 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea
NOT_REACHED_GCOVR_EXCL_LINE;
}

void ProxyFilter::addHostAddressToFilterState(
const Network::Address::InstanceConstSharedPtr& address) {
void ProxyFilter::addHostAddressToFilterState(const Network::Address::InstanceConstSharedPtr& address) {
ASSERT(address); // null pointer checks must be done before calling this function.

@RyanTheOptimist RyanTheOptimist Jan 18, 2022

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 was confused for a minute about how a "const T&" could even be null.... then I realized that T is "SharedPtr". Carry on! facepalm


if (!config_->saveUpstreamAddress()) {
return;
}

// `onLoadDnsCacheComplete` is called by DNS cache on first resolution even if there was a
// resolution failure (null address). This check makes sure that we do not add null address to
// FilterState when this happens.
if (!address) {
ENVOY_STREAM_LOG(debug, "Cannot add address to filter state: invalid address",
*decoder_callbacks_);
return;
}

ENVOY_STREAM_LOG(trace, "Adding resolved host {} to filter state", *decoder_callbacks_,
address->asString());

Expand All @@ -203,6 +202,15 @@ void ProxyFilter::onLoadDnsCacheComplete(
ASSERT(circuit_breaker_ != nullptr);
circuit_breaker_.reset();

if (!host_info->address()) {
// Generally in Envoy it is not Ok to send a local reply at 2 code points with the same
// details, but here we could leak prior queries if we differentiate new failures from
// cached failures, so intentionally reuse the details.

@RyanTheOptimist RyanTheOptimist Jan 18, 2022

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.

Maybe we could chat offline, but I'm not sure that I really understand. Is this comment observing that in ProxyFilter::decodeHeaders() we send the same details that we're sending here? And normally that would be verboten. But if we returned a new error code here, we'd leak to the caller that a previous resolution had failed? Is that a privacy leak or some such?

@alyssawilk alyssawilk Jan 18, 2022

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 worried this is more confusing than it's worth. Looking at our TODOs I think I also want to add a response flag, then refactored, so I think I can just remove this comment.

largely when we sendLocalReply we're supposed to be able to use rc-details to track exactly what went wrong. generally I'd want separate codes for synchronous-dns-fail and async-dns-fail but that leaks prior requests so I'm intentionally not doing it. I don't think anyone else would realize the quandry so removing for clarity :-P

@RyanTheOptimist RyanTheOptimist Jan 18, 2022

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.

SGTM!

decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable,
ResponseStrings::get().DnsResolutionFailure, nullptr,
absl::nullopt, RcDetails::get().DnsResolutionFailure);
return;
}
addHostAddressToFilterState(host_info->address());

decoder_callbacks_->continueDecoding();
Expand Down
4 changes: 4 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "test/integration/http_integration.h"
#include "test/integration/ssl_utility.h"

using testing::HasSubstr;

namespace Envoy {
namespace {

Expand Down Expand Up @@ -195,6 +197,7 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithBody) {
// Currently if the first DNS resolution fails, the filter will continue with
// a null address. Make sure this mode fails gracefully.
TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) {
useAccessLog("%RESPONSE_CODE_DETAILS%");
initializeWithArgs();
codec_client_ = makeHttpConnection(lookupPort("http"));
const Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"},
Expand All @@ -205,6 +208,7 @@ TEST_P(ProxyFilterIntegrationTest, RequestWithUnknownDomain) {
auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("503", response->headers().getStatusValue());
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("dns_resolution_failure"));
}

// Verify that after we populate the cache and reload the cluster we reattach to the cache with
Expand Down
22 changes: 9 additions & 13 deletions test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, AddResolvedHostFilterStateMetadata
return host_info;
}));

EXPECT_CALL(*host_info, address());
EXPECT_CALL(*host_info, address()).Times(2).WillRepeatedly(Return(host_info->address_));

EXPECT_CALL(callbacks_, streamInfo());
EXPECT_CALL(callbacks_, streamInfo());
Expand Down Expand Up @@ -468,7 +468,7 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, UpdateResolvedHostFilterStateMetad
return host_info;
}));

EXPECT_CALL(*host_info, address());
EXPECT_CALL(*host_info, address()).Times(2).WillRepeatedly(Return(host_info->address_));

EXPECT_CALL(callbacks_, dispatcher());
EXPECT_CALL(callbacks_, streamInfo());
Expand Down Expand Up @@ -500,9 +500,6 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre
Upstream::ResourceAutoIncDec* circuit_breakers_(
new Upstream::ResourceAutoIncDec(pending_requests_));

EXPECT_CALL(callbacks_, streamInfo());
auto& filter_state = callbacks_.streamInfo().filterState();

InSequence s;

// Setup test host
Expand All @@ -529,14 +526,13 @@ TEST_F(UpstreamResolvedHostFilterStateHelper, IgnoreFilterStateMetadataNullAddre
}));

EXPECT_CALL(*host_info, address());
EXPECT_CALL(callbacks_, streamInfo());
EXPECT_CALL(callbacks_, dispatcher());

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false));

// We do not expect FilterState to be populated
EXPECT_FALSE(
filter_state->hasData<StreamInfo::UpstreamAddress>(StreamInfo::UpstreamAddress::key()));
EXPECT_CALL(callbacks_,
sendLocalReply(Http::Code::ServiceUnavailable, Eq("DNS resolution failure"), _, _,
Eq("dns_resolution_failure")));
EXPECT_CALL(callbacks_, encodeHeaders_(_, false));
EXPECT_CALL(callbacks_, encodeData(_, true));
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration,
filter_->decodeHeaders(request_headers_, false));

filter_->onDestroy();
}
Expand Down