Cache rules in front of the flag lookup - #45
Open
lbajsarowicz wants to merge 1 commit into
Open
Conversation
Every request that reaches the Shield plugin loads the sansec_shield_rules flag, which is a point SELECT on the flag table plus Flag model hydration. Put Magento's cache in front of it as a read accelerator: the flag stays the source of truth, a cache miss or a failing cache backend falls back to it, and both saveFlag() and deleteFlag() keep the cache in step. A 300 second lifetime bounds staleness to one cron cycle on cache backends that are not shared between nodes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Plugin\ShieldinjectsModel\Wafeagerly, andWaf::__construct()callsRules::loadRules(). So every request that reaches the plugin does:FlagResource::load()— a point SELECT onflagbyflag_code, throughAbstractDb::load()(connection lookup, select build, fetch,Flag::setData()).Flag::getFlagData()— JSON decode offlag_data.Step 2 is unavoidable; the rules have to be decoded. Step 1 is a database round trip repeated on every request for a value that only changes when the 5-minute cron writes it.
Design
The obvious move (store the rules in the cache) is the design this module already had and deliberately dropped in #13 (
c5085f2, "store shield rules in flag instead of cache"), which deletedModel/Cache/Type/CacheType.phpandetc/cache.xml. The failure mode that change removed:bin/magento cache:flush, a disabled cache type, or a cold cache after deploy left the WAF with no rules at all until the next cron run, a silent fail-open of up to five minutes. This PR does not reintroduce that.The rule here is that the flag remains the only source of truth. The cache is an accelerator with no authority:
saveFlag()writes the flag and then the cache, so rules synced by cron or bysansec:shield:sync-rulesare live on the next request without waiting for a cache miss.deleteFlag()(403 from the API, legacy flag format) removes the cache entry too, so revoked rules cannot survive in it.saveFlag()means the TTL is not what normally refreshes the entry, so on a shared backend it costs close to nothing: one extra flag read per five minutes. What it buys is a bound on staleness where the backend is not shared (see the multi-node row below) — an entry can then be at most one cron cycle behind the flag, instead of stale until someone cleans the cache.Failure modes, compared against
main:maincache:flush/cache:cleanmainWaf::__construct()into a 500deleteFlag(), no rules until next syncThere is no state in which the cache can hold rules the flag does not have, and no state in which an empty cache means an empty rule set.
Which cache:
Magento\Framework\App\CacheInterface, with the entry taggedSANSEC_SHIELD, not a dedicated cache type. A dedicated type (etc/cache.xml+ aTagScopesubclass, i.e. exactly what #13 removed) buys an independent flush switch in Cache Management, but that switch can only be used to turn the acceleration off; it cannot fix anything, because the flag is authoritative and correctness never depends on the cache. The cost is two files, an admin-visible toggle that does nothing an admin would want, and the reappearance of the machinery this project already decided to delete.CacheInterfacewrites to the default frontend, which is the same backend (Redis in most production topologies), respects the existing tag-based cleaning, and adds no configuration surface. Core uses dedicated cache types where the cache is the storage (config, layout, block HTML); this is not that case.Serialization: the cache payload goes through the module's
Model\Serializer(already injected intoRulesviaetc/di.xml), so the cached bytes are the same JSON, with the sameJSON_INVALID_UTF8_SUBSTITUTEbehaviour, as whatFlagstores inflag_data.Out of scope:
Waf's constructor still loads rules eagerly, signature verification, the fetch, and the cron schedule are untouched.Change
Model/Rules.php: injectMagento\Framework\App\CacheInterface; splitloadRules()into a cache read, a flag read, and a cache write with a 300-second lifetime. The read and the repopulating write are both guarded, becauseloadRules()runs insideWaf::__construct()and an exception there fails the whole request.saveFlag()'s write-through is deliberately left unguarded: it runs from cron and the CLI, where a cache error should surface in the log.Test/Model/RulesTest.php(new): cache hit never touches the flag resource; a cache miss loads the flag and populates the cache; a corrupt cache entry falls back to the flag; a cache write that throws still returns the flag rules; a missing flag is not cached;saveFlag()writes the cache;deleteFlag()removes it; the legacy (non-array) flag format still deletes both.Measurements
Be clear about what is and is not measured here.
What was measured: the decode step, in isolation, on PHP 8.3 (
php:8.3-cli-alpine),Test/fixture/testrules.json(12,464 bytes, 37 rules), 2,000 iterations after a warm-up:json_decode($json, true)Serializer::unserialize()json_encode(..., JSON_INVALID_UTF8_SUBSTITUTE)Serializer::serialize()The point of that table is that the decode is identical either way.
Flag::getFlagData()json-decodesflag_data; the cache path json-decodes the cached string. The moduleSerializercosts the same as rawjson_decode. This PR saves nothing on decoding. The entire benefit is the storage round trip.What is estimated, not measured: Magento cannot be booted in this environment, so the storage round trip is not benchmarked here. Order-of-magnitude figures from the usual public numbers for these paths:
AbstractDb::load(), DB on the same hostAbstractDb::load(), DB over the networkGET, same host or same LANTreat these as estimates. They are not from a run on this branch.
What follows honestly from them:
Anyone with a production install can confirm the direction cheaply: the
flagSELECT forsansec_shield_rulesdisappears from the query log on every request after the first.Testing
vendor/bin/phpunit --fail-on-warning Test— 65 tests, 80 assertions, green (PHP 8.3; the new tests use only PHPUnit 8 APIs).php -lacross the module andxmllint --nooutonetc/**/*.xml.