ratelimits: always use a pointer for limit (#7804)

The zero value for `limit` is invalid, so returning `nil` in error cases
avoids silently returning invalid limits (and means that if code makes a
mistake and references an invalid limit it will be an obvious clear
stack trace).

This is more consistent, since the methods on `limit` use a pointer
receiver. Also, since `limit` is a fairly large object, this saves some
copying.

Related to #7803 and #7797.

Author: jsha

ratelimits/gcra_test.go

@@ -11,7 +11,7 @@ import (
 
 func TestDecide(t *testing.T) {
 	clk := clock.NewFake()
-	limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
+	limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
 	limit.precompute()
 
 	// Begin by using 1 of our 10 requests.
@@ -138,7 +138,7 @@ func TestDecide(t *testing.T) {
 
 func TestMaybeRefund(t *testing.T) {
 	clk := clock.NewFake()
-	limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
+	limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
 	limit.precompute()
 
 	// Begin by using 1 of our 10 requests.

ratelimits/limit.go

@@ -63,7 +63,7 @@ func (l *limit) precompute() {
 	l.burstOffset = l.emissionInterval * l.Burst
 }
 
-func validateLimit(l limit) error {
+func validateLimit(l *limit) error {
 	if l.Burst <= 0 {
 		return fmt.Errorf("invalid burst '%d', must be > 0", l.Burst)
 	}
@@ -76,7 +76,7 @@ func validateLimit(l limit) error {
 	return nil
 }
 
-type limits map[string]limit
+type limits map[string]*limit
 
 // loadDefaults marshals the defaults YAML file at path into a map of limits.
 func loadDefaults(path string) (limits, error) {
@@ -156,7 +156,8 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
 
 	for _, ov := range fromFile {
 		for k, v := range ov {
-			err = validateLimit(v.limit)
+			limit := &v.limit
+			err = validateLimit(limit)
 			if err != nil {
 				return nil, fmt.Errorf("validating override limit %q: %w", k, err)
 			}
@@ -167,7 +168,6 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
 			v.limit.name = name
 
 			for _, entry := range v.Ids {
-				limit := v.limit
 				id := entry.Id
 				err = validateIdForName(name, id)
 				if err != nil {
@@ -248,11 +248,11 @@ func newLimitRegistry(defaults, overrides string) (*limitRegistry, error) {
 // required, bucketKey is optional. If bucketkey is empty, the default for the
 // limit specified by name is returned. If no default limit exists for the
 // specified name, errLimitDisabled is returned.
-func (l *limitRegistry) getLimit(name Name, bucketKey string) (limit, error) {
+func (l *limitRegistry) getLimit(name Name, bucketKey string) (*limit, error) {
 	if !name.isValid() {
 		// This should never happen. Callers should only be specifying the limit
 		// Name enums defined in this package.
-		return limit{}, fmt.Errorf("specified name enum %q, is invalid", name)
+		return nil, fmt.Errorf("specified name enum %q, is invalid", name)
 	}
 	if bucketKey != "" {
 		// Check for override.
@@ -265,5 +265,5 @@ func (l *limitRegistry) getLimit(name Name, bucketKey string) (limit, error) {
 	if ok {
 		return dl, nil
 	}
-	return limit{}, errLimitDisabled
+	return nil, errLimitDisabled
 }

ratelimits/limit_test.go

@@ -42,11 +42,11 @@ func TestParseOverrideNameId(t *testing.T) {
 }
 
 func TestValidateLimit(t *testing.T) {
-	err := validateLimit(limit{Burst: 1, Count: 1, Period: config.Duration{Duration: time.Second}})
+	err := validateLimit(&limit{Burst: 1, Count: 1, Period: config.Duration{Duration: time.Second}})
 	test.AssertNotError(t, err, "valid limit")
 
 	// All of the following are invalid.
-	for _, l := range []limit{
+	for _, l := range []*limit{
 		{Burst: 0, Count: 1, Period: config.Duration{Duration: time.Second}},
 		{Burst: 1, Count: 0, Period: config.Duration{Duration: time.Second}},
 		{Burst: 1, Count: 1, Period: config.Duration{Duration: 0}},

ratelimits/limiter_test.go

@@ -497,7 +497,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 5 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewRegistrationsPerIPAddress,
 						Burst:  10,
 						Period: config.Duration{Duration: time.Hour},
@@ -513,7 +513,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 10 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewRegistrationsPerIPv6Range,
 						Burst:  5,
 						Period: config.Duration{Duration: time.Hour},
@@ -529,7 +529,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 10 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewOrdersPerAccount,
 						Burst:  2,
 						Period: config.Duration{Duration: time.Hour},
@@ -545,7 +545,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 15 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   FailedAuthorizationsPerDomainPerAccount,
 						Burst:  7,
 						Period: config.Duration{Duration: time.Hour},
@@ -562,7 +562,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 20 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   CertificatesPerDomain,
 						Burst:  3,
 						Period: config.Duration{Duration: time.Hour},
@@ -579,7 +579,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 20 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   CertificatesPerDomainPerAccount,
 						Burst:  3,
 						Period: config.Duration{Duration: time.Hour},
@@ -596,7 +596,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 30 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name: 9999999,
 					},
 				},

ratelimits/transaction.go

@@ -107,7 +107,7 @@ func newFQDNSetBucketKey(name Name, orderNames []string) (string, error) { //nol
 // it would fail validateTransaction (for instance because cost and burst are zero).
 type Transaction struct {
 	bucketKey string
-	limit     limit
+	limit     *limit
 	cost      int64
 	check     bool
 	spend     bool
@@ -143,7 +143,7 @@ func validateTransaction(txn Transaction) (Transaction, error) {
 	return txn, nil
 }
 
-func newTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,
@@ -153,7 +153,7 @@ func newTransaction(limit limit, bucketKey string, cost int64) (Transaction, err
 	})
 }
 
-func newCheckOnlyTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newCheckOnlyTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,
@@ -162,7 +162,7 @@ func newCheckOnlyTransaction(limit limit, bucketKey string, cost int64) (Transac
 	})
 }
 
-func newSpendOnlyTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newSpendOnlyTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,