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

[Refactor] Move all Salt provisioning files into /srv/salt/securedrop_salt #1048

Merged
merged 6 commits into from
Jun 3, 2024

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented May 24, 2024

Status

Ready for Review

Description of Changes

Towards #837 (this may be sufficient for our purposes to close it), helps with #1047 if we decide we like it

Changes proposed in this pull request:

  • Move files used in Salt provisioning into /srv/salt/securedrop_salt/ directory (and adjust imports and provisioning scripts accordingly). This means the sd-proxy, sd-whonix and sd-workstation directories are no longer used, and the dom0 directory becomes securedrop_salt.
  • files that were formerly in /srv/salt/sd/ or /srv/salt/usb-autoattach are now in /srv/salt/securedrop_salt.
  • Enable sd-workstation.top by name, instead of enabling all top files matching a given pattern. (If we ever add additional top files we will have to enable them specifically, which seems appropriately conservative.)
  • rpm .spec file now copies everything in securedrop_salt into /srv/salt/securedrop_salt on dom0. (This is a change from when we globbed by filename/type)
  • Similarly, MANIFEST.in includes everything in files/ and securedrop_salt/.

To sum up, that means

  • Everything in securedrop_salt/ gets provisioned to /srv/salt/securedrop_salt
  • Everything in files/ gets provisioned to somewhere else on dom0, as defined in the .spec file (future possibility of organizing this in a directory structure instead of in the spec file: Simpler rpm build (and/or more legible repo)  #1047)
  • Everything in scripts/ is responsible for building, linting, or dev provisioning [and is not provisioned to sdw laptops]
  • (The launcher stuff is a bit of an outlier but that's fine)

Testing

  • CI is green
  • Visual review
  • dom0 tests are passing

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing pilot instances
  2. New installs

Checklist

If you have made changes to the provisioning logic

  • All tests (make test) pass in dom0

If you have added or removed files

  • I have updated MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

If documentation is required

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation

@rocodes rocodes force-pushed the 837-salt-reorg branch 2 times, most recently from f4f42ee to 9a6862c Compare May 27, 2024 16:52
@rocodes rocodes changed the title [Refactor] Move all Salt provisioning files into /srv/salt/securedrop_salt [Refactor] Move all Salt provisioning files into /srv/securedrop_salt May 27, 2024
@rocodes rocodes force-pushed the 837-salt-reorg branch 3 times, most recently from fa6cf6a to e0cecb7 Compare May 31, 2024 17:03
@rocodes rocodes changed the title [Refactor] Move all Salt provisioning files into /srv/securedrop_salt [Refactor] Move all Salt provisioning files into /srv/salt/securedrop_salt May 31, 2024
@rocodes
Copy link
Contributor Author

rocodes commented May 31, 2024

I went back and forth a number of times on whether to put files in /srv/salt/securedrop_salt (a la unman/shaker) or in /srv/securedrop_salt (a la original #837 proposal, and in line with the user_dirs approach eg /srv/user_salt/).

  • /srv/securedrop_salt/ would be: more encapsulated, less possibility of interference in the target directory from a Qubes update, but potentially more prone to breakage (relies on the testing of user_dirs, see eg 4.2: user_salt isn't working QubesOS/qubes-issues#8491), maybe slightly more complex for us to implement on short notice, and maybe slightly more salt debugging required.
  • /srv/salt/securedrop_salt would be: simpler, since it wouldn't require any fussing to get qubesctl commands to work; less encapsulated, but also in line with some other Qubes integrations projects.

Since we're not wild about Salt and trying to make it less of a feature in our lives, I'm going (back to) the second option and just putting our stuff in a subdirectory of /srv/salt. It feels like an okay compromise between not making a mess in /srv/salt/ vs not adding too much complexity to our lives.

@rocodes rocodes force-pushed the 837-salt-reorg branch 3 times, most recently from 18e7ac7 to af738b8 Compare May 31, 2024 19:00
@rocodes
Copy link
Contributor Author

rocodes commented May 31, 2024

Not sure why sd testrunner isn't running on this

@rocodes rocodes force-pushed the 837-salt-reorg branch 4 times, most recently from b796dce to b2cd2db Compare May 31, 2024 22:33
@rocodes rocodes marked this pull request as ready for review May 31, 2024 22:41
@rocodes
Copy link
Contributor Author

rocodes commented May 31, 2024

Just ran through a successful provisioning locally - going to interrupt CI in order to sign that last commit, so it may not be green for another hour

Update rpm specfile.
Include all securedrop_salt files in MANIFEST.in.

Use securedrop_salt path in Jinja and sls requirement import statements.
@rocodes rocodes requested a review from a team May 31, 2024 22:55
dom0/sd-workstation.top Outdated Show resolved Hide resolved
files/clean-salt Outdated Show resolved Hide resolved
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Deployment is working and tests are passing - but the global copy of securedrop_salt includes the apt-test sources file and the test key. we shouldn't be packaging those in the RPM. They do need to be in place for dev and staging runs, - we could keep them in the repo and copy them in place via the make target (or some other solution).

include dom0/*.conf
include dom0/remove-tags.py
include dom0/securedrop-handle-upgrade
include securedrop_salt/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This includes the apt-test key and sources file. We probably should be excluding these and adding them in via the dev setup script when make dev or make staging is run instead. They shouldn't be shipped in the prod RPM.

install -m 755 -d %{buildroot}/srv/salt/sd/usb-autoattach

install -m 755 -d %{buildroot}/srv/salt/
cp -a securedrop_salt %{buildroot}/srv/salt/
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in manifest, should be excluding apt-test config.

@zenmonkeykstop
Copy link
Contributor

Withdrawing CR in favour of #1058 - @rocodes pointed out this is an existing issue with the RPM with a few potential solutions.

@zenmonkeykstop zenmonkeykstop added this pull request to the merge queue Jun 3, 2024
Merged via the queue into main with commit c827937 Jun 3, 2024
7 checks passed
@cfm cfm mentioned this pull request Jun 3, 2024
3 tasks
@legoktm legoktm deleted the 837-salt-reorg branch June 18, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants