Merge branch 'main' into drop-tables

Author: jprenken

ratelimits/limit.go

@@ -69,13 +69,8 @@ type limit struct {
 	// precomputed to avoid doing the same calculation on every request.
 	burstOffset int64
 
-	// overrideKey is the key used to look up this limit in the overrides map.
-	overrideKey string
-}
-
-// isOverride returns true if the limit is an override.
-func (l *limit) isOverride() bool {
-	return l.overrideKey != ""
+	// isOverride is true if the limit is an override.
+	isOverride bool
 }
 
 // precompute calculates the emissionInterval and burstOffset for the limit.
@@ -178,11 +173,13 @@ func parseOverrideLimits(newOverridesYAML overridesYAML) (limits, error) {
 			}
 
 			lim := &limit{
-				burst:  v.Burst,
-				count:  v.Count,
-				period: v.Period,
-				name:   name,
+				burst:      v.Burst,
+				count:      v.Count,
+				period:     v.Period,
+				name:       name,
+				isOverride: true,
 			}
+			lim.precompute()
 
 			err := validateLimit(lim)
 			if err != nil {
@@ -196,14 +193,12 @@ func parseOverrideLimits(newOverridesYAML overridesYAML) (limits, error) {
 					return nil, fmt.Errorf(
 						"validating name %s and id %q for override limit %q: %w", name, id, k, err)
 				}
-				lim.overrideKey = joinWithColon(name.EnumString(), id)
 				if name == CertificatesPerFQDNSet {
 					// FQDNSet hashes are not a nice thing to ask for in a
 					// config file, so we allow the user to specify a
 					// comma-separated list of FQDNs and compute the hash here.
 					id = fmt.Sprintf("%x", core.HashNames(strings.Split(id, ",")))
 				}
-				lim.precompute()
 				parsed[joinWithColon(name.EnumString(), id)] = lim
 			}
 		}

ratelimits/limiter.go

@@ -37,8 +37,7 @@ type Limiter struct {
 	source Source
 	clk    clock.Clock
 
-	spendLatency       *prometheus.HistogramVec
-	overrideUsageGauge *prometheus.GaugeVec
+	spendLatency *prometheus.HistogramVec
 }
 
 // NewLimiter returns a new *Limiter. The provided source must be safe for
@@ -52,17 +51,10 @@ func NewLimiter(clk clock.Clock, source Source, stats prometheus.Registerer) (*L
 	}, []string{"limit", "decision"})
 	stats.MustRegister(spendLatency)
 
-	overrideUsageGauge := prometheus.NewGaugeVec(prometheus.GaugeOpts{
-		Name: "ratelimits_override_usage",
-		Help: "Proportion of override limit used, by limit name and bucket key.",
-	}, []string{"limit", "bucket_key"})
-	stats.MustRegister(overrideUsageGauge)
-
 	return &Limiter{
-		source:             source,
-		clk:                clk,
-		spendLatency:       spendLatency,
-		overrideUsageGauge: overrideUsageGauge,
+		source:       source,
+		clk:          clk,
+		spendLatency: spendLatency,
 	}, nil
 }
 
