Skip to content

Commit

Permalink
Replace limit.overrideKey and remove limit.isOverride()
Browse files Browse the repository at this point in the history
  • Loading branch information
beautifulentropy committed Dec 19, 2024
1 parent 3e8537a commit 12fb338
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 22 deletions.
11 changes: 3 additions & 8 deletions ratelimits/limit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions ratelimits/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 12 additions & 12 deletions ratelimits/transaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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"})
Expand All @@ -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.
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 12fb338

Please sign in to comment.