Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ratelimits: always use a pointer for limit #7804

Merged
merged 2 commits into from
Nov 15, 2024
Merged

ratelimits: always use a pointer for limit #7804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4c5ac4
ratelimits: always use a pointer for limit
jsha Nov 12, 2024
032c976
Merge branch 'main' into limit-is-a-pointer
beautifulentropy Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ratelimits/gcra_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
14 changes: 7 additions & 7 deletions ratelimits/limit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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
}
4 changes: 2 additions & 2 deletions ratelimits/limit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}},
Expand Down
14 changes: 7 additions & 7 deletions ratelimits/limiter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -596,7 +596,7 @@ func TestRateLimitError(t *testing.T) {
allowed: false,
retryIn: 30 * time.Second,
transaction: Transaction{
limit: limit{
limit: &limit{
name: 9999999,
},
},
Expand Down
8 changes: 4 additions & 4 deletions ratelimits/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down