Initial implementation of key-value rate limits#6947
Conversation
aarongable
left a comment
There was a problem hiding this comment.
I haven't yet reviewed gcra.go; sending comments so you have them before I disappear into other stuff for the afternoon.
When you check out gcra.go use https://brandur.org/rate-limiting#gcra instead of my old (and I now realize) wrong explainer in the design doc. |
|
Merged main so we can have working CI again (#6997). |
Co-authored-by: Phil Porada <pgporada@users.noreply.github.com>
Co-authored-by: Phil Porada <pgporada@users.noreply.github.com>
Co-authored-by: Phil Porada <pgporada@users.noreply.github.com>
Co-authored-by: Phil Porada <pgporada@users.noreply.github.com>
| ) | ||
|
|
||
| func Test_decide(t *testing.T) { | ||
| clk := clock.NewFake() |
There was a problem hiding this comment.
Nitpick: You could declare these variables in each specific test scope and reference them throughout the test. Where this helped me was in all the test.AssertEquals(t, d.ResetIn, burstTime) helping to hammer home the TAT related to burst concept.
const burst = 10
const burstTime = (time.Second * burst)
|
The The new So this PR as-merged was still an API PoC - right? Where a storage backend for the bucket states is still TBD and none of the exiting rate-limits code refers to this new API yet? |
|
Correct. As this PR states, it is the "initial implementation". None of the ACME API code paths invoke this new rate limit infrastructure yet, and the "inmem" backing store (which cannot share data between multiple instances of the same service, and would drop all of its data every time that service is restarted e.g. for a deploy) is obviously only intended for testing purposes, not production use. The next steps here are to implement a closer-to-production-ready backing store, and to begin calling into this code for least one actual rate limit, so that we can see how well this system performs in practice. |
…7117) The `Limiter` API has been adjusted significantly to both improve both safety and ergonomics and two `Limit` types have been corrected to match the legacy implementations. **Safety** Previously, the key used for looking up limit overrides and for fetching individual buckets from the key-value store was constructed within the WFE. This posed a risk: if the key was malformed, the default limit would still be enforced, but individual overrides would fail to function properly. This has been addressed by the introduction of a new `BucketId` type along with a `BucketId` constructor for each `Limit` type. Each constructor is responsible for producing a well-formed bucket key which undergoes the very same validation as any potentially matching override key. **Ergonomics** Previously, each of the `Limiter` methods took a `Limit` name, a bucket identifier, and a cost to be spent/ refunded. To simplify this, each method now accepts a new `Transaction` type which provides a cost, and wraps a `BucketId` identifying the specific bucket. The two changes above, when taken together, make the implementation of batched rate limit transactions considerably easier, as a batch method can accept a slice of `Transaction`. **Limit Corrections** PR #6947 added all of the existing rate limits which could be made compatible with the key-value approach. Two of these were improperly implemented; - `CertificatesPerDomain` and `CertificatesPerFQDNSet`, were implemented as - `CertificatesPerDomainPerAccount` and `CertificatesPerFQDNSetPerAccount`. Since we do not actually associate these limits with a particular ACME account, the `regID` portion of each of their bucket keys has been removed.
This design seeks to reduce read-pressure on our DB by moving rate limit tabulation to a key-value datastore. This PR provides the following:
Note: the included source implementation is test-only and currently accomplished using a simple in-memory map protected by a mutex, implementations using Redis and potentially other data stores will follow.
Part of #5545