-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
ratelimits: API improvements necessary for batches and limit fixes #7117
Conversation
The test failure is
The test that's triggering this states:
I think what you may want is for |
Thanks, it appears I got a little overzealous when chopping up the larger changeset to produce this PR. This missing clause' have been added back and now an error is returned, as intended. |
52d7bcb
to
3ce9716
Compare
Co-authored-by: Aaron Gable <aaron@letsencrypt.org>
The
Limiter
API has been adjusted significantly to both improve both safety and ergonomics and twoLimit
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 aBucketId
constructor for eachLimit
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 aLimit
name, a bucket identifier, and a cost to be spent/ refunded. To simplify this, each method now accepts a newTransaction
type which provides a cost, and wraps aBucketId
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
andCertificatesPerFQDNSet
, were implemented asCertificatesPerDomainPerAccount
andCertificatesPerFQDNSetPerAccount
.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.