Page MenuHomePhabricator

Remove an old digest in Celerity code and some obsolete configuration options
ClosedPublic

Authored by epriestley on Jan 2 2019, 4:56 AM.

Details

Summary

Ref T12509. This upgrades a weakDigest() callsite to SHA256-HMAC and removes three config options:

  • celerity.resource-hash: Now hard-coded, since the use case for ever adjusting it was very weak.
  • celerity.enable-deflate: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
  • celerity.minify: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.

In the latter two cases, the options were purely developer-focused, and it's easy to go add an && false somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.

The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.

(An earlier version of this change did more magic with celerity.resource-hash, but this ended up with a more substantial simplification.)

Test Plan

Grepped for removed configuration options.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 2 2019, 4:56 AM
epriestley requested review of this revision.Jan 2 2019, 4:58 AM
epriestley planned changes to this revision.Jan 2 2019, 5:02 AM

Oh, this is actually a little more complicated than it seems: now, different developers will generate different local values for the HMAC key and thus generate different Celerity maps.

I'll either add an option like "get a named HMAC key, using this default one if no key exists yet" or simply hard-code this the key since the amount of complexity here is starting to feel hugely not-worth-it given that no one has ever needed to adjust this option, as far as I know.

epriestley updated this revision to Diff 47614.Jan 3 2019, 12:24 PM
  • Just switch to a hard-coded hash, since the code ends up being quite a bit simpler.
  • This triggers a one-time full rebuild of the map.

Broadly, the need to change hashes dates from a time at Facebook circa 2007 where:

  • The stack itself didn't work quite right.
  • Akamai had some odd behavior with caching certain resource/response types.
  • Akamai didn't have an easy/fast/reliable "drop everything out of cache" button.

We have a stack that works right and modern CDNs also generally work better, so none of these dirty-CDN-cache issues really appear to be issues anymore.

amckinley accepted this revision.Jan 3 2019, 7:36 PM
This revision is now accepted and ready to land.Jan 3 2019, 7:36 PM
epriestley edited the summary of this revision. (Show Details)Jan 3 2019, 7:42 PM
This revision was automatically updated to reflect the committed changes.