@@ -284,11 +276,6 @@ func (l *Limiter) BatchSpend(ctx context.Context, txns []Transaction) (*Decision
 		storedTAT, bucketExists := tats[txn.bucketKey]
 		d := maybeSpend(l.clk, txn, storedTAT)
 
-		if txn.limit.isOverride() {
-			utilization := float64(txn.limit.burst-d.remaining) / float64(txn.limit.burst)
-			l.overrideUsageGauge.WithLabelValues(txn.limit.name.String(), txn.limit.overrideKey).Set(utilization)
-		}
-
 		if d.allowed && (storedTAT != d.newTAT) && txn.spend {
 			if !bucketExists {
 				newBuckets[txn.bucketKey] = d.newTAT

ratelimits/limiter_test.go

@@ -8,7 +8,6 @@ import (
 	"time"
 
 	"github.com/jmhodges/clock"
-	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/letsencrypt/boulder/config"
 	berrors "github.com/letsencrypt/boulder/errors"
@@ -60,12 +59,6 @@ func TestLimiter_CheckWithLimitOverrides(t *testing.T) {
 	testCtx, limiters, txnBuilder, clk, testIP := setup(t)
 	for name, l := range limiters {
 		t.Run(name, func(t *testing.T) {
-			// Verify our overrideUsageGauge is being set correctly. 0.0 == 0%
-			// of the bucket has been consumed.
-			test.AssertMetricWithLabelsEquals(t, l.overrideUsageGauge, prometheus.Labels{
-				"limit":      NewRegistrationsPerIPAddress.String(),
-				"bucket_key": joinWithColon(NewRegistrationsPerIPAddress.EnumString(), tenZeroZeroTwo)}, 0)
-
 			overriddenBucketKey, err := newIPAddressBucketKey(NewRegistrationsPerIPAddress, net.ParseIP(tenZeroZeroTwo))
 			test.AssertNotError(t, err, "should not error")
 			overriddenLimit, err := txnBuilder.getLimit(NewRegistrationsPerIPAddress, overriddenBucketKey)
@@ -87,12 +80,6 @@ func TestLimiter_CheckWithLimitOverrides(t *testing.T) {
 			test.AssertEquals(t, d.remaining, int64(0))
 			test.AssertEquals(t, d.resetIn, time.Second)
 
-			// Verify our overrideUsageGauge is being set correctly. 1.0 == 100%
-			// of the bucket has been consumed.
-			test.AssertMetricWithLabelsEquals(t, l.overrideUsageGauge, prometheus.Labels{
-				"limit_name": NewRegistrationsPerIPAddress.String(),
-				"bucket_key": joinWithColon(NewRegistrationsPerIPAddress.EnumString(), tenZeroZeroTwo)}, 1.0)
-
 			// Verify our RetryIn is correct. 1 second == 1000 milliseconds and
 			// 1000/40 = 25 milliseconds per request.
 			test.AssertEquals(t, d.retryIn, time.Millisecond*25)

ratelimits/source_redis.go

@@ -99,10 +99,6 @@ func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time
 	}
 
 	totalLatency := r.clk.Since(start)
-	perSetLatency := totalLatency / time.Duration(len(buckets))
-	for range buckets {
-		r.observeLatency("batchset_entry", perSetLatency, nil)
-	}
 
 	r.observeLatency("batchset", totalLatency, nil)
 	return nil
@@ -128,17 +124,14 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin
 
 	alreadyExists := make(map[string]bool, len(buckets))
 	totalLatency := r.clk.Since(start)
-	perSetLatency := totalLatency / time.Duration(len(buckets))
 	for bucketKey, cmd := range cmds {
 		success, err := cmd.Result()
 		if err != nil {
-			r.observeLatency("batchsetnotexisting_entry", perSetLatency, err)
 			return nil, err
 		}
 		if !success {
 			alreadyExists[bucketKey] = true
 		}
-		r.observeLatency("batchsetnotexisting_entry", perSetLatency, nil)
 	}
 
 	r.observeLatency("batchsetnotexisting", totalLatency, nil)
@@ -163,11 +156,6 @@ func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]inc
 	}
 
 	totalLatency := r.clk.Since(start)
-	perSetLatency := totalLatency / time.Duration(len(buckets))
-	for range buckets {
-		r.observeLatency("batchincrby_entry", perSetLatency, nil)
-	}
-
 	r.observeLatency("batchincrby", totalLatency, nil)
 	return nil
 }
@@ -211,7 +199,6 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st
 	}
 
 	totalLatency := r.clk.Since(start)
-	perEntryLatency := totalLatency / time.Duration(len(bucketKeys))
 
 	tats := make(map[string]time.Time, len(bucketKeys))
 	notFoundCount := 0
@@ -224,13 +211,10 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st
 				r.observeLatency("batchget", r.clk.Since(start), err)
 				return nil, err
 			}
-			// Bucket key does not exist.
-			r.observeLatency("batchget_entry", perEntryLatency, err)
 			notFoundCount++
 			continue
 		}
 		tats[bucketKeys[i]] = time.Unix(0, tatNano).UTC()
-		r.observeLatency("batchget_entry", perEntryLatency, nil)
 	}
 
 	var batchErr error

ratelimits/transaction.go

@@ -403,7 +403,7 @@ func (builder *TransactionBuilder) certificatesPerDomainCheckOnlyTransactions(re
 			return nil, err
 		}
 		if accountOverride {
-			if !perAccountLimit.isOverride() {
+			if !perAccountLimit.isOverride {
 				return nil, fmt.Errorf("shouldn't happen: CertificatesPerDomainPerAccount limit is not an override")
 			}
 			perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name)
@@ -481,7 +481,7 @@ func (builder *TransactionBuilder) CertificatesPerDomainSpendOnlyTransactions(re
 			return nil, err
 		}
 		if accountOverride {
-			if !perAccountLimit.isOverride() {
+			if !perAccountLimit.isOverride {
 				return nil, fmt.Errorf("shouldn't happen: CertificatesPerDomainPerAccount limit is not an override")
 			}
 			perAccountPerDomainKey, err := NewRegIdDomainBucketKey(CertificatesPerDomainPerAccount, regId, name)

ratelimits/transaction_test.go

@@ -79,29 +79,29 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) {
 	test.AssertEquals(t, len(txns), 1)
 	test.AssertEquals(t, txns[0].bucketKey, "4:123456789:so.many.labels.here.example.com")
 	test.Assert(t, txns[0].checkOnly(), "should be check-only")
-	test.Assert(t, !txns[0].limit.isOverride(), "should not be an override")
+	test.Assert(t, !txns[0].limit.isOverride, "should not be an override")
 
 	// A spend-only transaction for the default per-account limit.
 	txn, err := tb.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(123456789, "so.many.labels.here.example.com")
 	test.AssertNotError(t, err, "creating transaction")
 	test.AssertEquals(t, txn.bucketKey, "4:123456789:so.many.labels.here.example.com")
 	test.Assert(t, txn.spendOnly(), "should be spend-only")
-	test.Assert(t, !txn.limit.isOverride(), "should not be an override")
+	test.Assert(t, !txn.limit.isOverride, "should not be an override")
 
 	// A check-only transaction for the per-account limit override.
 	txns, err = tb.FailedAuthorizationsPerDomainPerAccountCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com"})
 	test.AssertNotError(t, err, "creating transactions")
 	test.AssertEquals(t, len(txns), 1)
 	test.AssertEquals(t, txns[0].bucketKey, "4:13371338:so.many.labels.here.example.com")
 	test.Assert(t, txns[0].checkOnly(), "should be check-only")
-	test.Assert(t, txns[0].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[0].limit.isOverride, "should be an override")
 
 	// A spend-only transaction for the per-account limit override.
 	txn, err = tb.FailedAuthorizationsPerDomainPerAccountSpendOnlyTransaction(13371338, "so.many.labels.here.example.com")
 	test.AssertNotError(t, err, "creating transaction")
 	test.AssertEquals(t, txn.bucketKey, "4:13371338:so.many.labels.here.example.com")
 	test.Assert(t, txn.spendOnly(), "should be spend-only")
-	test.Assert(t, txn.limit.isOverride(), "should be an override")
+	test.Assert(t, txn.limit.isOverride, "should be an override")
 }
 
 func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testing.T) {
@@ -115,7 +115,7 @@ func TestFailedAuthorizationsForPausingPerDomainPerAccountTransactions(t *testin
 	test.AssertNotError(t, err, "creating transaction")
 	test.AssertEquals(t, txn.bucketKey, "8:13371338:so.many.labels.here.example.com")
 	test.Assert(t, txn.check && txn.spend, "should be check and spend")
-	test.Assert(t, txn.limit.isOverride(), "should be an override")
+	test.Assert(t, txn.limit.isOverride, "should be an override")
 }
 
 func TestCertificatesPerDomainTransactions(t *testing.T) {
@@ -153,15 +153,15 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) {
 	test.AssertEquals(t, len(txns), 1)
 	test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com")
 	test.Assert(t, txns[0].checkOnly(), "should be check-only")
-	test.Assert(t, txns[0].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[0].limit.isOverride, "should be an override")
 
 	// Same as above, but with multiple example.com domains.
 	txns, err = tb.certificatesPerDomainCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com", "z.example.com"})
 	test.AssertNotError(t, err, "creating transactions")
 	test.AssertEquals(t, len(txns), 1)
 	test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com")
 	test.Assert(t, txns[0].checkOnly(), "should be check-only")
-	test.Assert(t, txns[0].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[0].limit.isOverride, "should be an override")
 
 	// Same as above, but with different domains.
 	txns, err = tb.certificatesPerDomainCheckOnlyTransactions(13371338, []string{"so.many.labels.here.example.com", "z.example.net"})
@@ -170,10 +170,10 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) {
 	test.AssertEquals(t, len(txns), 2)
 	test.AssertEquals(t, txns[0].bucketKey, "6:13371338:example.com")
 	test.Assert(t, txns[0].checkOnly(), "should be check-only")
-	test.Assert(t, txns[0].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[0].limit.isOverride, "should be an override")
 	test.AssertEquals(t, txns[1].bucketKey, "6:13371338:example.net")
 	test.Assert(t, txns[1].checkOnly(), "should be check-only")
-	test.Assert(t, txns[1].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[1].limit.isOverride, "should be an override")
 
 	// Two spend-only transactions, one for the global limit and one for the
 	// per-account limit override.
@@ -183,11 +183,11 @@ func TestCertificatesPerDomainPerAccountTransactions(t *testing.T) {
 	txns = sortTransactions(txns)
 	test.AssertEquals(t, txns[0].bucketKey, "5:example.com")
 	test.Assert(t, txns[0].spendOnly(), "should be spend-only")
-	test.Assert(t, !txns[0].limit.isOverride(), "should not be an override")
+	test.Assert(t, !txns[0].limit.isOverride, "should not be an override")
 
 	test.AssertEquals(t, txns[1].bucketKey, "6:13371338:example.com")
 	test.Assert(t, txns[1].spendOnly(), "should be spend-only")
-	test.Assert(t, txns[1].limit.isOverride(), "should be an override")
+	test.Assert(t, txns[1].limit.isOverride, "should be an override")
 }
 
 func TestCertificatesPerFQDNSetTransactions(t *testing.T) {
@@ -202,7 +202,7 @@ func TestCertificatesPerFQDNSetTransactions(t *testing.T) {
 	namesHash := fmt.Sprintf("%x", core.HashNames([]string{"example.com", "example.net", "example.org"}))
 	test.AssertEquals(t, txn.bucketKey, "7:"+namesHash)
 	test.Assert(t, txn.checkOnly(), "should be check-only")
-	test.Assert(t, !txn.limit.isOverride(), "should not be an override")
+	test.Assert(t, !txn.limit.isOverride, "should not be an override")
 }
 
 func TestNewTransactionBuilder(t *testing.T) {

web/context.go

@@ -35,7 +35,12 @@ type RequestEvent struct {
 	Slug           string   `json:",omitempty"`
 	InternalErrors []string `json:",omitempty"`
 	Error          string   `json:",omitempty"`
-	UserAgent      string   `json:"ua,omitempty"`
+	// If there is an error checking the data store for our rate limits
+	// we ignore it, but attach the error to the log event for analysis.
+	// TODO(#7796): Treat errors from the rate limit system as normal
+	// errors and put them into InternalErrors.
+	IgnoredRateLimitError string `json:",omitempty"`
+	UserAgent             string `json:"ua,omitempty"`
 	// Origin is sent by the browser from XHR-based clients.
 	Origin string                 `json:",omitempty"`
 	Extra  map[string]interface{} `json:",omitempty"`

wfe2/wfe.go

@@ -784,8 +784,7 @@ func (wfe *WebFrontEndImpl) NewAccount(
 			wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err)
 			return
 		} else {
-			wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While checking rate limits"), err)
-			return
+			logEvent.IgnoredRateLimitError = err.Error()
 		}
 	}
 
@@ -2352,7 +2351,7 @@ func (wfe *WebFrontEndImpl) NewOrder(
 				wfe.sendError(response, logEvent, probs.RateLimited(err.Error()), err)
 				return
 			} else {
-				wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While checking rate limits"), err)
+				logEvent.IgnoredRateLimitError = err.Error()
 				return
 			}
 		}