From 092f4ecfed1900d1efec5ede87c36265853b01d8 Mon Sep 17 00:00:00 2001 From: Samantha Date: Wed, 18 Dec 2024 15:43:38 -0500 Subject: [PATCH 1/5] ratelimits: Remove a couple metrics that we're not finding useful --- ratelimits/limiter.go | 21 ++---- ratelimits/limiter_test.go | 13 ---- ratelimits/source_redis.go | 129 ++----------------------------------- 3 files changed, 8 insertions(+), 155 deletions(-) diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 0654787b6ec..1dd42837120 100644 --- a/ratelimits/limiter.go +++ b/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 diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index cf18fe271f5..97115277010 100644 --- a/ratelimits/limiter_test.go +++ b/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) diff --git a/ratelimits/source_redis.go b/ratelimits/source_redis.go index 4d32f7c2a6d..f13f4c57262 100644 --- a/ratelimits/source_redis.go +++ b/ratelimits/source_redis.go @@ -3,7 +3,6 @@ package ratelimits import ( "context" "errors" - "net" "time" "github.com/jmhodges/clock" @@ -16,76 +15,22 @@ var _ Source = (*RedisSource)(nil) // RedisSource is a ratelimits source backed by sharded Redis. type RedisSource struct { - client *redis.Ring - clk clock.Clock - latency *prometheus.HistogramVec + client *redis.Ring + clk clock.Clock } // NewRedisSource returns a new Redis backed source using the provided // *redis.Ring client. func NewRedisSource(client *redis.Ring, clk clock.Clock, stats prometheus.Registerer) *RedisSource { - latency := prometheus.NewHistogramVec( - prometheus.HistogramOpts{ - Name: "ratelimits_latency", - Help: "Histogram of Redis call latencies labeled by call=[set|get|delete|ping] and result=[success|error]", - // Exponential buckets ranging from 0.0005s to 3s. - Buckets: prometheus.ExponentialBucketsRange(0.0005, 3, 8), - }, - []string{"call", "result"}, - ) - stats.MustRegister(latency) - return &RedisSource{ - client: client, - clk: clk, - latency: latency, - } -} - -var errMixedSuccess = errors.New("some keys not found") - -// resultForError returns a string representing the result of the operation -// based on the provided error. -func resultForError(err error) string { - if errors.Is(errMixedSuccess, err) { - // Indicates that some of the keys in a batchset operation were not found. - return "mixedSuccess" - } else if errors.Is(redis.Nil, err) { - // Bucket key does not exist. - return "notFound" - } else if errors.Is(err, context.DeadlineExceeded) { - // Client read or write deadline exceeded. - return "deadlineExceeded" - } else if errors.Is(err, context.Canceled) { - // Caller canceled the operation. - return "canceled" - } - var netErr net.Error - if errors.As(err, &netErr) && netErr.Timeout() { - // Dialer timed out connecting to Redis. - return "timeout" - } - var redisErr redis.Error - if errors.Is(err, redisErr) { - // An internal error was returned by the Redis server. - return "redisError" - } - return "failed" -} - -func (r *RedisSource) observeLatency(call string, latency time.Duration, err error) { - result := "success" - if err != nil { - result = resultForError(err) + client: client, + clk: clk, } - r.latency.With(prometheus.Labels{"call": call, "result": result}).Observe(latency.Seconds()) } // BatchSet stores TATs at the specified bucketKeys using a pipelined Redis // Transaction in order to reduce the number of round-trips to each Redis shard. func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time) error { - start := r.clk.Now() - pipeline := r.client.Pipeline() for bucketKey, tat := range buckets { // Set a TTL of TAT + 10 minutes to account for clock skew. @@ -94,25 +39,14 @@ func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time } _, err := pipeline.Exec(ctx) if err != nil { - r.observeLatency("batchset", r.clk.Since(start), err) return err } - - 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 } // BatchSetNotExisting attempts to set TATs for the specified bucketKeys if they // do not already exist. Returns a map indicating which keys already existed. func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[string]time.Time) (map[string]bool, error) { - start := r.clk.Now() - pipeline := r.client.Pipeline() cmds := make(map[string]*redis.BoolCmd, len(buckets)) for bucketKey, tat := range buckets { @@ -122,26 +56,19 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin } _, err := pipeline.Exec(ctx) if err != nil { - r.observeLatency("batchsetnotexisting", r.clk.Since(start), err) return nil, err } 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) return alreadyExists, nil } @@ -149,8 +76,6 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin // Redis Transaction in order to reduce the number of round-trips to each Redis // shard. func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]increment) error { - start := r.clk.Now() - pipeline := r.client.Pipeline() for bucketKey, incr := range buckets { pipeline.IncrBy(ctx, bucketKey, incr.cost.Nanoseconds()) @@ -158,38 +83,22 @@ func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]inc } _, err := pipeline.Exec(ctx) if err != nil { - r.observeLatency("batchincrby", r.clk.Since(start), err) return err } - - 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 } // Get retrieves the TAT at the specified bucketKey. If the bucketKey does not // exist, ErrBucketNotFound is returned. func (r *RedisSource) Get(ctx context.Context, bucketKey string) (time.Time, error) { - start := r.clk.Now() - tatNano, err := r.client.Get(ctx, bucketKey).Int64() if err != nil { if errors.Is(err, redis.Nil) { // Bucket key does not exist. - r.observeLatency("get", r.clk.Since(start), err) return time.Time{}, ErrBucketNotFound } - // An error occurred while retrieving the TAT. - r.observeLatency("get", r.clk.Since(start), err) return time.Time{}, err } - - r.observeLatency("get", r.clk.Since(start), nil) return time.Unix(0, tatNano).UTC(), nil } @@ -198,21 +107,15 @@ func (r *RedisSource) Get(ctx context.Context, bucketKey string) (time.Time, err // shard. If a bucketKey does not exist, it WILL NOT be included in the returned // map. func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[string]time.Time, error) { - start := r.clk.Now() - pipeline := r.client.Pipeline() for _, bucketKey := range bucketKeys { pipeline.Get(ctx, bucketKey) } results, err := pipeline.Exec(ctx) if err != nil && !errors.Is(err, redis.Nil) { - r.observeLatency("batchget", r.clk.Since(start), err) return nil, err } - totalLatency := r.clk.Since(start) - perEntryLatency := totalLatency / time.Duration(len(bucketKeys)) - tats := make(map[string]time.Time, len(bucketKeys)) notFoundCount := 0 for i, result := range results { @@ -221,59 +124,35 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st if !errors.Is(err, redis.Nil) { // This should never happen as any errors should have been // caught after the pipeline.Exec() call. - 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 - if notFoundCount < len(results) { - // Some keys were not found. - batchErr = errMixedSuccess - } else if notFoundCount == len(results) { - // All keys were not found. - batchErr = redis.Nil } - - r.observeLatency("batchget", totalLatency, batchErr) return tats, nil } // Delete deletes the TAT at the specified bucketKey ('name:id'). A nil return // value does not indicate that the bucketKey existed. func (r *RedisSource) Delete(ctx context.Context, bucketKey string) error { - start := r.clk.Now() - err := r.client.Del(ctx, bucketKey).Err() if err != nil { - r.observeLatency("delete", r.clk.Since(start), err) return err } - - r.observeLatency("delete", r.clk.Since(start), nil) return nil } // Ping checks that each shard of the *redis.Ring is reachable using the PING // command. func (r *RedisSource) Ping(ctx context.Context) error { - start := r.clk.Now() - err := r.client.ForEachShard(ctx, func(ctx context.Context, shard *redis.Client) error { return shard.Ping(ctx).Err() }) if err != nil { - r.observeLatency("ping", r.clk.Since(start), err) return err } - - r.observeLatency("ping", r.clk.Since(start), nil) return nil } From 12dcb84bfc8d29ebcb0c602d0a2a238e3ca06ee2 Mon Sep 17 00:00:00 2001 From: Samantha Date: Wed, 18 Dec 2024 15:48:24 -0500 Subject: [PATCH 2/5] Remove an additional variable. --- ratelimits/source_redis.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/ratelimits/source_redis.go b/ratelimits/source_redis.go index f13f4c57262..eb010395cba 100644 --- a/ratelimits/source_redis.go +++ b/ratelimits/source_redis.go @@ -117,7 +117,6 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st } tats := make(map[string]time.Time, len(bucketKeys)) - notFoundCount := 0 for i, result := range results { tatNano, err := result.(*redis.StringCmd).Int64() if err != nil { @@ -127,7 +126,6 @@ func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[st return nil, err } // Bucket key does not exist. - notFoundCount++ continue } tats[bucketKeys[i]] = time.Unix(0, tatNano).UTC() From a2445e35e53241f9f4836fbd5f84fbc9ddb2dbda Mon Sep 17 00:00:00 2001 From: Samantha Date: Wed, 18 Dec 2024 17:37:46 -0500 Subject: [PATCH 3/5] Revert just *_entry latency metrics --- ratelimits/source_redis.go | 117 +++++++++++++++++++++++++++++++++++-- 1 file changed, 112 insertions(+), 5 deletions(-) diff --git a/ratelimits/source_redis.go b/ratelimits/source_redis.go index eb010395cba..ff32931efc2 100644 --- a/ratelimits/source_redis.go +++ b/ratelimits/source_redis.go @@ -3,6 +3,7 @@ package ratelimits import ( "context" "errors" + "net" "time" "github.com/jmhodges/clock" @@ -15,22 +16,76 @@ var _ Source = (*RedisSource)(nil) // RedisSource is a ratelimits source backed by sharded Redis. type RedisSource struct { - client *redis.Ring - clk clock.Clock + client *redis.Ring + clk clock.Clock + latency *prometheus.HistogramVec } // NewRedisSource returns a new Redis backed source using the provided // *redis.Ring client. func NewRedisSource(client *redis.Ring, clk clock.Clock, stats prometheus.Registerer) *RedisSource { + latency := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "ratelimits_latency", + Help: "Histogram of Redis call latencies labeled by call=[set|get|delete|ping] and result=[success|error]", + // Exponential buckets ranging from 0.0005s to 3s. + Buckets: prometheus.ExponentialBucketsRange(0.0005, 3, 8), + }, + []string{"call", "result"}, + ) + stats.MustRegister(latency) + return &RedisSource{ - client: client, - clk: clk, + client: client, + clk: clk, + latency: latency, + } +} + +var errMixedSuccess = errors.New("some keys not found") + +// resultForError returns a string representing the result of the operation +// based on the provided error. +func resultForError(err error) string { + if errors.Is(errMixedSuccess, err) { + // Indicates that some of the keys in a batchset operation were not found. + return "mixedSuccess" + } else if errors.Is(redis.Nil, err) { + // Bucket key does not exist. + return "notFound" + } else if errors.Is(err, context.DeadlineExceeded) { + // Client read or write deadline exceeded. + return "deadlineExceeded" + } else if errors.Is(err, context.Canceled) { + // Caller canceled the operation. + return "canceled" + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + // Dialer timed out connecting to Redis. + return "timeout" } + var redisErr redis.Error + if errors.Is(err, redisErr) { + // An internal error was returned by the Redis server. + return "redisError" + } + return "failed" +} + +func (r *RedisSource) observeLatency(call string, latency time.Duration, err error) { + result := "success" + if err != nil { + result = resultForError(err) + } + r.latency.With(prometheus.Labels{"call": call, "result": result}).Observe(latency.Seconds()) } // BatchSet stores TATs at the specified bucketKeys using a pipelined Redis // Transaction in order to reduce the number of round-trips to each Redis shard. func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time) error { + start := r.clk.Now() + pipeline := r.client.Pipeline() for bucketKey, tat := range buckets { // Set a TTL of TAT + 10 minutes to account for clock skew. @@ -39,14 +94,21 @@ func (r *RedisSource) BatchSet(ctx context.Context, buckets map[string]time.Time } _, err := pipeline.Exec(ctx) if err != nil { + r.observeLatency("batchset", r.clk.Since(start), err) return err } + + totalLatency := r.clk.Since(start) + + r.observeLatency("batchset", totalLatency, nil) return nil } // BatchSetNotExisting attempts to set TATs for the specified bucketKeys if they // do not already exist. Returns a map indicating which keys already existed. func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[string]time.Time) (map[string]bool, error) { + start := r.clk.Now() + pipeline := r.client.Pipeline() cmds := make(map[string]*redis.BoolCmd, len(buckets)) for bucketKey, tat := range buckets { @@ -56,10 +118,12 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin } _, err := pipeline.Exec(ctx) if err != nil { + r.observeLatency("batchsetnotexisting", r.clk.Since(start), err) return nil, err } alreadyExists := make(map[string]bool, len(buckets)) + totalLatency := r.clk.Since(start) for bucketKey, cmd := range cmds { success, err := cmd.Result() if err != nil { @@ -69,6 +133,8 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin alreadyExists[bucketKey] = true } } + + r.observeLatency("batchsetnotexisting", totalLatency, nil) return alreadyExists, nil } @@ -76,6 +142,8 @@ func (r *RedisSource) BatchSetNotExisting(ctx context.Context, buckets map[strin // Redis Transaction in order to reduce the number of round-trips to each Redis // shard. func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]increment) error { + start := r.clk.Now() + pipeline := r.client.Pipeline() for bucketKey, incr := range buckets { pipeline.IncrBy(ctx, bucketKey, incr.cost.Nanoseconds()) @@ -83,22 +151,33 @@ func (r *RedisSource) BatchIncrement(ctx context.Context, buckets map[string]inc } _, err := pipeline.Exec(ctx) if err != nil { + r.observeLatency("batchincrby", r.clk.Since(start), err) return err } + + totalLatency := r.clk.Since(start) + r.observeLatency("batchincrby", totalLatency, nil) return nil } // Get retrieves the TAT at the specified bucketKey. If the bucketKey does not // exist, ErrBucketNotFound is returned. func (r *RedisSource) Get(ctx context.Context, bucketKey string) (time.Time, error) { + start := r.clk.Now() + tatNano, err := r.client.Get(ctx, bucketKey).Int64() if err != nil { if errors.Is(err, redis.Nil) { // Bucket key does not exist. + r.observeLatency("get", r.clk.Since(start), err) return time.Time{}, ErrBucketNotFound } + // An error occurred while retrieving the TAT. + r.observeLatency("get", r.clk.Since(start), err) return time.Time{}, err } + + r.observeLatency("get", r.clk.Since(start), nil) return time.Unix(0, tatNano).UTC(), nil } @@ -107,50 +186,78 @@ func (r *RedisSource) Get(ctx context.Context, bucketKey string) (time.Time, err // shard. If a bucketKey does not exist, it WILL NOT be included in the returned // map. func (r *RedisSource) BatchGet(ctx context.Context, bucketKeys []string) (map[string]time.Time, error) { + start := r.clk.Now() + pipeline := r.client.Pipeline() for _, bucketKey := range bucketKeys { pipeline.Get(ctx, bucketKey) } results, err := pipeline.Exec(ctx) if err != nil && !errors.Is(err, redis.Nil) { + r.observeLatency("batchget", r.clk.Since(start), err) return nil, err } + totalLatency := r.clk.Since(start) + tats := make(map[string]time.Time, len(bucketKeys)) + notFoundCount := 0 for i, result := range results { tatNano, err := result.(*redis.StringCmd).Int64() if err != nil { if !errors.Is(err, redis.Nil) { // This should never happen as any errors should have been // caught after the pipeline.Exec() call. + r.observeLatency("batchget", r.clk.Since(start), err) return nil, err } - // Bucket key does not exist. + notFoundCount++ continue } tats[bucketKeys[i]] = time.Unix(0, tatNano).UTC() } + + var batchErr error + if notFoundCount < len(results) { + // Some keys were not found. + batchErr = errMixedSuccess + } else if notFoundCount == len(results) { + // All keys were not found. + batchErr = redis.Nil + } + + r.observeLatency("batchget", totalLatency, batchErr) return tats, nil } // Delete deletes the TAT at the specified bucketKey ('name:id'). A nil return // value does not indicate that the bucketKey existed. func (r *RedisSource) Delete(ctx context.Context, bucketKey string) error { + start := r.clk.Now() + err := r.client.Del(ctx, bucketKey).Err() if err != nil { + r.observeLatency("delete", r.clk.Since(start), err) return err } + + r.observeLatency("delete", r.clk.Since(start), nil) return nil } // Ping checks that each shard of the *redis.Ring is reachable using the PING // command. func (r *RedisSource) Ping(ctx context.Context) error { + start := r.clk.Now() + err := r.client.ForEachShard(ctx, func(ctx context.Context, shard *redis.Client) error { return shard.Ping(ctx).Err() }) if err != nil { + r.observeLatency("ping", r.clk.Since(start), err) return err } + + r.observeLatency("ping", r.clk.Since(start), nil) return nil } From 12fb338e2b2d7f5f84974d01dcf255d374b69d4c Mon Sep 17 00:00:00 2001 From: Samantha Date: Thu, 19 Dec 2024 11:33:38 -0500 Subject: [PATCH 4/5] Replace limit.overrideKey and remove limit.isOverride() --- ratelimits/limit.go | 11 +++-------- ratelimits/transaction.go | 4 ++-- ratelimits/transaction_test.go | 24 ++++++++++++------------ 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/ratelimits/limit.go b/ratelimits/limit.go index 16dc65ac962..d6039124bdc 100644 --- a/ratelimits/limit.go +++ b/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. @@ -196,7 +191,7 @@ 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) + lim.isOverride = true 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 diff --git a/ratelimits/transaction.go b/ratelimits/transaction.go index b5fd1653269..fa4b5e88715 100644 --- a/ratelimits/transaction.go +++ b/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) diff --git a/ratelimits/transaction_test.go b/ratelimits/transaction_test.go index 8cf0b798a1e..b4a25e837f0 100644 --- a/ratelimits/transaction_test.go +++ b/ratelimits/transaction_test.go @@ -79,14 +79,14 @@ 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"}) @@ -94,14 +94,14 @@ func TestFailedAuthorizationsPerDomainPerAccountTransactions(t *testing.T) { 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,7 +153,7 @@ 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"}) @@ -161,7 +161,7 @@ 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 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) { From e4af87b5d75d3e2c5d3bfa569fcc96efd472d108 Mon Sep 17 00:00:00 2001 From: Samantha Date: Thu, 19 Dec 2024 13:53:09 -0500 Subject: [PATCH 5/5] Move assignment and precompute --- ratelimits/limit.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ratelimits/limit.go b/ratelimits/limit.go index d6039124bdc..72fc3a1a555 100644 --- a/ratelimits/limit.go +++ b/ratelimits/limit.go @@ -173,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 { @@ -191,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.isOverride = true 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 } }