Enforce new orders per acct per window rate limit. by cpu · Pull Request #3501 · letsencrypt/boulder

cpu · 2018-03-01T00:04:11Z

Previously we introduced the concept of a "pending orders per account
ID" rate limit. After struggling with making an implementation of this
rate limit perform well we reevaluated the problem and decided a "new
orders per account per time window" rate limit would be a better fit for
ACMEv2 overall.

This commit introduces the new newOrdersPerAccount rate limit. The RA
now checks this before creating new pending orders in ra.NewOrder. It
does so after order reuse takes place ensuring the rate limit is only
applied in cases when a distinct new pending order row would be created.
To accomplish this a migration for a new orders field (created) and an
index over created and registrationID is added. It would be possible to
use the existing expires field for this like we've done in the past, but that
was primarily to avoid running a migration on a large table in prod. Since
we don't have that problem yet for V2 tables we can Do The Right Thing
and add a column.

For deployability the deprecated pendingOrdersPerAccount code & SA
gRPC bits are left around. A follow-up PR will be needed to remove
those (#3502).

Resolves #3410

Previously we introduced the concept of a "pending orders per account ID" rate limit. After struggling with making an implementation of this rate limit perform well we reevaluated the problem and decided a "new orders per account per time window" rate limit would be a better fit for ACMEv2 overall. This commit introduces the new `newOrdersPerAccount` rate limit. The RA now checks this before creating new pending orders in `ra.NewOrder`. It does so _after_ order reuse takes place ensuring the rate limit is only applied in cases when a distinct new pending order row would be created. For deployability the deprecated `pendingOrdersPerAccount` code & SA gRPC bits are left around. A follow-up PR will be needed to remove those.

…w-rl

jsha

Overall looks great, thanks for the quick turnaround on this! On nit: I think the SA method should be called CountOrders rather than CountNewOrders.

cpu · 2018-03-01T21:34:26Z

I think the SA method should be called CountOrders rather than CountNewOrders.

@jsha agreed. Resolved in 582789c. Thanks!

rolandshoemaker

Two small questions. Also currently there are no checks for order.Created in the various gRPC wrapper files, which is necessary for deployability but could we add a comment/open a issue that we will need to add those checks once the code has been deployed.

rolandshoemaker · 2018-03-01T22:18:14Z


-	// Check that the registration ID in question has rate limit space for another
-	// pending order
-	if err := ra.checkPendingOrderLimit(ctx, *req.RegistrationID); err != nil {


Should we not also kill off the checkPendingOrderLimit code?

Sure, that makes sense. I think I was overly generous in what I left around for the deployability guidelines.

Resolved in 000d0c9

rolandshoemaker · 2018-03-01T22:19:01Z

 	// order

+	// Check if there is rate limit space for a new order within the current window
+	if err := ra.checkNewOrdersPerAccountLimit(ctx, *order.RegistrationID); err != nil {


I know this is for an API that isn't yet prod but given if this doesn't work how we want (or has negative side effects) I kind of think this should be gated behind a feature... I could probably be convinced that isn't needed though.

I think this feedback and your feedback about killing off ra.checkPendingOrderLimit are hard to achieve together.

If I delete the ra.checkPendingOrderLimit impl in the RA (and the call from ra.NewOrder that is already deleted stays deleted), then the feature flag would make NewOrder limitless (minus the authz limits ofc) if disabled which seems more harmful overall.

I'm slightly inclined to not introduce the feature flag here.

I think it could be done (i.e. if featureEnabled { limitA } else { limitB }) but your right it would require keeping the old code around. I'm fine going forward without it to be honest, I'm just hesitant that if we ever need to back it out we'll need to essentially revert and re-deploy, which is a pain.

That said... this code looks right enough to me, so I'm happy to just dive in head first 😄.

I'm just hesitant that if we ever need to back it out we'll need to essentially revert and re-deploy, which is a pain.

That's true, but the pain of doing so only in staging is relatively minor compared to a prod revert.

For rate limits, the limit.Enabled() flag acts as a good feature flag, since Enabled() will return false if there is not entry for that limit.

cpu · 2018-03-01T22:55:41Z

@jsha @rolandshoemaker Ready for more 🔍's

rolandshoemaker · 2018-03-01T23:11:32Z

Ping on

currently there are no checks for order.Created in the various gRPC wrapper files, which is necessary for deployability but could we add a comment/open a issue that we will need to add those checks once the code has been deployed.

Otherwise looks fine.

cpu · 2018-03-01T23:28:00Z

currently there are no checks for order.Created in the various gRPC wrapper files,

Missed that feedback, thanks! Resolved in 5fa5f33

#3501 made this code deprecated. We've deployed 3501 to the staging environment and can now pull out the old cruft.

#3501 made this code deprecated. We've deployed 3501 to the staging environment and can now pull out the old cruft. Resolves #3502

Remove PendingOrdersPerAccount, which was officially deprecated in #3501 (5 years ago). Part of #5545

cpu self-assigned this Mar 1, 2018

cpu requested review from jsha and rolandshoemaker March 1, 2018 00:04

cpu requested a review from a team as a code owner March 1, 2018 00:04

Merge remote-tracking branch 'le/master' into cpu-v2-orders-per-windo…

9a7207c

…w-rl

cpu mentioned this pull request Mar 1, 2018

ACMEv2: Remove deprecated pendingOrdersPerAccount code #3502

Closed

jsha requested changes Mar 1, 2018

View reviewed changes

Rename CountNewOrders -> CountOrders

582789c

jsha previously approved these changes Mar 1, 2018

View reviewed changes

rolandshoemaker suggested changes Mar 1, 2018

View reviewed changes

Remove ra.checkPendingOrderLimit

000d0c9

cpu dismissed jsha’s stale review via 000d0c9 March 1, 2018 22:55

Check Order.Created != nil in GetOrder wrappers

5fa5f33

jsha previously approved these changes Mar 1, 2018

View reviewed changes

Fix OrderValid unit test

7459ce5

cpu dismissed jsha’s stale review via 7459ce5 March 1, 2018 23:37

jsha approved these changes Mar 1, 2018

View reviewed changes

rolandshoemaker approved these changes Mar 2, 2018

View reviewed changes

rolandshoemaker merged commit f2d3ad6 into master Mar 2, 2018

cpu deleted the cpu-v2-orders-per-window-rl branch March 2, 2018 18:48

cpu pushed a commit that referenced this pull request Mar 6, 2018

Remove deprecated sa.CountPendingOrders cruft.

641ecfc

#3501 made this code deprecated. We've deployed 3501 to the staging environment and can now pull out the old cruft.

cpu mentioned this pull request Mar 6, 2018

Remove deprecated sa.CountPendingOrders cruft. #3527

Merged

rolandshoemaker pushed a commit that referenced this pull request Mar 6, 2018

Remove deprecated sa.CountPendingOrders cruft. (#3527)

2612bf7

#3501 made this code deprecated. We've deployed 3501 to the staging environment and can now pull out the old cruft. Resolves #3502

beautifulentropy mentioned this pull request Aug 25, 2023

ratelimit: Remove deprecated PendingOrdersPerAccount #7053

Merged

beautifulentropy added a commit that referenced this pull request Aug 28, 2023

ratelimit: Remove deprecated PendingOrdersPerAccount (#7053)

c8740c0

Remove PendingOrdersPerAccount, which was officially deprecated in #3501 (5 years ago). Part of #5545

Uh oh!

Conversation

cpu commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsha left a comment

Choose a reason for hiding this comment

Uh oh!

cpu commented Mar 1, 2018

Uh oh!

rolandshoemaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpu Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpu commented Mar 1, 2018

Uh oh!

rolandshoemaker commented Mar 1, 2018

Uh oh!

cpu commented Mar 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpu commented Mar 1, 2018 •

edited

Loading

cpu Mar 1, 2018 •

edited

Loading