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

Updated testinfra tests to optionally run against a prod instance #5318

Merged
merged 10 commits into from
Sep 22, 2020

Conversation

zenmonkeykstop
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop commented Jun 16, 2020

Status

Ready for Review

Description of Changes

Fixes #4216

Updates testinfra tests to allow them to be run against a production SecureDrop hardware instance from an Admin Workstation.

Testing

Verify staging unchanged:

  • check out this branch and run:
    make build-debs && make staging && make testinfra
    
    • testinfra tests complete without error

test against prod hardware (assuming default IPs, DNS settings, and both v2 and v3 services enabled):

  • on the admin workstation for a previously configured instance, cd ~/Persistent/securedrop and check out this branch

  • run ./securedrop-admin setup -t to install testinfra dependencies

  • run ./securedrop-admin verify

    • testinfra tests complete with no errors
  • remove the test dependencies with rm -rf ~/Persistent/securedrop/admin/.venv3

  • run ./securedrop-admin setup to install vanilla dependencies

  • verify testinfra module not installed with:

    cd ~/Persistent/securedrop
    source admin/.venv3/bin/activate
    pip list | grep testinfra  # there should be no output
    

Deployment

No special requirements for deployment

Checklist

because securedrop-admin changes:

  • make -C admin test passing locally.

If you made non-trivial code changes:

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

@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch from 99de0fa to eb703f0 Compare June 16, 2020 23:27
@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch 2 times, most recently from 44829c8 to 2988455 Compare June 18, 2020 23:15
@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch 4 times, most recently from ba5460f to 411f743 Compare July 8, 2020 15:26
@zenmonkeykstop zenmonkeykstop marked this pull request as ready for review July 8, 2020 19:10
@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch 2 times, most recently from 4316b22 to fcf3f49 Compare July 8, 2020 22:05
@kushaldas
Copy link
Contributor

Note: TEST_ENV='prodVM' ./devops/scripts/run_prod_testinfra this step requires a lot of system package dependencies. I am slowly and painfully installing them. maybe we can add them or via the setup step.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • testinfra tests complete without error

This works, I could not finish up the prod testing thanks to tails.

@zenmonkeykstop
Copy link
Contributor Author

I'm not 100% into adding the test dependencies in the regular setup step, as they're not required for securedrop-admin, so they're going to make that step longer for users for a start. But yeah it's a bit of a pain. At least it's faster on repeated runs (assuming the virtualenv wasn't nuked in between).

What problem did you hit with Tails and prod?

@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch from fcf3f49 to a08a96a Compare July 14, 2020 21:27
@zenmonkeykstop zenmonkeykstop marked this pull request as draft July 21, 2020 16:34
@zenmonkeykstop
Copy link
Contributor Author

flipped back to draft to allow for some updates

@zenmonkeykstop zenmonkeykstop force-pushed the testinfra_neutral_env branch 2 times, most recently from 6da5b82 to eea01e0 Compare July 31, 2020 18:16
@zenmonkeykstop zenmonkeykstop marked this pull request as ready for review September 17, 2020 15:32
@zenmonkeykstop
Copy link
Contributor Author

Have addressed @kushaldas's point above by adding a new requirements file including testinfra deps that can optionally be installed at setup (using the -t flag), and making the call out to the testinfra script a securedrop-admin command for good measure. Also working around some remaining issues with iptables tests by suppressing the test in prod - not ideal but prod instances do have the daily check recently added by @rmol. This should be ready for review.

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great! Confirmed staging is unaffected (CI appears happy, as well). For testing the Tails behavior, I didn't use prod HW, rather prod VMs. The IP addresses for the vagrant environment were different from the HW defaults, and those failures were reported in the testinfra run, as expected. Everything else passed.

Also confirmed that the virtualenv only installs testinfra if the -t flag is passed to the setup subcommand. Only question I have is given that

Also working around some remaining issues with iptables tests by suppressing the test in prod

there might be a bit of leftover prod iptables info in the diff, that's now unused. Even if that's the case, I wouldn't be opposed to merging, if it's partial progress toward full coverage.

@conorsch
Copy link
Contributor

@kushaldas It'd be great to have your approval on here too, since you performed active review previously!

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through all the test steps (used prod vms), this looks good to me. 💯

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zenmonkeykstop for your hard work on these changes, this is very exciting and these changes will be of great help with the upcoming release testing.

Took one last pass through the test plan, all tests are passing with the exception of (3) tests for IP addresses related to OSSEC (using non-standard IP addresses, so this is expected, we can always follow-up later).

#
# pip-compile --allow-unsafe --generate-hashes --output-file=requirements-testinfra.txt requirements-ansible.in requirements-testinfra.in requirements.in
#
ansible==2.9.7 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting for posterity that the versions of cffi, cryptography, jinja and others are different from the ones locked in develop-requirements.txt and admin/requirements.txt. Given that the changes here work, and that this is exclusively used for testing, we can always update the other requirements files at a later date.

@emkll emkll merged commit 5bd908f into freedomofpress:develop Sep 22, 2020
@zenmonkeykstop
Copy link
Contributor Author

A good future improvement against this would be to have testinfra read from site-specific when run in prod for the IP addresses etc mentioned above (also reinstating the iptables checks). When I can stand to look at this code again I'll see about adding that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate running test_infra tests against production HW
4 participants