-
Notifications
You must be signed in to change notification settings - Fork 46
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
Uses local RPM build for "dev" and "staging" scenarios #587
Conversation
cf454cf
to
7885cc3
Compare
Marking as ready-for-review. Rebased these changes and tested locally, and was pleased with the results, so it's ready for more eyes. |
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.
Went through a preliminary, functional testing worked as expected. A couple of thoughts inline
# in the subsequent tarball, which is fetched to dom0. | ||
function build-dom0-rpm() { | ||
printf "Building RPM on %s ...\n" "${dev_vm}" | ||
qvm-run -q "$dev_vm" "make -C $dev_dir dom0-rpm" |
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.
would it make sense here to bump the RPM version to make sure that the version is always higher than anything available?
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.
I'm not aware of any RPM tooling like dch
for programmatically bumping the version. Ideally it'd be something like <current_version><alpha1><git_short_hash>
, but absent tooling, simply defaulting to <current_version>
makes the most sense to me.
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.
Went through this again, from a development scenario perspective, changes look good and are a significant improvement for developers and brings more consistency across environments.
However it looks like there might be some implications for prod/staging scenarios (see inline). It might make sense to only invoke prep-salt
exclusively for the dev scenario (instead of conditionals).
There's also a potential versioning issue: if the version of the local RPM is lower than the one served by the rpm repo, changes to the salt files may be overwritten by the ones on the RPM servers. We can autoupdate but we can also just provide a docs update to unblock, since this will only affect developers.
Finally, tests rely on the create_tarball
function https://github.com/freedomofpress/securedrop-workstation/pull/587/files#diff-3c3ec2225864916f85515009ebeeb428L24 . Packaging the tests and reducing/eliminating the requirement of a full clone of the repository in dom0 may help reduce developer error (changes to salt files in the dom0 securedrop-workstation folder will effectively be useless). I don't think this should be immediately addressed as part of this PR, but should probably be tracked in a follow-up issue
UPDATE: we will need to rebase and update references to the installer (securedrop-admin
-> sdw-admin
since #596 has been merged)
We should clarify the README as part of this PR, @conorsch if you want I can add a commit to take a stab at it once other comments are addressed. |
(Needs rebase after #596) |
Builds the RPM in sd-dev VM, then fetches it to dom0 for local installation via the make-clone action. Simplifies the provisioning logic by trusting dnf to handle the add/remove of all config files. Adjusts the docker build command, removing flag for interactive session. In order to run the build via qvm-run from dom0, we must remove the flag, otherwise `docker run` fails with: the input device is not a TTY There are no prompts in the rpm build process.
Now we use the RPM to manage presence/absence of all scripts and configs, Salt or otherwise. So we don't want to double-remove the files, we'll trust the "dnf remove" action to handle it, regardless of environment (dev, staging, or prod).
Adjusts the "make clean" target to reuse the local securedrop-admin script for provisioning. Added two new cli flags to the script, both off by default, to accommodate dev-scenario settings: --keep-template-rpm (to avoid time spent redownloading) and --force (to avoid prompts).
Previously it was emitting helpful text, but still exiting zero, so when used in chains like "make all && make test" it would try to continue processing.
7885cc3
to
2a31b21
Compare
Pointed out by @emkll during review: the "staging" and "prod" makefile targets were including "prep-salt", but they really don't need to. So, removing them, and renaming the target accordingly.
Rebased to resolve conflicts, and implement a few requested changes. Ready for review! @eloquence To your point about the README, thank you, I'd welcome fresh eyes on docs updates here. |
Downgraded to draft, to block merge until we've got the docs updated. Still ready for functional review. |
Per our discussion at standup, I've added a commit 9dc287a that updates the That commit also removes the While nightly RPMs are currently "temporarily" disabled, I would propose to block merge of this PR until freedomofpress/securedrop-builder#186 is resolved, as the current CircleCI configuration still suggests that we may want to re-enable nightly RPM builds at a later date, which would be unsafe to do in combination with this change. |
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 @conorsch and @eloquence. I have tested both the staging and dev environments locally, left a few notes inline mostly related to documentation. I think this is otherwise good to squash and promote for final review/merge
README.md
Outdated
@@ -192,9 +192,9 @@ In the Qubes Menu, navigate to `System Tools` and click on `Qubes Update`. Click | |||
|
|||
You can install the staging environment in two ways: | |||
|
|||
- If you have an up-to-date clone of this repo with a valid configuration in `dom0`, you can use the `make staging` target to provision a staging environment. Prior to provisioning, `make staging` will set your `config.json` environment to `staging`. As part of the provisioning, your package repository configuration will be updated to use the latest test release of the RPM package, and the latest nightlies of the Debian packages. | |||
- If you have an up-to-date clone of this repo with a valid configuration in `dom0`, you can use the `make staging` target to provision a staging environment. Prior to provisioning, `make staging` will set your `config.json` environment to `staging`. As part of the provisioning, a locally built RPM will be installed in dom0, and your package repository configuration will be updated to use the latest test release of the RPM package, and the latest nightlies of the Debian packages (same as `make dev`). |
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.
your package repository configuration will be updated to use the latest test release of the RPM package
This could be somewhat misleading. If the version of the locally built package is equal or higher than the one mirrored by yum-test, the latest test release of the RPM package will not be used (but it will receive test package updates). We should specify to use the prod install procedures to fully mirror the prod flow (or ensure the RPM is build off the latest tag/rc when cloning the repo in the dev VM). (related to comment in https://github.com/freedomofpress/securedrop-workstation/pull/587/files#r458880338)
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.
Fair point. Rephrased as:
As part of the provisioning, a locally built RPM will be installed in dom0. The dom0 package repository configuration will be updated to install future test-only versions of the RPM package from the https://yum-test.securedrop.org repository, and Workstation VMs will receive the latest nightlies of the Debian packages (same as
make dev
).
README.md
Outdated
@@ -313,7 +313,7 @@ make clone | |||
make dev | |||
``` | |||
|
|||
In the future, we plan on shipping a *SecureDrop Workstation* installer package as an RPM package in `dom0` to automatically update the salt provisioning logic. | |||
The `make clone` command will build a new version of the RPM package that contains the provisioning logic and copy it to `dom0`. |
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.
We should state here that the rpm package is built in the dev env. We should also include a note stating that Docker is now strictly required in the Dev VM
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.
This is stated in the setup instructions for sd-dev
:
You must install Docker in that VM in order to build a development environment using the standard workflow.
But I agree a parenthetical here won't hurt as well, will add a commit to that effect.
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.
A bit more detail added in 902fc18
|
||
{% else %} | ||
|
||
{% if d.environment != "dev" %} |
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.
If I understand correctly, here we are explicitly excluding dev from installing the RPM to avoid installing the latest from the yum repos should the locally built version be lesser than the ones on the server. if that's the case, it may be worth adding a comment here for future maintainers as it is somewhat counter-intuitive.
sudo qubesctl --show-output state.sls sd-workstation-buster-template | ||
sudo qubesctl --show-output --skip-dom0 --targets sd-workstation-buster-template state.highstate | ||
|
||
sd-proxy: prep-salt ## Provisions SD Proxy VM | ||
sd-proxy: prep-dev ## Provisions SD Proxy VM |
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.
The changes to the dev environment may have repercussions on these individual makefile targets :
prep-salt
makefile target would copy the local salt file in /srv/salt/
whereas this updated prep-dev
target will install the dom0 rpm. This means that any changes to the local files in the securedrop-workstation
folder in dom0 will not be used.
If this is the case, adding a note to this effect in the dev docs could be helpful
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.
If this is the case, adding a note to this effect in the dev docs could be helpful
That's definitely true. Editing the files in e.g. /srv/salt/ still works fine, but I wouldn't recommend either, given how easy it is to lose changes that way. Will clarify in the docs!
Suggested by @emkll as part of review. Explains: * dev changes should be made in sd-dev * 'make dev' tries to skip pulling rpm from repos * explaining that rpm upgrades in staging are ideal, not guaranteed, depending on version number
Added commits addressing requests for further docs clarifications. Updated the test plan to include staging references. Marking ready for final review. |
Running through test scenarios on a fresh Qubes install (no previous dev env),
|
copying
|
@zenmonkeykstop You're right, there are a bunch of |
|
Since Salt state files require access to the json file to import, we should be using the
Unintuitive, but running 'make clean' will actually install the rpm, then remove everything, because the files for removal are installed by the RPM. That's not new in this PR, that's true even on the main branch now, where "prep-salt" is run as a dependency of make-clean. |
The salt states require secrets such as the config file and privkey to be stored in `/srv/salt/sd/`, as the import lines in the state files assume those locations. The sdw-admin install action copies those files into place, but in the case of "make clean", the secrets won't be there. Let's make sure they are, and make sure to remove them (as on main) via the uninstall action.
@zenmonkeykstop The latest commit now supports make-clean even when no prior installation has been run. I did not move the import locations, preferring /usr/share/, but rather re-added the copy-to-srv-salt logic. There are two secrets, the privkey and the config.json file, and the privkey needs a "salt:// |
Was still used by sdw-admin, so we must keep it around, even if it's not useful in the raw Makefile targets.
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.
Test plan for dev and staging is passing for me now. There is one unrelated test failure in make test
due to that race condition with systemctl in sd-log
, but otherwise this LGTM!
That'd be #583. Thanks for thorough review, @zenmonkeykstop ! |
Subsequent review performed by @zenmonkeykstop
Follow up to #587. The `dnf install -y <local_file>` action will not reinstall a package if the versions are the same. Since we expect package contents to change, but not version strings, when running `make clone`, let's make sure to uninstall the rpm package in dom0 so that the install action always takes effect.
Follow up to #587. The `dnf install -y <local_file>` action will not reinstall a package if the versions are the same. Since we expect package contents to change, but not version strings, when running `make clone`, let's make sure to uninstall the rpm package in dom0 so that the install action always takes effect.
* Uninstalls RPM in prep-dev script Follow up to #587. The `dnf install -y <local_file>` action will not reinstall a package if the versions are the same. Since we expect package contents to change, but not version strings, when running `make clone`, let's make sure to uninstall the rpm package in dom0 so that the install action always takes effect.
Uses local RPM build for "dev" and "staging" scenarios
Status
Ready for review
Description of Changes
Fixes #538
Changes proposed in this pull request:
Hopefully this will make reasoning about problems like #505 a lot more accessible to developers.
Testing
Requesting testing for multiple scenarios, since early review showed some surprises.
Dev scenario
There's a catch here—you must run
make clone
twice in order to get started, because the first clone will only pull in the new docker logic, not run it.Mess around with the clean/clone/dev/test combinations and see if you can get anything to break.
Staging scenario
Now let's check that "staging" scenario continues to work well. Notably the locally built RPM will be installed in staging.
Interact, confirm staging-specific functionality like shutdown-on-lid close is working, then run
make clean
to clean up and confirm no errors.Prod scenario
Since the prod flow uses a separate setup process (https://workstation.securedrop.org/en/stable/admin/install.html#download-and-install-securedrop-workstation), there shouldn't be any conflicts with the logic presented here. If you can think of some, please comment or directly edit this test plan!
Checklist
If you have made code changes
make flake8
) passes in the development environment (this box maybe left unchecked, as
flake8
also runs in CI)If you have made changes to the provisioning logic
All tests (
make test
) pass indom0
of a Qubes installThis PR adds/removes files, and includes required updates to the packaging
logic in
MANIFEST.in
andrpm-build/SPECS/securedrop-workstation-dom0-config.spec