DNS: officially (previously only unofficially) allowing null resolutions#19588
DNS: officially (previously only unofficially) allowing null resolutions#19588alyssawilk merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
As this is extension code I'm inclined to with you two signing off but if you want a senior maintainer pass LMK. |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Looks great to me, other than a question about a comment that I don't quite grok.
| 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. |
There was a problem hiding this comment.
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 (!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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
/lgtm api
Minor comment on the code, but otherwise LGTM.
| if (host.has_value()) { | ||
| if (!host.value()->address()) { | ||
| onDnsResolutionFail(); | ||
| return Http::FilterHeadersStatus::StopIteration; |
There was a problem hiding this comment.
Consider calling latchTime(decoder_callbacks_, DNS_END); before this line (or in onDnsResolutionFail().
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
|
||
| void ProxyFilter::onDnsResolutionFail() { | ||
| decoder_callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::DnsResolutionFailed); | ||
| decoder_callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, |
There was a problem hiding this comment.
@Augustyniak @buildbreaker @goaway @jpsim @Reflejo FYI, now we will have a more explicit error for DNS failures from the forward proxy filter. Rather than the upstream failure error we have gotten used to.
…ons (envoyproxy#19588) in envoyproxy#17645 there was a bunch of discussion around the DNS cache returning null addresses and how to handle it. After discussion on envoyproxy#19461 we agreed to keep sending null updates, but to fast-fail if no address was resolved. Risk Level: Medium (data plane change) Testing: updated integration tests, unit tests Docs Changes: n/a Release Notes: inline Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Josh Perry <josh.perry@mx.com>
4 participants
in #17645 there was a bunch of discussion around the DNS cache returning null addresses and how to handle it. After discussion on #19461 we agreed to keep sending null updates, but to fast-fail if no address was resolved.
Risk Level: Medium (data plane change)
Testing: updated integration tests, unit tests
Docs Changes: n/a
Release Notes: inline