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

Update base templates to buster #352

Merged
merged 13 commits into from Dec 3, 2019
Merged

Update base templates to buster #352

merged 13 commits into from Dec 3, 2019

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Nov 27, 2019

Closes #345

For new installs: Buster templates by default
For existing installs: In-place update to buster

Note that the upgrade logic will shut down sd-svs and sd-gpg (normally only a single time)

Testing

Clean scenario

  • make all and all tests should pass

Upgrade scenario

  • make all and all tests should pass

@emkll emkll added this to Ready for Review in SecureDrop Team Board Nov 27, 2019
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.

Ran through a make all locally, and I'm pleased to report all tests were passing! The cleanup logic is sound. There are certainly conflicts with the outstanding changes in #351, so let's coordinate where changes should land.

The specific request I have is that the new cleanup script logic be called via Salt (but not ported to Salt tasks!). If it's called via salt, then our coalescing unattended-upgrade plans will be able to trigger these VM migrations without any Admin action, same as we did in #331.

# DispVM templates.

# sd-svs, we simply shutdown the machine as we want to preserve the data
qvm-check sd-svs > /dev/null 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: qvm-check --quiet will omit the user-facing messages without masking all errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also collapse the conditional into:

if qvm-check --quiet sd-svs; then

and skip checking the exit code separately.

if [[ ! $BASE_TEMPLATE =~ "buster" ]]; then
qvm-check --running sd-svs > /dev/null 2>&1
if [[ $? -eq 0 ]]; then
qvm-shutdown sd-svs && sleep 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use qvm-shutdown --wait to block, rather than sleeping.

qvm-shutdown sd-gpg
sleep 5
fi
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

At the end of this script seems to be the appropriate time to clean up the old VMs. That, or in a subsequent Salt task. Hardcoding the VMs to remove seems to be what we want. This Python one-liner describes the logic:

python3 -c 'from qubesadmin import Qubes; q = Qubes(); sdw_vms = [x.name for x in q.domains \
            if "sd-workstation" in x.tags and x.klass == "TemplateVM" \
            and "buster" not in x.name \
            and not x.installed_by_rpm ]; print("\n".join(sdw_vms))'

I don't recommend we use that convoluted one-liner, although it does illustrate how useful the Admin API can be for use cases like this. For now, we can simply hardcode the list of "old" VMs in a Salt list and ensure they're gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we cannot set them as part of this script, because the old (stretch) templates are still being used: we need to wait for the state of the templates to be applies, so that the stretch templates are no longer being used by an appvm

@@ -7,7 +7,7 @@ set -o pipefail

# Format list of all VMs comma-separated, for use as qubesctl target
all_sdw_vms_target="$(./scripts/list-vms | perl -npE 's/\n/,/g' | perl -npE 's/,$//' )"

templates_to_remove="$(./scripts/list-vms old-templates | perl -npE 's/\n/ /g')"
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. that #351 removes the list-vms script, in favor of the new qvm-ls --tags support. As above, if we perform the removal in Salt, we can execute in parallel against all of them, as long as we wait until the bash script has finished.

# This is required only when swapping base templates, see scripts/prep-upgrade
# for more details.
echo "Prep for upgrade, shut down AppVMs that require reboot for template swap"
./scripts/prep-upgrade
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend moving the script call out of the provision-all script and into a Salt state, reason being that then we have the ability to trigger the change via unattended-upgrades, e.g. via dom0 state.highstate. Currently only developers run this bash script, so let's not depend on it overmuch to enforce updates we want to land on pilot/prod instances.

@@ -27,3 +32,6 @@ echo "Provision all SecureDrop Workstation VMs with service-specific configs"
# The max concurrency reduction (4->2) was required to avoid "did not return clean data"
# errors from qubesctl. It may be possible to raise this again.
sudo qubesctl --show-output --max-concurrency 2 --targets "$all_sdw_vms_target" state.highstate

echo "Cleaning up old templates"
./scripts/destroy-vm "$templates_to_remove"
Copy link
Contributor

@conorsch conorsch Nov 27, 2019

Choose a reason for hiding this comment

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

Same as above: let's port the removal to Salt, so it's enforced even during highstate runs when the provision-all bash script isn't called.

@@ -26,7 +26,7 @@ def test_rpm_repo_config(self):
"gpgcheck=1",
"gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation-test", # noqa
"enabled=1",
"baseurl=https://dev-bin.ops.securedrop.org/dom0-rpm-repo/",
"baseurl=https://yum-test.securedrop.org/workstation/dom0/f25",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing this! Heads up, fix also exists in #351.

@emkll emkll moved this from Ready for Review to In Development in SecureDrop Team Board Nov 28, 2019
@emkll emkll force-pushed the 345-debian-buster branch 3 times, most recently from 33b6a1a to 127667a Compare December 2, 2019 16:10
@emkll emkll marked this pull request as ready for review December 2, 2019 16:13
@emkll emkll moved this from In Development to Ready for Review in SecureDrop Team Board Dec 2, 2019
@redshiftzero
Copy link
Contributor

