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

Warn or fail when running outdated playbook #5317

Closed
eloquence opened this issue Jun 16, 2020 · 5 comments · Fixed by #5788
Closed

Warn or fail when running outdated playbook #5317

eloquence opened this issue Jun 16, 2020 · 5 comments · Fixed by #5788
Assignees
Labels
Milestone

Comments

@eloquence
Copy link
Member

It's not uncommon that admins run an outdated playbook, because they've not updated their Admin Workstation for whatever reason (e.g. due to an update failure). Running an outdated playbook can have harmful consequences, reinstating changed configurations, or even breaking functionality. For an existing install, securedrop-admin install should attempt to obtain the version info from the server, and fail or warn if it doesn't correspond to the version info running on the workstation.

User Story

As an administrator, I don't want to accidentally mess up the configuration of my SecureDrop installation, so that I can avoid spending the rest of my afternoon debugging what I just did.

@eloquence
Copy link
Member Author

eloquence commented Jan 8, 2021

See the older issue #2964 for some ideas for how this could be implemented. Tentatively adding for consideration to the Focal epic, as this would potentially mark a nice cutoff for us to ensure that all new installs on Focal have this logic in place.

@zenmonkeykstop
Copy link
Contributor

Anecdata supports this issue!

@eloquence eloquence added this to the 1.8.0 milestone Jan 21, 2021
@eloquence eloquence self-assigned this Jan 21, 2021
@eloquence
Copy link
Member Author

eloquence commented Jan 23, 2021

Implementation proposal

After evaluating several options, I would propose a pragmatic implementation within securedrop-admin that relies on our existing check_for_updates code, hardened with additional error checks, and with more user-friendly output.

We would perform a check_for_updates run before any securedrop-admin subcommand that connects to the server (e.g., backup, install, restore). If it detects a mismatch with the latest git tag or the update check fails (e.g., GitHub not reachable), it would print an informative error message, asking the user to specify a --force argument if they are certain that they want to proceed.

Advantages of this approach:

  • It will work regardless of whether SecureDrop is already installed or not, and regardless of whether the server is reachable or not, preventing any potentially harmful uses of an outdated playbook
  • It relies on existing version check logic, minimizing our maintenance and testing footprint
  • It's easy to make this user-friendly without asking the user to parse Ansible's typically noisy output, and we can add additional messages or interactivity as we see fit
  • As far as I can tell, it's a relatively small diff

Disadvantages:

  • We would have to use --force routinely during QA due to the ~rc version mismatch (arguable whether it's a disadvantage -- we're running unreleased code against the server during QA, so a --force argument may be warranted)
  • Doesn't handle some edge cases well (both of which can already arise due to auto-updates of workstations, so I do not view this as a regression)
    • Server has fallen behind for an unclear reason
    • Admin runs securedrop-admin subcommands after a new release has been tagged but before their server has been updated

Alternatives worth considering

  • Running an Ansible task which fetches version.py (or checks the securedrop-app-code package version) and compares its output with the local git status output
    • Can't handle outdated playbook for fresh install (at least not without falling back on securedrop-admin logic to check git tags)
    • Error output may be noisy and difficult to interpret
    • Ansible's version check behavior is not clearly specified, and its behavior may differ in edge cases from our existing version checks
  • In securedrop-admin, hitting the metadata endpoint if we have a local onion service definition we can curl
    • Could be combined with a git version check in securedrop-admin, but seems overly complicated, slow, and potentially error-prone if server is not reachable

@eloquence
Copy link
Member Author

eloquence commented Feb 10, 2021

Started with the securedrop-admin approach here: c007e1b

Early comments welcome, next I'll take a closer look at error cases and tests.

@eloquence
Copy link
Member Author

eloquence commented Feb 11, 2021

In 4192093, I've refactored this into a decorator, which allows us to mark any securedrop-admin subcommand as requiring an update check (that can be optionally skipped). Since this seems like a good idea for more than just the install playbook, this approach seemed very DRY to me. Comments again welcome; if this looks good in principle, I'll flesh out the type annotation, tests and error handling.

@eloquence eloquence removed the needs/discussion queued up for discussion at future team meeting. Use judiciously. label Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants