-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
Deprecate DisableLegacyLimitWrites & UseKvLimitsForNewOrder flags; remove code using certificatesPerName & newOrdersRL tables #7858
base: main
Are you sure you want to change the base?
Conversation
…move code using certificatesPerName & newOrdersRL tables
@jprenken, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
IN-10906 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small nits, and one high-level question: can the PR description grow a paragraph describing how the deleted RA test cases are covered by WFE kv-limit test cases?
Addressed and added, thanks! I think there does need to be a replacement for |
Remove code using
certificatesPerName
&newOrdersRL
tables.Deprecate
DisableLegacyLimitWrites
&UseKvLimitsForNewOrder
flags.Remove legacy
ratelimit
package.Delete these RA test cases:
TestAuthzFailedRateLimitingNewOrder
(rl:FailedAuthorizationsPerDomainPerAccount
)TestCheckCertificatesPerNameLimit
(rl:CertificatesPerDomain
)TestCheckExactCertificateLimit
(rl:CertificatesPerFQDNSet
)TestExactPublicSuffixCertLimit
(rl:CertificatesPerDomain
)Rate limits in NewOrder are now enforced by the WFE, starting here:
boulder/wfe2/wfe.go
Line 781 in 5a9b4c4
We collect a batch of transactions to check limits, check them all at once, go through and find which failed, and serve the failure with the Retry-After that's furthest in the future. All this code doesn't really need to be tested again; what needs to be tested is that we're returning the correct failure. That code is
NewOrderLimitTransactions
, and theratelimits
package's tests cover this.Add a WFE test for the public suffix handling behavior, specifically, which did not have existing coverage outside the RA.
Some other RA rate limit tests were deleted earlier, in #7869.
Part of #7671.