i'm seeing concurrent.futures as uninstalled for python 2 in the buster base template - my understanding is that salt is using this - can someone explain?

@conorsch
Copy link
Contributor

conorsch commented Dec 2, 2019

@redshiftzero Judging by QubesOS/qubes-issues#4272 (comment), we should re-evaluate whether we must ensure the python futures apt package is present.

@conorsch
Copy link
Contributor

conorsch commented Dec 2, 2019

After some code spelunking by @rmol, we're satisfied that the Qubes salt mgmt logic is using python3 on the target VMs, meaning that concurrent is no longer necessary.

For all SecureDrop-workstation provisioned VMs, with the exception of Whonix-based templates. This will not replace templates in-place for existing installs.

Update tests to reflect new kernel version in buster template (4.14.151) and localname (-workstation suffix).
This is required because sd-proxy uses Whonix-14 (Stretch) based VMs. This should be removed once we move sd-proxy to Whonix-15
This will allow for in-place upgrades for all VMs based on this template.
This will allow for in-place upgrades for all VMs based on this template.
This will allow for in-place upgrades for all VMs based on this template.
…mplate swapped, by ensuring the following:

1. The AppVMs to be upgraded are shutdown
2. The AppVMs to be upgraded are not DispVM templates
3. To satisfy (2), the AppVMs that are DispVM templates cannot be the default DispVM template in qubes-prefs
Shut down and remove the following Debian Stretch templates:
* sd-svs-template
* sd-svs-disp-template
* sd-export-template
* securedrop-workstation

Adds test to ensure the templates have been properly deleted.
Ensures the changes are applied to the correct template
@emkll
Copy link
Contributor Author

emkll commented Dec 2, 2019

rebased on latest master, which now includes #351

Conor Schaefer added 2 commits December 2, 2019 15:33
Leveraging Salt "grains" (equivalent of Ansible "facts") to detect
Buster/Stretch intelligently. Whichever the target system is using will
be substituted in-place. Saves us the headache of maintaining different
Salt state files for backwards compatibility as we drive toward the
Stretch -> Buster conversion for all SDW templates.
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.

Ran through another test round, changes look great. I have some minor comments in-line that warrant your attention, @emkll, but most are superficial.

I'm requesting changes mostly to block merge so that you can review two commits (one reversion, one addition) related to the apt repo logic. If you agree that's what we want, then up to you to squash/rebase as you see fit.

dom0/fpf-apt-test-repo-stretch.sls Outdated Show resolved Hide resolved
dom0/fpf-apt-test-repo.sls Outdated Show resolved Hide resolved
scripts/handle-upgrade Outdated Show resolved Hide resolved
scripts/handle-upgrade Outdated Show resolved Hide resolved
scripts/prep-salt Outdated Show resolved Hide resolved
tests/test_vms_platform.py Show resolved Hide resolved
dom0/sd-svs-disp.sls Outdated Show resolved Hide resolved
tests/test_dom0_config.py Outdated Show resolved Hide resolved
@emkll
Copy link
Contributor Author

emkll commented Dec 3, 2019

Thanks @conorsch your changes look good to me, consider those approved. They have been tested in both install and upgrade scenarios: make all and make test pass in both cases (including additions described below)

I've resolved all but one review comment in b9f07e1, commented on that final comment (set -e).

I've also appended two further more commits, for your review, to ensure test coverage of the conditional apt-test logic, as well as improvements to the cleanup logic.

* Move handle-upgrade to dom0/securedrop-handle-upgrade
* Replace buster with sd-buster
* Invert test logic for template deletion
* set -e on securedrop-handle-upgrade script
…/salt

* Delete files prefixed by securedrop- and fpf in /srv/salt/
Ensures the securedrop_workstation.list apt source is correctly populated.
@emkll
Copy link
Contributor Author

emkll commented Dec 3, 2019

Just pushed some final changes (see https://github.com/freedomofpress/securedrop-workstation/compare/469a3fe..7e56fca) addressing the comments in #352 (comment)

This is now ready for re-review

@conorsch
Copy link
Contributor

conorsch commented Dec 3, 2019

Roger that. Proceeding with upgrade test. In preparation, I provisioned from latest master (75de802), and will apply these changes without a clean to verify the upgrade-in-place logic.

@conorsch
Copy link
Contributor

conorsch commented Dec 3, 2019

All tests passing for the upgrade-in-place scenario. Overall, the process is remarkably smooth. Thanks for the hard work on this one, @emkll.

@conorsch conorsch merged commit 7c19c63 into master Dec 3, 2019
SecureDrop Team Board automation moved this from Ready for Review to Done Dec 3, 2019
@emkll emkll deleted the 345-debian-buster branch December 17, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Specify Buster-based securedrop-workstation template as new default
3 participants