Fix exceptions thrown from cryptography import #16723

cdosborn · 2016-07-15T16:25:26Z

ISSUE TYPE

Bugfix Pull Request

ANSIBLE VERSION

ansible 2.1.0.0
  config file = 
  configured module search path = Default w/o overrides

SUMMARY

Fix exceptions thrown from cryptography import
(see my reasoning below)

To reproduce:

pip install setuptools==3.3 (anything less than 11.3 which is cryptography's requirement)
pip install ansible=2.1.0.0 (or latest)
ansible --version

Before:

$ ansible --version
ERROR! Unexpected Exception: (setuptools 3.3 (/usr/lib/python2.7/dist-packages), Requirement.parse('setuptools>=11.3'))
the full traceback was:

Traceback (most recent call last):
  File "/usr/local/bin/ansible", line 81, in <module>
    from ansible.cli.adhoc import AdHocCLI as mycli
  File "/usr/local/lib/python2.7/dist-packages/ansible/cli/adhoc.py", line 28, in <module>
    from ansible.executor.task_queue_manager import TaskQueueManager
  File "/usr/local/lib/python2.7/dist-packages/ansible/executor/task_queue_manager.py", line 28, in <module>
    from ansible.executor.play_iterator import PlayIterator
  File "/usr/local/lib/python2.7/dist-packages/ansible/executor/play_iterator.py", line 29, in <module>
    from ansible.playbook.block import Block
  File "/usr/local/lib/python2.7/dist-packages/ansible/playbook/__init__.py", line 25, in <module>
    from ansible.playbook.play import Play
  File "/usr/local/lib/python2.7/dist-packages/ansible/playbook/play.py", line 27, in <module>
    from ansible.playbook.base import Base
  File "/usr/local/lib/python2.7/dist-packages/ansible/playbook/base.py", line 35, in <module>
    from ansible.parsing.dataloader import DataLoader
  File "/usr/local/lib/python2.7/dist-packages/ansible/parsing/dataloader.py", line 33, in <module>
    from ansible.parsing.vault import VaultLib
  File "/usr/local/lib/python2.7/dist-packages/ansible/parsing/vault/__init__.py", line 67, in <module>
    from cryptography.hazmat.primitives.hashes import SHA256 as c_SHA256
  File "/usr/local/lib/python2.7/dist-packages/cryptography/hazmat/primitives/hashes.py", line 15, in <module>
    from cryptography.hazmat.backends.interfaces import HashBackend
  File "/usr/local/lib/python2.7/dist-packages/cryptography/hazmat/backends/__init__.py", line 7, in <module>
    import pkg_resources
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources.py", line 2720, in <module>
    parse_requirements(__requires__), Environment()
  File "/usr/local/lib/python2.7/dist-packages/pkg_resources.py", line 592, in resolve
    raise VersionConflict(dist,req) # XXX put more info here
VersionConflict: (setuptools 3.3 (/usr/lib/python2.7/dist-packages), Requirement.parse('setuptools>=11.3'))

After:

ansible --version
ansible 2.2.0
  config file = 
  configured module search path = Default w/o overrides

A simple import of cryptography can throw several types of errors. For example, if setuptools is less than cryptography's minimum requirement of 11.3, then this import of cryptography will throw a VersionConflict here. Ansible places no restrictions on setuptools, so a valid install of ansible throws an exception because of an optional dependency. Ansible is making assumptions about the environment.

cdosborn · 2016-07-15T16:49:24Z

I believe this addresses #16686, which is incorrect in its diagnosis.

bcoca · 2016-07-15T16:58:02Z

we try to keep the exception capture narrow to avoid swallowing up unknown issues.

bcoca · 2016-07-15T16:58:46Z

lib/ansible/parsing/vault/__init__.py

@@ -68,13 +68,8 @@
    from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
    from cryptography.hazmat.backends import default_backend
    HAS_PBKDF2HMAC = True
-except ImportError:
+except:


don't use a catchall, if there are other errors we need to capture here please use specific exception captures

I was considering adding a case for VersionConflict. When would you want an optional import to raise an exception? Should a warning be printed?

yeah, if something unexpected happens, you need to know about it. If it's an optional import, then printing a warning might be the best thing in that case. So rather than a catch-all that passes you probably should add one for VersionConflict that passes, keep the previous exception handler for DistributionNotFound, and change the raise case in that one into a warning.

It looks like we don't have display available here yet, so you'll need to add an import/definition for that like this code:
https://github.com/ansible/ansible/blob/devel/lib/ansible/cli/playbook.py#L39

cdosborn · 2016-07-15T17:02:25Z

I understand that, but its an optional dependency. I may not be interested in ansible-vault and a simple run of ansible breaks. That logic of not swallowing issues is general good advice, but when it comes to optional imports, it's not the way to go. They can raise any exception which can crash the build. If you inspect the diff, there was already an edge case for another exception thrown from the import.

cdosborn · 2016-07-16T18:22:18Z

A relevant issue:
#13867

cdosborn · 2016-07-16T23:03:00Z

Here is the updated output:

$ ansible --version
 [WARNING]: Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'

Traceback (most recent call last):
  File "/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages/ansible/parsing/vault/__init__.py", line 73, in <module>
    from cryptography.hazmat.primitives.hashes import SHA256 as c_SHA256
  File "/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages/cryptography/hazmat/primitives/hashes.py", line 15, in <module>
    from cryptography.hazmat.backends.interfaces import HashBackend
  File "/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages/cryptography/hazmat/backends/__init__.py", line 7, in <module>
    import pkg_resources
  File "/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages/pkg_resources.py", line 2720, in <module>
    parse_requirements(__requires__), Environment()
  File "/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages/pkg_resources.py", line 592, in resolve
    raise VersionConflict(dist,req) # XXX put more info here
VersionConflict: (setuptools 2.2 (/Users/cdosborn/proj/py/ansible_fix/ve/lib/python2.7/site-packages), Requirement.parse('setuptools>=11.3'))
ansible 2.2.0
  config file = 
  configured module search path = Default w/o overrides

This change is pep8 compliant against make pep8.

Before:
Ansible breaks with traceback

After:
Ansible continues but warns.

Now errors are not hidden so that they can be fixed in ansible or elsewhere.

A simple import of cryptography can throw several types of errors. For example, if `setuptools` is less than cryptography's minimum requirement of 11.3, then this import of cryptography will throw a VersionConflict here. An earlier case threw a DistributionNotFound exception. An optional dependency should not stop ansible. If the error is more than an ImportError, log a warning, so that errors can be fixed in ansible or elsewhere.

abadger · 2016-07-20T03:03:03Z

Hit rebuild on the shippable job to make sure that this doesn't have any problems. @jimi-c, this is the PR that I think better fixes the cryptography with outdated setuptools behaviour, at least for now. I'd need to test more extensively but i think that ultimately the answer is going to be that pip's dep solving is mishandling this case.

cdosborn · 2016-07-20T03:52:52Z

It looks like this task failed in this job in the build-ci section:

2016-07-20 03:01:44 TASK [test_yum : install sos]
**************************************************
2016-07-20 03:01:57 fatal: [testhost]: FAILED! => {"changed": true, "failed":
true, "msg": "Yum command has been deprecated
...

This test failed on fedora rawhide but not on fedora 22. In the job for fedora 22 there was a deprecation warning but yum still ran, I think yum in rawhide exits with an error code.

abadger · 2016-07-20T10:19:49Z

Yeah, that particular error isn't a fault of your change. I think that it's a problem in that distribution's package repository. We're all at a meeting that will keep us from easily being able to fix things if they're broken otherwise I'd assume the other passing tests are good enough (I'm hitting rebuild again with the hope that the distro will have fixed its repo by now)

abadger · 2016-07-20T10:33:54Z

Everything passed. Merged to devel.

@jimi-c should I cherry-pick this for the 2.1.1rc4 branch or for 2.1.2?

A simple import of cryptography can throw several types of errors. For example, if `setuptools` is less than cryptography's minimum requirement of 11.3, then this import of cryptography will throw a VersionConflict here. An earlier case threw a DistributionNotFound exception. An optional dependency should not stop ansible. If the error is more than an ImportError, log a warning, so that errors can be fixed in ansible or elsewhere.

jhnferraris · 2016-07-21T15:59:03Z

I'm still getting this error on ansible 2.2.0 version

$ ansible --version
 [WARNING]: Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'

Traceback (most recent call last):
  File "/Users/jhnferraris/ansible/lib/ansible/parsing/vault/__init__.py", line 73, in <module>
    from cryptography.hazmat.primitives.hashes import SHA256 as c_SHA256
  File "/usr/local/lib/python2.7/site-packages/cryptography/hazmat/primitives/hashes.py", line 15, in <module>
    from cryptography.hazmat.backends.interfaces import HashBackend
  File "/usr/local/lib/python2.7/site-packages/cryptography/hazmat/backends/__init__.py", line 7, in <module>
    import pkg_resources
  File "build/bdist.macosx-10.10-x86_64/egg/pkg_resources/__init__.py", line 83, in <module>
    packaging = pkg_resources._vendor.packaging
AttributeError: 'module' object has no attribute '_vendor'
ansible 2.2.0 (devel 372018dfce) last updated 2016/07/21 23:33:29 (GMT +800)
  lib/ansible/modules/core: (detached HEAD 7de287237f) last updated 2016/07/21 23:33:55 (GMT +800)
  lib/ansible/modules/extras: (detached HEAD 68ca157f3b) last updated 2016/07/21 23:34:13 (GMT +800)
  config file =
  configured module search path = Default w/o overrides

This is was installed via source. Machine: Mac OS X El Capitan

cdosborn · 2016-07-21T18:51:14Z

@jhnferraris. This warning can be resolved by: pip install -U setuptools. The issue is that cryptography's dependency on setuptools>=11.3 is ignored by pip.

@abadger ansible could specify setuptools>=11.3 in its dependencies. However this doesn't fix the culprit. Any pkg that depends on ansible that supplies their own version of setuptools will also need to be greater than 11.3. The culprit will be fixed when pip gets dependency resolution.

jhnferraris · 2016-07-22T02:59:06Z

@cdosborn

I tried installing setuptools but warning is still coming out

$ pip install -U setuptools
You are using pip version 6.0.8, however version 8.1.2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Collecting setuptools from https://pypi.python.org/packages/dd/b8/d195d56c0a6ef8f2816a6e41a4e8ed8446bc647c6abd6e212f7190e93aff/setuptools-24.3.0-py2.py3-none-any.whl#md5=990e73b8ad68fe4349ac3a606ab48a83
  Downloading setuptools-24.3.0-py2.py3-none-any.whl (442kB)
    100% |################################| 446kB 159kB/s
Installing collected packages: setuptools
  Found existing installation: setuptools 12.0.5
    Uninstalling setuptools-12.0.5:
      Successfully uninstalled setuptools-12.0.5

Successfully installed setuptools
jhn:~ jhnferraris$ ansible-playbook --version
 [WARNING]: Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'

Traceback (most recent call last):
  File "/Users/jhnferraris/ansible/lib/ansible/parsing/vault/__init__.py", line 73, in <module>
    from cryptography.hazmat.primitives.hashes import SHA256 as c_SHA256
  File "/usr/local/lib/python2.7/site-packages/cryptography/hazmat/primitives/hashes.py", line 15, in <module>
    from cryptography.hazmat.backends.interfaces import HashBackend
  File "/usr/local/lib/python2.7/site-packages/cryptography/hazmat/backends/__init__.py", line 7, in <module>
    import pkg_resources
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2928, in <module>
    @_call_aside
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2914, in _call_aside
    f(*args, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2941, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 635, in _build_master
    ws.require(__requires__)
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 943, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 829, in resolve
    raise DistributionNotFound(req, requirers)
DistributionNotFound: The 'pycrypto>=2.6' distribution was not found and is required by ansible
ansible-playbook 2.2.0 (devel 372018dfce) last updated 2016/07/21 23:33:29 (GMT +800)
  lib/ansible/modules/core: (detached HEAD 7de287237f) last updated 2016/07/21 23:33:55 (GMT +800)
  lib/ansible/modules/extras: (detached HEAD 68ca157f3b) last updated 2016/07/21 23:34:13 (GMT +800)
  config file =
  configured module search path = Default w/o overrides

abadger · 2016-07-22T11:33:00Z

Pip bug about its lack of dependency solving: pypa/pip#988

abadger · 2016-07-22T12:20:14Z

@jhnferraris If you take a look at the traceback, you'll see that the problem in your case is now that pycrypto isn't installed rather than cryptography: https://pypi.python.org/pypi/pycrypto

A simple import of cryptography can throw several types of errors. For example, if `setuptools` is less than cryptography's minimum requirement of 11.3, then this import of cryptography will throw a VersionConflict here. An earlier case threw a DistributionNotFound exception. An optional dependency should not stop ansible. If the error is more than an ImportError, log a warning, so that errors can be fixed in ansible or elsewhere.

bcoca added bugfix_pull_request needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 15, 2016

bcoca reviewed Jul 15, 2016
View reviewed changes

cdosborn mentioned this pull request Jul 16, 2016

Remove install_requires dependency on setuptools. #11405

Closed

Christoph-D mentioned this pull request Jul 18, 2016

Integration test on Shippable runs into timeout #16745

Closed

abadger merged commit b06c61c into ansible:devel Jul 20, 2016

abadger removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 20, 2016

abadger assigned jimi-c Jul 20, 2016

abadger added this to the stable-2.1 milestone Jul 20, 2016

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 5, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix exceptions thrown from cryptography import #16723

Fix exceptions thrown from cryptography import #16723

cdosborn commented Jul 15, 2016

cdosborn commented Jul 15, 2016

bcoca commented Jul 15, 2016

bcoca Jul 15, 2016

cdosborn Jul 15, 2016

abadger Jul 16, 2016

cdosborn commented Jul 15, 2016 •

edited

cdosborn commented Jul 16, 2016

cdosborn commented Jul 16, 2016

abadger commented Jul 20, 2016

cdosborn commented Jul 20, 2016 •

edited

abadger commented Jul 20, 2016

abadger commented Jul 20, 2016

jhnferraris commented Jul 21, 2016 •

edited

cdosborn commented Jul 21, 2016

jhnferraris commented Jul 22, 2016

abadger commented Jul 22, 2016

abadger commented Jul 22, 2016 •

edited

Fix exceptions thrown from cryptography import #16723

Fix exceptions thrown from cryptography import #16723

Conversation

cdosborn commented Jul 15, 2016

ISSUE TYPE

ANSIBLE VERSION

SUMMARY

cdosborn commented Jul 15, 2016

bcoca commented Jul 15, 2016

bcoca Jul 15, 2016

Choose a reason for hiding this comment

cdosborn Jul 15, 2016

Choose a reason for hiding this comment

abadger Jul 16, 2016

Choose a reason for hiding this comment

cdosborn commented Jul 15, 2016 • edited

cdosborn commented Jul 16, 2016

cdosborn commented Jul 16, 2016

abadger commented Jul 20, 2016

cdosborn commented Jul 20, 2016 • edited

abadger commented Jul 20, 2016

abadger commented Jul 20, 2016

jhnferraris commented Jul 21, 2016 • edited

cdosborn commented Jul 21, 2016

jhnferraris commented Jul 22, 2016

abadger commented Jul 22, 2016

abadger commented Jul 22, 2016 • edited

cdosborn commented Jul 15, 2016 •

edited

cdosborn commented Jul 20, 2016 •

edited

jhnferraris commented Jul 21, 2016 •

edited

abadger commented Jul 22, 2016 •

edited