greendns: don't contact nameservers if one entry is returned from hosts file #512
Conversation
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
======================================
+ Coverage 46% 46% +<1%
======================================
Files 81 81
Lines 7945 7948 +3
Branches 1359 1361 +2
======================================
+ Hits 3659 3661 +2
- Misses 4033 4034 +1
Partials 253 253
Continue to review full report at Codecov.
|
eventlet/support/greendns.py
Outdated
| @@ -398,10 +399,12 @@ def getaliases(self, hostname): | |||
| resolver = ResolverProxy(hosts_resolver=HostsResolver()) | |||
|
|
|||
|
|
|||
| def resolve(name, family=socket.AF_INET, raises=True, _proxy=None): | |||
| def resolve(name, family=socket.AF_INET, raises=True, _proxy=None, | |||
| use_dns=True): | |||
There was a problem hiding this comment.
Unless this name is commonly known in other APIs, could you please rename it to use_network?
| if answer.rrset: | ||
| addrs.extend(rr.address for rr in answer.rrset) | ||
| if addrs: | ||
| break | ||
| if err is not None and not addrs: | ||
| raise err | ||
| elif family == socket.AF_INET6 and flags & socket.AI_V4MAPPED: |
There was a problem hiding this comment.
When resolve() family is specified, it works as before, going to network. Is that how getaddrinfo works also?
There was a problem hiding this comment.
There's another else block below too.
There was a problem hiding this comment.
When resolve() family is specified it works as before, ie. returning the entry on hosts file and not using network if it's there.
My change basically applies to getaddrinfo() where if a IPv4 or IPv6 entry was fetched from /etc/hosts it won't attempt to get it from the network. Before this patch, if for example only an IPv4 entry was found, it'll query the network for the IPv6 resolution. Makes sense?
There was a problem hiding this comment.
Yes, sorry, I meant _getaddrinfo_lookup() with family not AF_UNSPEC.
There was a problem hiding this comment.
Yes if family not AF_UNSPEC then my patch is not changing the behavior
There was a problem hiding this comment.
It should, to match system getaddrinfo, right?
There was a problem hiding this comment.
@temoto before my patch, individual calls to resolve() (ie. family not being AF_UNSPEC) were already checking hosts file first. Problem was that calls with AF_UNSPEC will call twice to resolve (one with AF_INET6 and one with AF_INET) and if only one (4 or 6) entry is present in hosts file, the other call will contact the network.
|
Could you write a test for new behavior? |
|
I sent a new version with the test |
tests/greendns_test.py
Outdated
| base.rr.address = addr_from_ns | ||
| base.rr6.address = addr6_from_ns | ||
| rp._resolver = base() | ||
| greendns.resolver = rp |
There was a problem hiding this comment.
This modifies global state for all other tests. Please use @/with mock.patch or isolated test or try/finally.
…ts file getaddrinfo() behaves differently from the standard implementation as it will try to contact nameservers if only one (IPv4 or IPv6) entry is returned from /etc/hosts file. This patch avoids getaddrinfo() querying nameservers if at least one entry is fetched through the hosts file to match the behavior of the original socket.getaddrinfo() implementation. Closes eventlet#515 Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
|
@jstasiak review please |
|
Looks good to me! :) |
getaddrinfo() behaves differently from the standard implementation as
it will try to contact nameservers if only one (IPv4 or IPv6) entry
is returned from /etc/hosts file.
This patch avoids getaddrinfo() querying nameservers if at least one
entry is fetched through the hosts file to match the behavior of
the original socket.getaddrinfo() implementation.
Closes #515
Signed-off-by: Daniel Alvarez dalvarez@redhat.com