apple dns: resolve IP addresses without calling Apple APIs#13698
apple dns: resolve IP addresses without calling Apple APIs#13698junr03 merged 6 commits intoenvoyproxy:masterfrom junr03:local-resolution
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
| // After production testing with this resolver implementation it became empirically demonstrable | ||
| // that Apple's API issues queries to the DNS server for dns_name that might already be an IP | ||
| // address. In contrast, c-ares synchronously resolves such cases. Moreover, some DNS servers | ||
| // might not resolve IP addresses and thus result in callback targets that never get a |
There was a problem hiding this comment.
I think phrasing it this way is a little bit backwards. Even if we have an example of a DNS server that does "resolve" an IP address, I wouldn't bet on this being standard or expected. I think this comment can probably be truncated to something along the lines of, "When an IP address is submitted to c-ares it synchronously returns it without submitting a DNS query. Because we've come to rely on this behavior, this resolver implements a similar resolution path to avoid making improper DNS queries for resolved IPs."
Signed-off-by: Jose Nino <jnino@lyft.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small comment.
/wait
| try { | ||
| callback(DnsResolver::ResolutionStatus::Success, | ||
| {DnsResponse(address, std::chrono::seconds(60))}); | ||
| return nullptr; | ||
| } catch (const std::exception& e) { | ||
| ENVOY_LOG(warn, "std::exception in callback with local resolution: {}", e.what()); | ||
| throw EnvoyException(e.what()); | ||
| } catch (...) { | ||
| ENVOY_LOG(warn, "Unknown exception in callback with local resolution"); | ||
| throw EnvoyException("unknown"); |
There was a problem hiding this comment.
Why is this try catch needed vs. just letting the callback through if that is what it does?
There was a problem hiding this comment.
I was imitating the c-ares resolver which catches other exception types and throws them as EnvoyExceptions (plus post the throw). But, yeah if we don't mind about the exception type I can just call the callback without the try.
There was a problem hiding this comment.
Given that we are calling code we own, I don't know why we would be catching other types and wrapping them, so I would just remove all of this.
Signed-off-by: Jose Nino <jnino@lyft.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
I know it is a CI infra issue but I'd like to see the mac os test green here given the PR is for a apple resolver. If the CI error stays after a retest I will merge knowing that the test passes locally 😓 |
* master: dns: preserve custom resolver after channel destruction (envoyproxy#13820) docs: Further updates to quick-start (envoyproxy#13793) wasm: update V8 to v8.7.220.10. (envoyproxy#13568) test: adding a test for CONNECT to an IP address (envoyproxy#13818) [fuzz] Scaled Load balancer fuzz to 60k hosts (envoyproxy#13771) Removed Circle-CI reference. (envoyproxy#13824) wasm: strip only Custom Sections with precompiled Wasm modules. (envoyproxy#13775) examples: Add dynamic configuration (filesystem) sandbox (envoyproxy#13783) apple dns: resolve IP addresses without calling Apple APIs (envoyproxy#13698) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
3 participants
Commit Message: apple dns - resolve IP addresses without calling Apple APIs
Additional Description: After production testing with this resolver implementation it became empirically demonstrable that Apple's API issues queries to the DNS server for dns_name that might already be an IP address. In contrast, c-ares synchronously resolves such cases. Moreover, some DNS servers might not resolve IP addresses and thus result in callback targets that never get a resolution. Therefore, short circuiting was added to parse the dns_name into an internet address and synchronously issue the ResolveCb; only if the parsing throws an exception the resolver issues a call to Apple's API.
Risk Level: med - important changes to the resolver, but the resolver has low production usage.
Testing: local testing with iOS devices with Envoy Mobile. And added unit tests.
Signed-off-by: Jose Nino jnino@lyft.com