-
Notifications
You must be signed in to change notification settings - Fork 693
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
Update ansible to 2.9.7 #5199
Update ansible to 2.9.7 #5199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rmol, tested as follows:
dev/staging environments
- install dev-requirements
- make build-debs
- make staging
- make upgrade-start
- make upgrade-test-local
Prod environments
- ./securedrop-admin setup
- ./securedrop-admin install
- ❗ ./securedrop-admin install: Subsequent install fails, a
|changed
was not caught ininstall_files/ansible-base/tasks/transistion_ssh_local.yml
on lines 27 and 35. Modifying to.changed
like you do in other tasks resolved (see error output in 1 below) - ./securedrop-admin logs
- ./securedrop-admin backup
- ./securedrop-admin restore
- ./securedrop-admin tailsconfig
A couple of observations, I think we should address 1 and 2 prior to merge, what do you think?:
- When running a second install on the server, the install fails with the following message:
TASK [Force a reboot conditionally, when tor_over_ssh status changed] **************************************************************************************************
fatal: [app]: FAILED! => {"msg": "The conditional check 'aths_deletion_results|changed' failed. The error was: template error while templating string: no filter named 'changed'. String: {% if aths_deletion_results|changed %} True {% else %} False {% endif %}\n\nThe error appears to be in '/home/amnesia/Persistent/securedrop/install_files/ansible-base/tasks/transistion_ssh_local.yml': line 23, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: Force a reboot conditionally, when tor_over_ssh status changed\n ^ here\n"}
fatal: [mon]: FAILED! => {"msg": "The conditional check 'aths_deletion_results|changed' failed. The error was: template error while templating string: no filter named 'changed'. String: {% if aths_deletion_results|changed %} True {% else %} False {% endif %}\n\nThe error appears to be in '/home/amnesia/Persistent/securedrop/install_files/ansible-base/tasks/transistion_ssh_local.yml': line 23, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- name: Force a reboot conditionally, when tor_over_ssh status changed\n ^ here\n"}
- It appears like 2.9.7 was released as this update was being developed/tested, and there are CVEs associated . It's the
win_unzip
module which we don't use, but I'm guessing GitHub will soon warn us about the vulnerability [1]. I think it might make sense to update to 2.9.7 to avoid having to do it again in the future. - I am observing deprecation warnings during build but also install phase (example is from builder):
[DEPRECATION WARNING]: Distribution Ubuntu 16.04 on host xenial-sd-generic-
ossec-agent2 should use /usr/bin/python3, but is using /usr/bin/python for
backward compatibility with prior Ansible releases. A future Ansible release
will default to using the discovered platform python for this host. See https:/
/docs.ansible.com/ansible/2.9/reference_appendices/interpreter_discovery.html
for more information. This feature will be removed in version 2.12. Deprecation
warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
Based on the link in that message, we don't set the interpreter to Python3. My understanding is that it will use /usr/bin/python
which is 2.7.12. it is getting upstream security updates, but might be worth tracking as we eventually target other platforms. Perhaps we can track this is a follow-up?
Agreed on all points. I'm addressing 1 and 2 right now, and think we might as well solve 3 by setting |
Apparently Ansible used to inexact matching when looking for handlers. That changed in the 2.8 series. See: ansible/ansible#55575 That broke the fuzzy name matching relied upon in the restart-tor-naming handler, causing Tor not to be restarted properly. We could also specify a consistent topic to monitor with "listen", but I think this restart logic can be simplified to one task anyway.
Change deprecated "ansible_ssh_{host,user,port}" variables to just "ansible_{host,user,port}".
Use "interpreter_python=auto" to explicitly request the new discovery logic and eliminate the deprecation warnings. See https://docs.ansible.com/ansible/2.9/reference_appendices/interpreter_discovery.html
Sounds great, the commit you've added doing that looks like just what we need. Given that @emkll has already verified py3 support in prod env, will take a closer look at the upgrade & qubes-staging scenarios, make sure we're covered there. |
To be clear, verified that python3 was installed on a fresh Ubuntu 16.04.6 target as follows:
|
OK, I'm still having trouble with Molecule. When I try I've tried adding I converted the I cannot get this playbook to actually fire the handler in Ansible 2.8.11 or 2.9.7. Any ideas would be incredibly welcome. |
The transition to python3 by default for the Ansible interpreter means we'll need the python module dependencies to by py3, not py2.
The Ansible 2.8 porting guide [0] states that handlers cannot use "include", they should instead use "include_tasks". Done. In the "upgrade" scenario, however, the old version of the Ansible configs are cloned in via git, and those will still not be compatible with Ansible 2.9, so let's patch the handler file in place as a shim to provide mutual compatibility with the old & new Ansible versions. The patch task should be conditional on a semvar comparison, rather than a substring match, but in practice we only ever use the upgrade scenario to test adjacent versions anyway, so didn't seem worth the trouble. This commit also renames a handle for uniqueness. That wasn't strictly required for compatibility, but it makes it easier to discover run order when reading or grepping the config. [0] https://docs.ansible.com/ansible/latest/porting_guides/porting_guide_2.8.html#imports-as-handlers
Appended two commits. The first fixes the OSSEC package build by installing The second fixes the "upgrade" scenario.
The problem was that the "upgrade" scenario uses the old logic from the previous stable release of SecureDrop, cloning it into a separate subdir, which is then used to provision the servers. Since we jumped two minor versions of Ansible, we lost mutual compatibility between the two, so even though the handler task was happy as of 2.9, the combination of 1) a separate subdir with relative lookups for handlers and 2) the old-style "include" running under Ansible 2.9 was leading to the repeated failure to find the handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@conorsch Works great; the upgrade scenario is now fine. I'm just working through the rest as a final check, but I think you've done it. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rmol and @conorsch, changes look good to merge when CI passes, from my perspective. Approving this now since my review is blocking, @rmol feel free to merge whenever you've finished your review of the changes.
- diff review and hash match
- changes to ansible version check look good
dev/staging environments
- install dev-requirements
- make build-debs
- make staging
- make upgrade-start
- make upgrade-test-local
Prod environments
- ./securedrop-admin setup
- ./securedrop-admin install
- ./securedrop-admin install: Subsequent install succeeds, also changed ssh_over_tor
- ./securedrop-admin logs
- ./securedrop-admin backup
- ./securedrop-admin restore
- ./securedrop-admin tailsconfig
I've worked through the rest of the tests @emkll listed, and everything looks good. Merging. |
Status
Ready for review
Description of Changes
Updates Ansible.
Fixes #5144.
Testing
Ideally,
make clean
followed bymake venv && make build-debs && make staging
, thenmolecule verify -s libvirt-staging-xenial
(or whatever your staging scenario is).Even more ideally, run through a production install on VMs or hardware, using this branch.
Deployment
It will affect admin workstations and development environments, but should not affect the content of deployed instances.
Checklist
If you made changes to the server application code:
make lint
) and tests (make test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you added or updated a code dependency:
Choose one of the following:
I've done a timeboxed and therefore limited review of the differences between Ansible 2.7.13 and 2.9.7 (hashes of the tarballs I examined are in the packaging wiki) and did not spot any problems in changes to the core functionality. I also ran Bandit checks for high-severity problems on both versions, and found no new problems reported in 2.9.7.