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

Do not declare logo as conffile #6101

Merged
merged 2 commits into from
Sep 28, 2021
Merged

Do not declare logo as conffile #6101

merged 2 commits into from
Sep 28, 2021

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Sep 21, 2021

Resolves #5850

Status

Ready for review.

(conffiles needs to be preserved as a zero-byte file to squash the autogenerated one.)

Testing

  1. Set up a staging environment without this change
  2. Upload a custom logo
  3. make build-debs from this branch
  4. Install updated packages
    • Observe that packages are updated successfully and that logo image is not clobbered

Deployment considerations

This change will clobber custom logos on very long-running installations that were upgraded during a specific time window; see #5850 (comment) . Only a very small number of installations are likely to be impacted, which we can resolve with direct notices.

@eloquence eloquence requested a review from a team as a code owner September 21, 2021 03:57
@eloquence eloquence force-pushed the no-logo-conffile branch 3 times, most recently from bc3df94 to 7effd06 Compare September 21, 2021 06:01
@eloquence
Copy link
Member Author

Per comments on #5850 I've added a migration function for old logos to preinst; I've also removed some very old logo-related migration code that should no longer be needed.

The catch is this: When I look at stock logos in the wild, I am encountering different checksums. For example, sha256sum for the version in git is 32bb28c760da2731111b6785c960350f8d3e80c900fc8bf8535fe1aee5a5384c, for the copy on my own instance is ce40b8627790e42e4bcd5951010e892a6d52e01f6ee02044b39305d74e2d625e, and for several copies in the wild is b6ed25fd0decf6c57e16b059b3a2ee81e5a48915479e58e6d6ec3f56f375389f. There are some byte-for-byte differences between the files but they are visually identical. I'm not sure what explains those differences, but it makes it hard to rely on checksums as a way to detect whether an instance has customized the stock logo or not.

Options that I see:

  • Ditch the migration and just notify the ~8 instances whose logo will get clobbered by this upgrade.
  • Avoid the checksum test altogether and just copy any existing stock logo to custom_logo.png in preinst. Downside: Any updates to the stock logo will have zero effect on running instances.
  • Keep/improve the checksum comparison.
  • Abandon this effort and live with the fact that we unnecessarily declare a file as a conffile, forever.

Honestly, I am leaning towards the first option, there are diminishing returns in trying to handle this migration gracefully, and it'd be nice to get rid of this bit of technical debt to avoid future issues. But please let me know if I'm missing something, obvious or not!

@zenmonkeykstop
Copy link
Contributor

Clobbering the logos of instances with admins that may not be responsive to comms seems like something to avoid, even if there's only a few of them.

We're unlikely to update the stock logo anyway so I'd argue for option 2 - also, if folks already have updated the stock file it makes sense to me to copy that to custom-logo.png, as that's obviously the one they want to display!

@eloquence
Copy link
Member Author

OK, it's not clobberin' time. Have simplified the preinst migration accordingly.

I tested the previous preinst version in staging but not this one yet. Assuming this works well, we could potentially drop it a release or two down the road, if there are no issues with instance upgrades.

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.

Tested using the upgrade scenario documented in https://docs.securedrop.org/en/latest/development/upgrade_testing.html Confirmed that a custom logo was not overridden after upgrade. During upgrade, the checksum changed for /var/www/securedrop/static/i/logo.png, but not for /var/www/securedrop/static/i/custom_logo.png, as expected.

@conorsch conorsch merged commit 1410ca6 into develop Sep 28, 2021
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.

Do not treat logo image as a conffile in app code package
3 participants