-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add PyCryptodome to requirements_ansible.*, related #3180 #3305
Conversation
Fixes issue #3180
Build failed.
|
Build succeeded.
|
I may take a stab at updating some of the requirements in a controlled fashion in a separate PR. I tried to follow the instructions in |
We probably need to see if this requirement is needed for: #3266 This PR obviates any other inventory script related changes we'd make here. |
What part of the script needs this? I don't run into this during normal usage. |
@matburt
However, the open PR doesn't help me at that moment, whereas including this dependency does. ;) Are you testing the gce.py inventory script in Kubernetes/GKE?
The GCE dynamic inventory script requires apache-libcloud and credentials from a GCP service account. libcloud in turn, requires PyCrypto (or preferably, the newer PyCryptodome) to use a GCP service account's credentials. I've tested this bug in both 3.0.1 and the latest commits in the devel branch. |
Took me a while, but I got around to double-checking your work. I can confirm that I reproduced the linked issue report, and that adding this dependency resolves the issue. I only reproduced locally, running What I did:
Double checking plugin:
I do see the gap in testing. I know for sure that we did:
To my knowledge, we never got around to testing the combination these new features - running inventory updates in python3, ping @kdelee @ryanpetrello. My interest for inventory updates with custom virtual environments was to test inventory plugins. I think python3 is important to cross-test with, but it's not a priority for me until the inventory plugins are in. We might also want regression testing (which is easily viable), before merging this. Also, I'm worried about dependency conflicts in more than just gce, so that needs testing too. As for this PR, it's under consideration, but there are a lot of questions, so I hope you understand that we will delay for a while. Here's the background I have on this issue: See particular comment
This is an argument for the solution you took here. I understand, it is well-stated. However, here is what the Ansible core team did in response to that issue: https://github.com/ansible/ansible/pull/21430/files
To double check, I did:
Picking one library versus another is a high-consequence decision. I will not be the one to make it. If it is decided that we use this one, I'll be happy to merge this change. If not, we still appreciate the help. Thanks! EDIT: missed a step |
@skinlayers from the issue, it looks like you were running in the default ansible virtualenv. Is that right? I can't square that with my local testing, and when I test gce sources in AWX, they run successfully. |
If I run this in gce updates in a python3 virtualenv, then I do hit the error in the linked issue. I just need to be clear about what we're dealing with here. |
@AlanCoding if this is a matter of only installing certain packages ( awx/requirements/requirements_ansible.txt Line 53 in d2fa5cc
What if we did something like:
...in |
That has a lot of potential fallout, and it doesn't match the facts of the reported issues. Some of these issues mention python3, but what does that mean? Was the default venv hacked to use python3 and not python2? That's going to be a problem since we're trying to use system site packages in the default venv. Depending on what is actually going on with that issue, it's going to be a completely different conversation. Using a python3 default venv will take work dedicated specifically to that goal, I think you might be doing that. The inventory plugins, at least, will need to run in python3. Best thing we can do might be to start active work on testing that soon. |
The default Ansible venv uses the default system python. On certain OSes, that is python3, on others, that is python2. It would use |
My setup: Using existing GKE cluster.
my-inventory-devel modifications:
ingress-with-cert-manager.yaml:
|
@skinlayers I've actually opened a PR to libcloud to replace PyCrypto usage with a more modern dependency awx already uses (https://pypi.org/project/cryptography/): If it merges and is released, #3180 will be resolved, so please feel to chime in with your support there. |
FYI libcloud now no longer uses PyCrypto (apache/libcloud#1280); once a new release lands on PyPI, we'll update AWX's dependencies. |
related #3180
SUMMARY
Adds PyCryptodome to python requirements. Fixes gce.py dynamic inventory failure.
related #3180
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
Before:
After: