ratelimits: Provide verbose user-facing rate limit errors (letsencryp…

…t#7653) - Instruct callers to call *Decision.Result() to check the result of rate limit transactions - Preserve the Transaction within the resulting *Decision - Generate consistently formatted verbose errors using the metadata found in the *Decision - Fix broken key-value rate limits integration test in TestDuplicateFQDNRateLimit Fixes letsencrypt#7577
Embdsys · Aug 12, 2024 · 6a3e9d7 · 6a3e9d7
1 parent 9e28691
commit 6a3e9d7
Show file tree

Hide file tree

Showing 8 changed files with 551 additions and 340 deletions.
diff --git a/ra/ra_test.go b/ra/ra_test.go
@@ -874,19 +874,6 @@ func TestPerformValidationSuccess(t *testing.T) {
 		Problems: nil,
 	}
 
-	var remainingFailedValidations int64
-	var rlTxns []ratelimits.Transaction
-	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		// Gather a baseline for the rate limit.
-		var err error
-		rlTxns, err = ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(authzPB.RegistrationID, []string{Identifier}, 100)
-		test.AssertNotError(t, err, "FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions failed")
-
-		d, err := ra.limiter.BatchSpend(ctx, rlTxns)
-		test.AssertNotError(t, err, "BatchSpend failed")
-		remainingFailedValidations = d.Remaining
-	}
-
 	now := fc.Now()
 	challIdx := dnsChallIdx(t, authzPB.Challenges)
 	authzPB, err := ra.PerformValidation(ctx, &rapb.PerformValidationRequest{
@@ -928,13 +915,6 @@ func TestPerformValidationSuccess(t *testing.T) {
 	// Check that validated timestamp was recorded, stored, and retrieved
 	expectedValidated := fc.Now()
 	test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing")
-
-	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		// The failed validations bucket should be identical to the baseline.
-		d, err := ra.limiter.BatchSpend(ctx, rlTxns)
-		test.AssertNotError(t, err, "BatchSpend failed")
-		test.AssertEquals(t, d.Remaining, remainingFailedValidations)
-	}
 }
 
 func TestPerformValidationVAError(t *testing.T) {
@@ -943,19 +923,6 @@ func TestPerformValidationVAError(t *testing.T) {
 
 	authzPB := createPendingAuthorization(t, sa, Identifier, fc.Now().Add(12*time.Hour))
 
-	var remainingFailedValidations int64
-	var rlTxns []ratelimits.Transaction
-	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		// Gather a baseline for the rate limit.
-		var err error
-		rlTxns, err = ra.txnBuilder.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(authzPB.RegistrationID, []string{Identifier}, 100)
-		test.AssertNotError(t, err, "FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions failed")
-
-		d, err := ra.limiter.BatchSpend(ctx, rlTxns)
-		test.AssertNotError(t, err, "BatchSpend failed")
-		remainingFailedValidations = d.Remaining
-	}
-
 	va.PerformValidationRequestResultError = fmt.Errorf("Something went wrong")
 
 	challIdx := dnsChallIdx(t, authzPB.Challenges)
@@ -995,13 +962,6 @@ func TestPerformValidationVAError(t *testing.T) {
 	// Check that validated timestamp was recorded, stored, and retrieved
 	expectedValidated := fc.Now()
 	test.Assert(t, *challenge.Validated == expectedValidated, "Validated timestamp incorrect or missing")
-
-	if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		// The failed validations bucket should have been decremented by 1.
-		d, err := ra.limiter.BatchSpend(ctx, rlTxns)
-		test.AssertNotError(t, err, "BatchSpend failed")
-		test.AssertEquals(t, d.Remaining, remainingFailedValidations-1)
-	}
 }
 
 func TestCertificateKeyNotEqualAccountKey(t *testing.T) {

diff --git a/ratelimits/gcra.go b/ratelimits/gcra.go
@@ -9,8 +9,8 @@ import (
 // maybeSpend uses the GCRA algorithm to decide whether to allow a request. It
 // returns a Decision struct with the result of the decision and the updated
 // TAT. The cost must be 0 or greater and <= the burst capacity of the limit.
-func maybeSpend(clk clock.Clock, rl limit, tat time.Time, cost int64) *Decision {
-	if cost < 0 || cost > rl.Burst {
+func maybeSpend(clk clock.Clock, txn Transaction, tat time.Time) *Decision {
+	if txn.cost < 0 || txn.cost > txn.limit.Burst {
 		// The condition above is the union of the conditions checked in Check
 		// and Spend methods of Limiter. If this panic is reached, it means that
 		// the caller has introduced a bug.
@@ -27,45 +27,47 @@ func maybeSpend(clk clock.Clock, rl limit, tat time.Time, cost int64) *Decision
 	}
 
 	// Compute the cost increment.
-	costIncrement := rl.emissionInterval * cost
+	costIncrement := txn.limit.emissionInterval * txn.cost
 
 	// Deduct the cost to find the new TAT and residual capacity.
 	newTAT := tatUnix + costIncrement
-	difference := nowUnix - (newTAT - rl.burstOffset)
+	difference := nowUnix - (newTAT - txn.limit.burstOffset)
 
 	if difference < 0 {
 		// Too little capacity to satisfy the cost, deny the request.
-		residual := (nowUnix - (tatUnix - rl.burstOffset)) / rl.emissionInterval
+		residual := (nowUnix - (tatUnix - txn.limit.burstOffset)) / txn.limit.emissionInterval
 		return &Decision{
-			Allowed:   false,
-			Remaining: residual,
-			RetryIn:   -time.Duration(difference),
-			ResetIn:   time.Duration(tatUnix - nowUnix),
-			newTAT:    time.Unix(0, tatUnix).UTC(),
+			allowed:     false,
+			remaining:   residual,
+			retryIn:     -time.Duration(difference),
+			resetIn:     time.Duration(tatUnix - nowUnix),
+			newTAT:      time.Unix(0, tatUnix).UTC(),
+			transaction: txn,
 		}
 	}
 
 	// There is enough capacity to satisfy the cost, allow the request.
 	var retryIn time.Duration
-	residual := difference / rl.emissionInterval
+	residual := difference / txn.limit.emissionInterval
 	if difference < costIncrement {
 		retryIn = time.Duration(costIncrement - difference)
 	}
 	return &Decision{
-		Allowed:   true,
-		Remaining: residual,
-		RetryIn:   retryIn,
-		ResetIn:   time.Duration(newTAT - nowUnix),
-		newTAT:    time.Unix(0, newTAT).UTC(),
+		allowed:     true,
+		remaining:   residual,
+		retryIn:     retryIn,
+		resetIn:     time.Duration(newTAT - nowUnix),
+		newTAT:      time.Unix(0, newTAT).UTC(),
+		transaction: txn,
 	}
 }
 
 // maybeRefund uses the Generic Cell Rate Algorithm (GCRA) to attempt to refund
 // the cost of a request which was previously spent. The refund cost must be 0
 // or greater. A cost will only be refunded up to the burst capacity of the
 // limit. A partial refund is still considered successful.
-func maybeRefund(clk clock.Clock, rl limit, tat time.Time, cost int64) *Decision {
-	if cost < 0 || cost > rl.Burst {
+func maybeRefund(clk clock.Clock, txn Transaction, tat time.Time) *Decision {
+	if txn.cost < 0 || txn.cost > txn.limit.Burst {
 		// The condition above is checked in the Refund method of Limiter. If
 		// this panic is reached, it means that the caller has introduced a bug.
 		panic("invalid cost for maybeRefund")
@@ -77,16 +79,17 @@ func maybeRefund(clk clock.Clock, rl limit, tat time.Time, cost int64) *Decision
 	if nowUnix > tatUnix {
 		// The TAT is in the past, therefore the bucket is full.
 		return &Decision{
-			Allowed:   false,
-			Remaining: rl.Burst,
-			RetryIn:   time.Duration(0),
-			ResetIn:   time.Duration(0),
-			newTAT:    tat,
+			allowed:     false,
+			remaining:   txn.limit.Burst,
+			retryIn:     time.Duration(0),
+			resetIn:     time.Duration(0),
+			newTAT:      tat,
+			transaction: txn,
 		}
 	}
 
 	// Compute the refund increment.
-	refundIncrement := rl.emissionInterval * cost
+	refundIncrement := txn.limit.emissionInterval * txn.cost
 
 	// Subtract the refund increment from the TAT to find the new TAT.
 	newTAT := tatUnix - refundIncrement
@@ -97,14 +100,15 @@ func maybeRefund(clk clock.Clock, rl limit, tat time.Time, cost int64) *Decision
 	}
 
 	// Calculate the new capacity.
-	difference := nowUnix - (newTAT - rl.burstOffset)
-	residual := difference / rl.emissionInterval
+	difference := nowUnix - (newTAT - txn.limit.burstOffset)
+	residual := difference / txn.limit.emissionInterval
 
 	return &Decision{
-		Allowed:   (newTAT != tatUnix),
-		Remaining: residual,
-		RetryIn:   time.Duration(0),
-		ResetIn:   time.Duration(newTAT - nowUnix),
-		newTAT:    time.Unix(0, newTAT).UTC(),
+		allowed:     (newTAT != tatUnix),
+		remaining:   residual,
+		retryIn:     time.Duration(0),
+		resetIn:     time.Duration(newTAT - nowUnix),
+		newTAT:      time.Unix(0, newTAT).UTC(),
+		transaction: txn,
 	}
 }