Skip to content
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.

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

greendns: Pre-1.10.0 dnspython support #341

Closed
wants to merge 1 commit into from

Conversation

tipabu
Copy link
Contributor

@tipabu tipabu commented Aug 18, 2016

Following commit 861d684, eventlet cannot be installed alongside dnspython<1.10.0. This breaks on Ubuntu 12.04, for example, which ships 1.9.4 as a system package.

There are two parts to the fix:

Since version 1.10.0, dnspython's inet_aton functions would raise dns.exception.SyntaxError on invalid address strings. Prior to that, however, inet_aton was a thin wrapper around socket.inet_aton and would therefore raise socket.error on invalid addresses. So we'll catch either error when checking ipv4/6 addresses.

Additionally, dnspython 1.10.0 introduced the LRUCache class. We should fall back to the interval-based Cache class if LRUCache is not available. Note that it is already being used in clear().

Prior to commit 861d684, either of these would have been caught as an ImportError in eventlet/green/socket.py and we would have just assumed dnspython wasn't available.

Following commit 861d684, eventlet cannot be installed alongside
dnspython<1.10.0. This breaks on Ubuntu 12.04, for example, which ships
1.9.4 as a system package.

There are two parts to the fix:

Since version 1.10.0, dnspython's inet_aton functions would raise
dns.exception.SyntaxError on invalid address strings. Prior to that,
however, inet_aton was a thin wrapper around socket.inet_aton and would
therefore raise socket.error on invalid addresses. So we'll catch either
error when checking ipv4/6 addresses.

Additionally, dnspython 1.10.0 introduced the LRUCache class. We should
fall back to the interval-based Cache class if LRUCache is not
available. Note that it is already being used in clear().

Prior to commit 861d684, either of these would have been caught as an
ImportError in eventlet/green/socket.py and we would have just assumed
dnspython wasn't available.
@temoto
Copy link
Member

temoto commented Aug 19, 2016

The patch makes sense. But I can't understand the goal.

Can you explain how somebody would want to install latest eventlet in system packages of old freeze-release OS?

I know two good ways:

  • virtualenv/pip, rely on upstream maintainers, don't care about system packages
  • apt, rely on distro maintainers, don't care about latest upstream

@tipabu
Copy link
Contributor Author

tipabu commented Aug 19, 2016

Ah, but since eventlet doesn't have a dependency on dnspython, pip install -U eventlet won't automatically drag dnspython forward.

Suppose you had two packages you care about, A (which depends on dnspython) and B (which depends on eventlet). Years ago, you installed both of them, and life was good. As time passed, you upgraded B regularly (maybe you need some new features, or security fixes, or what-have-you) but never A (because it still did what you needed and given the costs/benefits of validating that newer versions still worked, it never made sense).

Things are still fine! Currently. Until you try to upgrade B following the next eventlet release, at which point B drags forward eventlet, and eventlet fails to install with something akin to

  Running setup.py egg_info for package from file:///home/user/eventlet
    Traceback (most recent call last):
      File "<string>", line 14, in <module>
      File "/home/user/eventlet/setup.py", line 3, in <module>
        from eventlet import __version__
      File "eventlet/__init__.py", line 10, in <module>
        from eventlet import convenience
      File "eventlet/convenience.py", line 6, in <module>
        from eventlet.green import socket
      File "eventlet/green/socket.py", line 17, in <module>
        from eventlet.support import greendns
      File "eventlet/support/greendns.py", line 332, in <module>
        resolver = ResolverProxy(hosts_resolver=HostsResolver())
      File "eventlet/support/greendns.py", line 161, in __init__
        self._load()
      File "eventlet/support/greendns.py", line 197, in _load
        if is_ipv4_addr(ip):
      File "eventlet/support/greendns.py", line 88, in is_ipv4_addr
        dns.ipv4.inet_aton(host)
    socket.error: illegal IP address string passed to inet_aton

... at which point you're thoroughly confused and have no idea that:

  1. you need to pip install -U A (or at least dnspython) before pip install -U B will work and
  2. the root cause of your troubles was not isolating A and B in separate virtualenvs. Maybe that was intentional and you need to be able to import from both A and B (in which case, you probably should have been upgrading A all along), or maybe that was just an under-appreciation of why you should be using virtualenvs.

(Alternatively, you could be in my position, of stupidly using the system dnspython while also wanting to ship latest eventlet on 12.04. But that's my own idiocy, and I should probably just go package and ship latest dnspython as well 😛 -- I just figured I ought to put this out there to save other people headaches.)

@temoto
Copy link
Member

temoto commented Aug 20, 2016

Thanks for sharing this issue. So we have a new kind of dependency: not required, but only compatible with some version.

For those who google install eventlet socket.error illegal IP address string passed to inet_aton: first do pip install -U dnspython.

I think solution is we should bundle dnspython. That also makes DNS requests always green. Such a good day.

temoto added a commit that referenced this pull request Aug 20, 2016
Fixes installation issue when older dnspython is present in system packages
#341
@temoto
Copy link
Member

temoto commented Aug 20, 2016

Please review 0983889

temoto added a commit that referenced this pull request Aug 20, 2016
Fixes installation issue when older dnspython is present in system packages
#341
temoto added a commit that referenced this pull request Aug 22, 2016
Fixes installation issue when older dnspython is present in system packages
#341
@temoto
Copy link
Member

temoto commented Aug 22, 2016

Thanks for feedback, please, review new version 95a3159

@tipabu
Copy link
Contributor Author

tipabu commented Aug 24, 2016

Change makes sense, and I can successfully install bundle-dns eventlet alongside old dnspython. I noticed that python -c 'import eventlet, dns; print dns.__file__; print eventlet.__file__' produced different output than python -c 'import dns, eventlet; print dns.__file__; print eventlet.__file__' (which notably included a traceback following an ImportError caught in init.py), but I'm not sure we can easily resolve that.

temoto added a commit that referenced this pull request Aug 24, 2016
Fixes installation issue when older dnspython is present in system packages
#341
@temoto temoto added this to the v0.20 milestone Aug 24, 2016
@temoto
Copy link
Member

temoto commented Aug 24, 2016

Well it should produce different output, right?
import dns, eventlet - first imports old dnspython from system packages.
import eventlet - also imports bundled dnspython, which stays in sys.modules, so latter import dns is a no-op.

Anyway, bundled dns is now merged in master 52b09be

@temoto temoto closed this Aug 24, 2016
@temoto
Copy link
Member

temoto commented Aug 24, 2016

Thinking about it more, import dns, eventlet could be a time bomb, because for what I know, it makes Eventlet use already imported old dnspython. Not sure if Eventlet should be so rude to push existing dns out of sys.modules though.

@tipabu
Copy link
Contributor Author

tipabu commented Aug 25, 2016

Thanks!

FWIW, it looks like requests handles a similar situation with urllib3 by using relative imports for their vendored version. I.e., working with eventlet.support.dns instead of just dns. Looks like that may require us to change the imports in dnspython to be relative, though.

@tipabu tipabu deleted the old-dnspython branch August 25, 2016 23:44
@temoto
Copy link
Member

temoto commented Aug 26, 2016

Yeah, at first I tried to go with relative imports, started writing a patch change imports in dnspython, imagined how much work it would be to update the patch to next version of dnspython and fainted. It is still an option if current approach leads to more troubles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants