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

Moves securedrop-admin into Python 3 #4867

Merged
merged 3 commits into from Oct 7, 2019

Conversation

@kushaldas
Copy link
Contributor

kushaldas commented Sep 25, 2019

Status

Work in progress

Description of Changes

Fixes #3489

This now needs an update of prompt_toolkit so that we can use it in Python3.5.

Testing

In both Tails3, and Tails4, execute the following scenarios with prod vms/hardware:

  • ./securedrop-admin sdconfig
  • ./securedrop-admin install
  • ./securedrop-admin tailsconfig
  • ./securedrop-admin backup
  • ./securedrop-admin restore
  • ./securedrop-admin update
  • ./securedrop-admin logs
  • ./securedrop-admin reset_admin_access

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally
@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Sep 26, 2019

rebase issue?

@kushaldas kushaldas force-pushed the admins_love_python3 branch from 459f7f6 to 3a15c7a Sep 26, 2019
@kushaldas

This comment has been minimized.

Copy link
Contributor Author

kushaldas commented Sep 26, 2019

There can be two different ways to migrate the virtualenv for the admins.

  • After checking out 1.1.0, if the admins run ./securedrop-admin install, it will fail and ask to rerun securedrop-admin setup first. This is the (current implementation) and seems to be simpler. We can decide what kind of message to tell the admins about why should they run the setup command again.

  • The other option is when ./securedrop-admin install will see that there is a Python2 venv exists, and it will automatically do the setup work for Python3 venv.

I am going to test the first item in a vm now.

@kushaldas kushaldas force-pushed the admins_love_python3 branch 2 times, most recently from c2f930e to 3a15f0e Sep 26, 2019
@kushaldas kushaldas marked this pull request as ready for review Sep 26, 2019
@kushaldas

This comment has been minimized.

Copy link
Contributor Author

kushaldas commented Sep 26, 2019

_./securedrop-admin install fails with following:


TASK [validate : Confirm host OS is Tails.] ************************************
fatal: [localhost]: FAILED! => {"msg": "The conditional check 'ansible_lsb.major_release >= 9' failed. The error was: Unexpected templating type error occurred on ({% if ansible_lsb.major_release >= 9 %} True {% else %} False {% endif %}): unorderable types: AnsibleUnsafeText() >= int()"}

NO MORE HOSTS LEFT *************************************************************

NO MORE HOSTS LEFT *************************************************************
	to retry, use: --limit @/home/amnesia/Persistent/securedrop/install_files/ansible-base/securedrop-prod.retry

PLAY RECAP *********************************************************************
localhost                  : ok=2    changed=0    unreachable=0    failed=1  

Probable reason: Old version of Ansible using Python3 in the controller (Tails)

@kushaldas kushaldas requested a review from conorsch as a code owner Sep 26, 2019
@conorsch

This comment has been minimized.

Copy link
Contributor

conorsch commented Sep 26, 2019

@kushaldas I've appended a commit that resolves a few of the Ansible-related Python 3 issues. Tried most (but not all) of the test scenarios you outlined, and they've all been successful with the changes in place in Tails. For the record, I tested on Tails 3.16, with Python 3.5. Noticed some "weakref" stderr output in a few places, which is a significant cosmetic issue, but should not be a factor on Tails 4 / Python 3.7.

Not approving, but I believe you should be unblocked for more vigorous testing now. Take another lap through it and see if you can identify any lingering Python 2 references that we haven't caught yet. When you're happy with it, I'd suggest we have @rmol and/or @zenmonkeykstop dig in for comprehensive review.

@kushaldas

This comment has been minimized.

Copy link
Contributor Author

kushaldas commented Sep 27, 2019

@rmol @zenmonkeykstop please go ahead with full scale review. This worked in my local testing.

@eloquence eloquence added this to Ready for review in SecureDrop Team Board Sep 27, 2019
@conorsch

This comment has been minimized.

Copy link
Contributor

conorsch commented Sep 27, 2019

#4865 was just merged, introducing conflicts here. We choose to merge #4865 first to minimize the manual conflict resolution. @rmol / @zenmonkeykstop please give a rebase here, holler in gitter with any blockers!

@zenmonkeykstop

This comment has been minimized.

Copy link
Contributor

zenmonkeykstop commented Sep 27, 2019

On Tails 4.0~beta2 VM, checked out this branch and verified:

  • ./securedrop-admin setup
  • ./securedrop-admin sdconfig
  • ./securedrop-admin install _(all good except for unmerged fixes in #4872)
  • ./securedrop-admin tailsconfig
  • ./securedrop-admin backup
  • ./securedrop-admin restore
  • ./securedrop-admin update
  • ./securedrop-admin logs
  • ./securedrop-admin reset_admin_access
@rmol

This comment has been minimized.

Copy link
Contributor

rmol commented Sep 27, 2019

Tested on Tails 3.16 and prod VMs:

  • ./securedrop-admin sdconfig
  • ./securedrop-admin install
  • ./securedrop-admin tailsconfig
  • ./securedrop-admin backup
  • ./securedrop-admin restore
  • ./securedrop-admin update
  • ./securedrop-admin logs
  • ./securedrop-admin reset_admin_access
@rmol rmol moved this from Ready for review to Under Review in SecureDrop Team Board Sep 27, 2019
kushaldas and others added 3 commits Sep 26, 2019
This will inform the admins to rerun ./securedrop-admin setup
as this will create .venv3 dirctory for Python3 dependencies.
As Tor4 has Python 3.7.3, we should just mention without any
particular version.
A smattering of the familiar Python 2 -> 3 changes, mostly around string
handling, filepaths to interpreters and virtualenvs, and syntax updates.
Specifically:

  * coerces major release number to int
  * converts iteritems() -> items() in dynamic inventory
  * updates string handling in dynamic inventory
  * ensures securedrop_init runs under python3

Tested under Tails 3.16 (based on Stretch), running Python 3.5. Worked
well, after these fixes landed, although I've observed a handful of the
"weakref" stderr output. The weakref output doesn't break anything, but
it may be alarming to Admins who haven't updated to Tails 4 & Python 3.7
yet.
@conorsch conorsch force-pushed the admins_love_python3 branch from 6266b56 to bc898e3 Sep 27, 2019
@conorsch

This comment has been minimized.

Copy link
Contributor

conorsch commented Sep 27, 2019

Rebased to resolve conflicts introduced via merge of #4865. The sole conflict was:

diff --cc admin/requirements.in
index 3e7fda7e0,2eebee385..000000000
--- a/admin/requirements.in
+++ b/admin/requirements.in
@@@ -1,2 -1,2 +1,7 @@@
++<<<<<<< HEAD
 +prompt_toolkit
 +pyyaml>=5.1.2
++=======
+ prompt_toolkit==2.0.9
+ pyyaml
++>>>>>>> 3a15f0e13... Moves securedrop-admin to Python3

which was manually solved as:

$ cat admin/requirements.in
prompt_toolkit==2.0.9
pyyaml>=5.1.2
@zenmonkeykstop

This comment has been minimized.

Copy link
Contributor

zenmonkeykstop commented Sep 27, 2019

On Tails 3.15 VM, checked out the rebased branch and verified:

  • ./securedrop-admin setup
  • ./securedrop-admin sdconfig
  • ./securedrop-admin install
  • ./securedrop-admin tailsconfig
  • ./securedrop-admin backup
  • ./securedrop-admin restore
  • ./securedrop-admin update
  • ./securedrop-admin logs
  • ./securedrop-admin reset_admin_access

LGTM!

Copy link
Contributor

zenmonkeykstop left a comment

LGTM based on test plan.

@zenmonkeykstop zenmonkeykstop merged commit 6cf415e into develop Oct 7, 2019
9 checks passed
9 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: app-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: static-analysis-and-no-known-cves Your tests passed on CircleCI!
Details
ci/circleci: translation-tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details
SecureDrop Team Board automation moved this from Under Review to Done Oct 7, 2019
@emkll

This comment has been minimized.

Copy link
Contributor

emkll commented Oct 7, 2019

While testing #4877 ran into an issue that may be worth flagging: we should ensure the 1.0.0 -> 1.1.0 docs have admins run ./securedrop-admin setup as part of the upgrade process to 1.1.0, as the SecureDrop GUI updater will not run if the checked out branch is python3 . This will affect the version after SecureDrop 1.1.0, if a user manually checks out a 1.1.0 + branch and does not run ./securedrop-admin setup

@emkll emkll deleted the admins_love_python3 branch Oct 7, 2019
--hash=sha256:1df952620eccb399c53ebb359cc7d9a8d3a9538cb34c5a1344bdbeb29fbcc381 \
--hash=sha256:3f473ae040ddaa52b52f97f6b4a493cfa9f5920c255a12dc56a7d34397a398a4 \
--hash=sha256:858588f1983ca497f1cf4ffde01d978a3ea02b01c8a26a8bbc5cd2e66d816917
prompt_toolkit==2.0.9 \

This comment has been minimized.

Copy link
@redshiftzero

redshiftzero Oct 8, 2019

Member

was diff review performed here?

This comment has been minimized.

Copy link
@redshiftzero
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.