ra/ratelimits: Update tests, use new TransactionBuilder constructor, fix ARI rate limit exception

[jprenken]

Add a new ratelimits.NewTransactionBuilderWithLimits constructor which takes pre-populated rate limit data, instead of filenames for reading it off disk.

Use this new constructor to change rate limits during RA tests, instead of using extra testdata files.

Fix ARI renewals' exception from rate limits: consider isARIRenewal as part of the isRenewal arg to checkNewOrderLimits.

Remove obsolete RA tests for rate limits that are now only checked in the WFE.

Update remaining new order rate limit tests from deprecated ratelimits to new Redis ratelimits.

[aarongable]

Largely LGTM, loving all the deletions in ra_test.go. Just a question about whether this could be even a bit more elegant.

[aarongable]

Changes to the ratelimits package LGTM. I think my only remaining question is around the tests deleted from ra_test.go: can we get confirmation that the behavior each of these tests cares about is covered by a different key-value-limits-based test?

For example, the only references to .IsARIRenewal that I see in ra_test.go are both in deleted tests. Shouldn't we still be testing that ARI-Renewal orders are exempt from key-value rate limits?

[jsha]

Looking through the deleted test cases, to summarize, they are:

TestNewOrderRateLimiting
TestEarlyOrderRateLimiting
TestExactPublicSuffixCertLimit
TestPendingAuthorizationsUnlimited // This one can definitely go away, there's no pending authz limit in new world
TestNewOrderCheckFailedAuthorizationsFirst
TestNewOrderRateLimitingExempt
TestNewOrderFailedAuthzRateLimitingExempt

The .IsARIRenewal field Aaron pointed out is in TestNewOrderRateLimitingExempt and TestNewOrderFailedAuthzRateLimitingExempt.

[jsha]

In the new rate limiting world, the new-order limits are enforced by the WFE, not the RA. I found this test in the WFE that simulates ARI renewal, TestCountNewOrderWithReplaces: https://github.com/letsencrypt/boulder/blob/6ee90ffdda3523b39a2bab5fe5c09d99e884e139/wfe2/wfe_test.go#L4424-L4470

But the asserts seem to only be verifying that the request succeeds and that metric is incremented. They don't test for the rate limit exemption.

Probably what's called for is a new unittest in the WFE that checks for the ARI-related rate limit conditions.

[jprenken]

I'll describe the deleted test cases in abbreviated pseudocode to improve my own understanding, then go checking for equivalent test coverage.

• TestNewOrderRateLimiting: Set the NewOrdersPerAccountPolicy threshold to 1. Try two orders; the second should fail. Try the first order again; reuse should permit it. Advance the clock. Try the second order again; it should now succeed.

• TestEarlyOrderRateLimiting: Override the CertificatesPerNamePolicy threshold to 0 (always fail the RL). Set the NewOrdersPerAccountPolicy threshold to 10. Try an order; it should fail with the expected error message.

• TestExactPublicSuffixCertLimit: This one wasn't deleted, just changed. Skipping.

• TestNewOrderCheckFailedAuthorizationsFirst: "Test that the failed authorizations limit is checked before authz reuse. Create an order (and thus a pending authz)." Treat it "as if it had a recent failure, but also a valid authz." Set the InvalidAuthorizationsPerAccountPolicy threshold to 1. Try the order; it should fail with the expected error message.

• TestNewOrderRateLimitingExempt: Set the NewOrdersPerAccountPolicy threshold to 1. Try two orders; the second should fail. Set the second order's .IsARIRenewal to true. Try the second order again; it should now succeed.

• TestNewOrderFailedAuthzRateLimitingExempt: "Create an order, and thus a pending authz." Mock a failed authz. Set the InvalidAuthorizationsPerAccountPolicy threshold to 1. Try the order; it should fail with the expected error message. Set the order's .IsARIRenewal to true. Try the order again; it should now succeed.

[aarongable]

In the new rate limiting world, the new-order limits are enforced by the WFE, not the RA.

Thank you! I knew that we'd previously talked about it generally making sense to delete these, but couldn't recreate that reasoning in my head.

[jprenken]

boulder/wfe2/wfe.go

Line 2439 in 81b616b

refundLimits, err := wfe.checkNewOrderLimits(ctx, acct.ID, names, isRenewal)

is ultimately where wfe.NewOrder calls out to check these rate limits:

• NewOrdersPerAccount
• FailedAuthorizationsPerDomainPerAccount
• CertificatesPerDomain
• CertificatesPerFQDNSet

I believe that confirms these have all moved from the RA to the WFE. Moving on to where each rate limit has been tested in the RA:

Some of the testing is around how the limits are ordered, which is no longer relevant: we collect a batch of transactions to check these 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 the ratelimits tests cover this.

This PR now adds a test of the ARI renewal exception, specifically, which did not have existing coverage.

We could probably still use more integration tests of the rate limits, overall, but that's beyond the scope of this PR.