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

Optional dependency 'cryptography' raised an exception #17982

Closed
renard opened this issue Oct 12, 2016 · 17 comments · Fixed by #17984
Closed

Optional dependency 'cryptography' raised an exception #17982

renard opened this issue Oct 12, 2016 · 17 comments · Fixed by #17984
Assignees
Labels
affects_2.2 This issue/PR affects Ansible v2.2 bug This issue/PR relates to a bug.

Comments

@renard
Copy link

renard commented Oct 12, 2016

ISSUE TYPE
  • Bug Report
ANSIBLE VERSION
 [WARNING]: Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'

ansible-playbook 2.2.0.0 (stable-2.2 3a822faeae) last updated 2016/10/10 13:52:58 (GMT +200)
  lib/ansible/modules/core: (detached HEAD cf0c652a88) last updated 2016/10/10 13:59:31 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 6979330c1d) last updated 2016/10/10 13:59:31 (GMT +200)
  config file = /Users/renard/Src/ansible-dc/ansible.cfg
  configured module search path = Default w/o overrides
CONFIGURATION

None specific

OS / ENVIRONMENT

MacOS

SUMMARY

The issue was generated because I do not have paramiko installed.

I use that diff to find out the exact problem:

diff --git a/lib/ansible/parsing/vault/__init__.py b/lib/ansible/parsing/vault/__init__.py
index 9cc0b5e..6c2b6db 100644
--- a/lib/ansible/parsing/vault/__init__.py
+++ b/lib/ansible/parsing/vault/__init__.py
@@ -83,7 +83,7 @@ try:
 except ImportError:
     pass
 except Exception as e:
-    display.warning("Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'")
+    display.warning("Optional dependency 'cryptography' raised an exception, falling back to 'Crypto' %s" % e)
     import traceback
     display.debug("Traceback from import of cryptography was {0}".format(traceback.format_exc()))

The I got this message:

[WARNING]: Optional dependency 'cryptography' raised an exception, falling back to 'Crypto' The 'paramiko' distribution was not found and is required by ansible

STEPS TO REPRODUCE

Simply uninstall the paramiko module.

EXPECTED RESULTS

Is paramiko really mandatory? for the moment I use ansiblev2 without paramiko and without any issue. I do not use the vault thought.

Maybe you should add the exception message to help the user.

Hope that help.

@ansibot
Copy link
Contributor

ansibot commented Oct 12, 2016

@renard, we need more information to resolve this issue. For that reason, we put this Issue into the 'needs_info' state.

Here are the required items we could not find in your description:

  • issue type

When you have filled in the missing data, we will notify the module maintainer for further action.
click here for bot help

@ansibot ansibot added needs_info This issue requires further information. Please answer any outstanding questions. affects_2.2 This issue/PR affects Ansible v2.2 labels Oct 12, 2016
@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

Hmmm..... that's complicated... The short answer is yes.

The longer answer is that it appears to only be required because we have it in setup.py as an install_requires. It is actually not required by the code unless you use the paramiko connection type.

I'll ask what the other committers want to do about it... There used to be many platforms where paramiko significantly outperformed shelling out to the ssh client to do the job so it made a ton of sense. Now there's only a handful of platforms where that's the case. keeping it in install_requires is the only way to make sure that pip installs paramiko when it installs ansible but if someone knows what they're doing, they can leave it off their system so it would be nice not to make this a runtime dependency...

@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

!needs_info

@renard
Copy link
Author

renard commented Oct 12, 2016

I didn't use pip to install ansible, but a git clone. Again I don't care about installing paramiko even if I do not need it. It was just the warning message that was confusing since cryptography was installed.

@ansibot ansibot added bug_report and removed needs_info This issue requires further information. Please answer any outstanding questions. labels Oct 12, 2016
@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

bcoca's suggestion: if we can confirm the 'paramiko is best' platforms ... which i believe are OS X due to sshpass and systems with very old openssh which does not do controlpersist (centos5) then make the install_requires include paramiko only when installed on those platforms.

@bcoca
Copy link
Member

bcoca commented Oct 12, 2016

We'll also have to update paramiko to give 'nice errors' and suggest installing requirements if someone specifies it directly (not via 'smart' setting).

Which will probably happen when the 'target' machines have poor controlpersist support as we can only really detect issues with the 'controller' on which Ansible is installed.

@renard
Copy link
Author

renard commented Oct 12, 2016

again the problem is not about installing paramiko or not.

The problem here is a warning suggesting cryptography is not (or not correctly) installed.

@bcoca
Copy link
Member

bcoca commented Oct 12, 2016

@renard, sorry, we did hijack the thread, I for one am OK with changing this warning into a -vvvv or debug 'information', as long as the system will work and the user should not see any difference.

@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

@renard The warning is just that, a warning. We could display more information about the exception (perhaps move the traceback from ansible.debug to a verbosity level so that -vvv shows it?) if that helps. For any library using setuptools' dependency management (unfortunately, cryptography and ansible both fall under that) there is the danger that localized problems in the overall dependency chain can affect unrelated packages in the overall chain. So a lot of times even the traceback might not be too helpful in diagnosing the problem.

@sivel
Copy link
Member

sivel commented Oct 12, 2016

I think part of the problem is that we are displaying a warning and a debug for the same thing. So you don't get full information immediately, or instruction on how to get more info:

except Exception as e:
    display.warning("Optional dependency 'cryptography' raised an exception, falling back to 'Crypto'")
    import traceback
    display.debug("Traceback from import of cryptography was {0}".format(traceback.format_exc()))

Maybe we should change that debug to something else, such as vvvv so that it is printed with higher verbosity, and indicating in the warning that running with -vvvv will provide more information.

@renard
Copy link
Author

renard commented Oct 12, 2016

ok do as you think it's the best solution. But please add a note somewhere that for user that used git clone to install ansible, the lack of paramiko can lead to this warning.
I am not confortable with "A warning is just a warning" since you expect your playbooks to to run properly.

If you change the warning to something like: "Optional dependency 'cryptography' raised an exception, falling back to 'Crypto' (If 'cryptography' is installed maybe 'paramiko' is not, install 'paramiko' to remove this warning)" many users would understand that warning and take adequate measure.

abadger added a commit to abadger/ansible that referenced this issue Oct 12, 2016
@renard
Copy link
Author

renard commented Oct 12, 2016

@sivel here is a -vvvv run output:

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

ansible-playbook 2.2.0.0 (stable-2.2 3a822faeae) last updated 2016/10/10 13:52:58 (GMT +200)
  lib/ansible/modules/core: (detached HEAD cf0c652a88) last updated 2016/10/10 13:59:31 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 6979330c1d) last updated 2016/10/10 13:59:31 (GMT +200)
  config file = /Users/renard/Src/ansible-dc/ansible.cfg
  configured module search path = Default w/o overrides

and my configuration file:

[defaults]
hostfile = inventory
ansible_managed = This file is managed by ansible. Any change would be overwritten.

[ssh_connection]
pipelining=True
control_path = %(directory)s/%%h-%%p

That's why I was a bit confused at the beginning since no traceback was displayed.

@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

Yeah -- it only displays if you use debugging currently (ANSIBLE_DEBUG=1 ansible [...]) We've been talking about whether to unify verbosity and debugging under one umbrella for a while. That's a much bigger issue though.

The PR I've submitted above will display both the warning and the traceback with verbosity 4. Otherwise it will be silent. In this case, the APIs in cryptography vs Crypto have similar security so that's not really a problem. The Cryptography using code is faster but that doesn't affect most people (and definitely doesn't affect people who aren't using vault).

We can't bake in instructions for how to fix this because there are many reasons that importing cryptography could throw an exception and all of them ultimately go down this path.

@abadger
Copy link
Contributor

abadger commented Oct 12, 2016

@renard PR merged. You shouldn't see the warning with low verbosity levels. With high verbosity levels you should see both the warning and the traceback that can lead you to a diagnosis.

@renard
Copy link
Author

renard commented Oct 12, 2016

@abadger thanks but I don't see the backtrace even if I run it with -vvvv:

(ansible-dc) renard@scrat:~/Src/ansible-dc $ ansible-playbook   --version
ansible-playbook 2.2.0.0 (stable-2.2 4cd32ee1ac) last updated 2016/10/12 21:20:06 (GMT +200)
  lib/ansible/modules/core: (detached HEAD cf0c652a88) last updated 2016/10/12 21:23:05 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 6979330c1d) last updated 2016/10/12 21:23:05 (GMT +200)
  config file = /Users/renard/Src/ansible-dc/ansible.cfg
  configured module search path = Default w/o overrides
(ansible-dc) renard@scrat:~/Src/ansible-dc $ ansible-playbook -vvvv   --version
ansible-playbook 2.2.0.0 (stable-2.2 4cd32ee1ac) last updated 2016/10/12 21:20:06 (GMT +200)
  lib/ansible/modules/core: (detached HEAD cf0c652a88) last updated 2016/10/12 21:23:05 (GMT +200)
  lib/ansible/modules/extras: (detached HEAD 6979330c1d) last updated 2016/10/12 21:23:05 (GMT +200)
  config file = /Users/renard/Src/ansible-dc/ansible.cfg
  configured module search path = Default w/o overrides
(ansible-dc) renard@scrat:~/Src/ansible-dc $

@abadger abadger reopened this Oct 18, 2016
@abadger
Copy link
Contributor

abadger commented Oct 19, 2016

Traced this to the fact that the module is imported before the command line is parsed. Thus we don't have a verbosity level yet. So the logic in vvvv() decides it shouldn't display anything.

It doesn't look like we can parse the command line for verbosity before we get to the point which is importing the vault code. So I think we may only have a choice of on or off here.. verbosity won't work.

@abadger abadger closed this as completed Oct 19, 2016
@renard
Copy link
Author

renard commented Oct 19, 2016

@abadger ok fair enough.
Thanks for the quick fix.

sereinity pushed a commit to sereinity-forks/ansible that referenced this issue Jan 25, 2017
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@ansible ansible locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.2 This issue/PR affects Ansible v2.2 bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants