Always use INCRBY for redis rate limits (#7856)

Deprecate the IncrementRateLimits feature flag, and always use the redis
INCRBY instruction to update rate limit TATs.

Fixes #7855

Author: aarongable

features/features.go

@@ -15,6 +15,9 @@ import (
 // then call features.Set(parsedConfig) to load the parsed struct into this
 // package's global Config.
 type Config struct {
+	// Deprecated flags.
+	IncrementRateLimits bool
+
 	// ServeRenewalInfo exposes the renewalInfo endpoint in the directory and for
 	// GET requests. WARNING: This feature is a draft and highly unstable.
 	ServeRenewalInfo bool
@@ -99,11 +102,6 @@ type Config struct {
 	// and pause issuance for each (account, hostname) pair that repeatedly
 	// fails validation.
 	AutomaticallyPauseZombieClients bool
-
-	// IncrementRateLimits uses Redis' IncrBy, instead of Set, for rate limit
-	// accounting. This catches and denies spikes of requests much more
-	// reliably.
-	IncrementRateLimits bool
 }
 
 var fMu = new(sync.RWMutex)

ratelimits/limiter.go

@@ -14,7 +14,6 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 
 	berrors "github.com/letsencrypt/boulder/errors"
-	"github.com/letsencrypt/boulder/features"
 )
 
 const (
@@ -276,7 +275,6 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision
 		return nil, err
 	}
 	batchDecision := allowedDecision
-	newTATs := make(map[string]time.Time)
 	newBuckets := make(map[string]time.Time)
 	incrBuckets := make(map[string]increment)
 	txnOutcomes := make(map[Transaction]string)
@@ -297,8 +295,6 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision
 
 		if d.allowed && (tat != d.newTAT) && txn.spend {
 			// New bucket state should be persisted.
-			newTATs[txn.bucketKey] = d.newTAT
-
 			if bucketExists {
 				incrBuckets[txn.bucketKey] = increment{
 					cost: time.Duration(txn.cost * txn.limit.emissionInterval),
@@ -321,25 +317,16 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision
 		}
 	}
 
-	if features.Get().IncrementRateLimits {
-		if batchDecision.allowed {
-			if len(newBuckets) > 0 {
-				err = l.source.BatchSet(ctx, newBuckets)
-				if err != nil {
-					return nil, err
-				}
-			}
-
-			if len(incrBuckets) > 0 {
-				err = l.source.BatchIncrement(ctx, incrBuckets)
-				if err != nil {
-					return nil, err
-				}
+	if batchDecision.allowed {
+		if len(newBuckets) > 0 {
+			err = l.source.BatchSet(ctx, newBuckets)
+			if err != nil {
+				return nil, err
 			}
 		}
-	} else {
-		if batchDecision.allowed && len(newTATs) > 0 {
-			err = l.source.BatchSet(ctx, newTATs)
+
+		if len(incrBuckets) > 0 {
+			err = l.source.BatchIncrement(ctx, incrBuckets)
 			if err != nil {
 				return nil, err
 			}
@@ -396,7 +383,6 @@ func (l *Limiter) BatchRefund(ctx context.Context, txns []Transaction) (*Decisio
 	}
 
 	batchDecision := allowedDecision
-	newTATs := make(map[string]time.Time)
 	incrBuckets := make(map[string]increment)
 
 	for _, txn := range batch {
@@ -414,27 +400,17 @@ func (l *Limiter) BatchRefund(ctx context.Context, txns []Transaction) (*Decisio
 		batchDecision = stricter(batchDecision, d)
 		if d.allowed && tat != d.newTAT {
 			// New bucket state should be persisted.
-			newTATs[txn.bucketKey] = d.newTAT
 			incrBuckets[txn.bucketKey] = increment{
 				cost: time.Duration(-txn.cost * txn.limit.emissionInterval),
 				ttl:  time.Duration(txn.limit.burstOffset),
 			}
 		}
 	}
 
-	if features.Get().IncrementRateLimits {
-		if len(incrBuckets) > 0 {
-			err = l.source.BatchIncrement(ctx, incrBuckets)
-			if err != nil {
-				return nil, err
-			}
-		}
-	} else {
-		if len(newTATs) > 0 {
-			err = l.source.BatchSet(ctx, newTATs)
-			if err != nil {
-				return nil, err
-			}
+	if len(incrBuckets) > 0 {
+		err = l.source.BatchIncrement(ctx, incrBuckets)
+		if err != nil {
+			return nil, err
 		}
 	}
 	return batchDecision, nil

ratelimits/limiter_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"math/rand/v2"
 	"net"
-	"os"
 	"testing"
 	"time"
 
@@ -13,7 +12,6 @@ import (
 
 	"github.com/letsencrypt/boulder/config"
 	berrors "github.com/letsencrypt/boulder/errors"
-	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/test"
 )
@@ -40,19 +38,6 @@ func newTestTransactionBuilder(t *testing.T) *TransactionBuilder {
 }
 
 func setup(t *testing.T) (context.Context, map[string]*Limiter, *TransactionBuilder, clock.FakeClock, string) {
-	// Because all test cases in this file are affected by this feature flag, we
-	// want to run them all both with and without the feature flag. This way, we
-	// get one set of runs with and one set without. It's difficult to defer
-	// features.Reset() from the setup func (these tests are parallel); as long
-	// as this code doesn't test any other features, we don't need to.
-	//
-	// N.b. This is fragile. If a test case does call features.Reset(), it will
-	// not be testing the intended code path. But we expect to clean this up
-	// quickly.
-	if os.Getenv("BOULDER_CONFIG_DIR") == "test/config-next" {
-		features.Set(features.Config{IncrementRateLimits: true})
-	}
-
 	testCtx := context.Background()
 	clk := clock.NewFake()
 

test/config-next/ra.json

@@ -128,8 +128,7 @@
 		"features": {
 			"AsyncFinalize": true,
 			"UseKvLimitsForNewOrder": true,
-			"AutomaticallyPauseZombieClients": true,
-			"IncrementRateLimits": true
+			"AutomaticallyPauseZombieClients": true
 		},
 		"ctLogs": {
 			"stagger": "500ms",

test/config-next/wfe2.json

@@ -128,8 +128,7 @@
 			"PropagateCancels": true,
 			"ServeRenewalInfo": true,
 			"CheckIdentifiersPaused": true,
-			"UseKvLimitsForNewOrder": true,
-			"IncrementRateLimits": true
+			"UseKvLimitsForNewOrder": true
 		},
 		"certProfiles": {
 			"legacy": "The normal profile you know and love",

test/config/ra.json

@@ -104,6 +104,9 @@
 				}
 			}
 		},
+		"features": {
+			"IncrementRateLimits": true
+		},
 		"ctLogs": {
 			"stagger": "500ms",
 			"logListFile": "test/ct-test-srv/log_list.json",

test/config/wfe2.json

@@ -103,7 +103,8 @@
 		"authorizationLifetimeDays": 30,
 		"pendingAuthorizationLifetimeDays": 7,
 		"features": {
-			"ServeRenewalInfo": true
+			"ServeRenewalInfo": true,
+			"IncrementRateLimits": true
 		}
 	},
 	"syslog": {