c-ares resolver: add option to use nameservers as fallback#19010
c-ares resolver: add option to use nameservers as fallback#19010
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Jose Nino <jnino@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, some small comments as a first pass.
/wait
|
@mattklein123 updated! I cleaned up with a variant. I think usage is clearer now. |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks some more comments.
/wait
|
/wait |
Signed-off-by: Jose Nino <jnino@lyft.com>
|
/wait |
Signed-off-by: Jose Nino <jnino@lyft.com>
|
This test made me realize the original implementation only worked on Android but not a general posix system. I have moved to an alternate implementation that uses c-ares' default (loopback) as a sentinel value for deciding if the fallback resolvers should be used. A nice side effect is that I was able to do this using the c-ares API used for overriding resolvers so the code is generally simpler. |
snowp
left a comment
There was a problem hiding this comment.
Thanks this looks good to me for the most part, a few comments
api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto
Outdated
Show resolved
Hide resolved
api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto
Outdated
Show resolved
Hide resolved
| // Otherwise, the resolvers listed in the resolvers list will override the default system | ||
| // resolvers. Defaults to false. | ||
| bool use_resolvers_as_fallback = 3; |
There was a problem hiding this comment.
Just sanity checking: leaving this as false means the behavior of setting resolvers is unchanged yes?
| const bool ret = servers == nullptr || (servers->next == nullptr && servers->family == AF_INET && | ||
| servers->addr.addr4.s_addr == htonl(INADDR_LOOPBACK) && | ||
| servers->udp_port == 0 && servers->tcp_port == 0); |
There was a problem hiding this comment.
For my understanding what kind of nameserver is this? Is it actually useful for resolving names or just a sentinel?
There was a problem hiding this comment.
it is loopback on port 0. Which is what ares sets in init_by_defaults. So extremely likely not useful, I actually wish that c-ares defaulted to returning an error code instead of "init by default". It doesn't preclude someone from setting up a DNS name server on loopback on a different port.
| // Given that the local machine will have a working conf in resolve.conf the resolver will not | ||
| // use the fallback given. |
There was a problem hiding this comment.
Any way to test the inverse? A case where we'll use the fallback
There was a problem hiding this comment.
I tried for a bit, but couldn't defeat all the fallbacks available on linux, i.e., I was not able to break the machine enough to get to init_by_defaults.
FWIW I have tested on android where the fallback is used. I can try to see if I can get there with a unit test again.
|
Thanks @snowp, updated! |
|
/retest |
|
Retrying Azure Pipelines: |
|
/lgtm api |
got and updated approval from @snowp
…y#19010) Commit Message: c-ares resolver: add option to use name servers as fallback. Risk Level: low - opt in behavior Testing: existing and additional tests Docs Changes: added Release Notes: added Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Josh Perry <josh.perry@mx.com>
Commit Message: c-ares resolver: add option to use name servers as fallback.
Risk Level: low - opt in behavior
Testing: existing and additional tests
Docs Changes: added
Release Notes: added
Signed-off-by: Jose Nino jnino@lyft.com