From d962c610673776abd46440da578d4e0e2a722f81 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Tue, 3 Dec 2024 14:52:16 -0800 Subject: [PATCH] RA and WFE tests: use inmem rate limit source (#7859) The purpose of these RA and WFE unit tests is to test how they deal with certain rate limit conditions, not to test talking to an actual redis instance. Streamline the tests by having them talk to an in-memory rate limits store, rather than a redis-backed one. --- ra/ra_test.go | 45 ++++++-------------------------------- ratelimits/limiter.go | 4 ++-- ratelimits/limiter_test.go | 2 +- ratelimits/source.go | 8 +++---- ratelimits/source_redis.go | 2 +- ratelimits/source_test.go | 2 +- wfe2/wfe_test.go | 27 +---------------------- 7 files changed, 17 insertions(+), 73 deletions(-) diff --git a/ra/ra_test.go b/ra/ra_test.go index 8969ad8aa..745275639 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -16,7 +16,6 @@ import ( "fmt" "math/big" mrand "math/rand/v2" - "os" "regexp" "strconv" "strings" @@ -40,7 +39,6 @@ import ( akamaipb "github.com/letsencrypt/boulder/akamai/proto" capb "github.com/letsencrypt/boulder/ca/proto" - "github.com/letsencrypt/boulder/cmd" "github.com/letsencrypt/boulder/config" "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" @@ -60,7 +58,6 @@ import ( rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/ratelimit" "github.com/letsencrypt/boulder/ratelimits" - bredis "github.com/letsencrypt/boulder/redis" "github.com/letsencrypt/boulder/sa" sapb "github.com/letsencrypt/boulder/sa/proto" "github.com/letsencrypt/boulder/test" @@ -283,7 +280,7 @@ func newAcctKey(t *testing.T) []byte { return acctKey } -func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAuthorityClient, *RegistrationAuthorityImpl, *ratelimits.RedisSource, clock.FakeClock, func()) { +func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAuthorityClient, *RegistrationAuthorityImpl, ratelimits.Source, clock.FakeClock, func()) { err := json.Unmarshal(AccountKeyJSONA, &AccountKeyA) test.AssertNotError(t, err, "Failed to unmarshal public JWK") err = json.Unmarshal(AccountKeyJSONB, &AccountKeyB) @@ -352,39 +349,11 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho }, }, nil, nil, 0, log, metrics.NoopRegisterer) - var source *ratelimits.RedisSource - var limiter *ratelimits.Limiter - var txnBuilder *ratelimits.TransactionBuilder - if strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") { - rc := bredis.Config{ - Username: "unittest-rw", - TLS: cmd.TLSConfig{ - CACertFile: "../test/certs/ipki/minica.pem", - CertFile: "../test/certs/ipki/localhost/cert.pem", - KeyFile: "../test/certs/ipki/localhost/key.pem", - }, - Lookups: []cmd.ServiceDomain{ - { - Service: "redisratelimits", - Domain: "service.consul", - }, - }, - LookupDNSAuthority: "consul.service.consul", - } - rc.PasswordConfig = cmd.PasswordConfig{ - PasswordFile: "../test/secrets/ratelimits_redis_password", - } - ring, err := bredis.NewRingFromConfig(rc, stats, log) - test.AssertNotError(t, err, "making redis ring client") - source = ratelimits.NewRedisSource(ring.Ring, fc, stats) - test.AssertNotNil(t, source, "source should not be nil") - err = source.Ping(context.Background()) - test.AssertNotError(t, err, "Ping should not error") - limiter, err = ratelimits.NewLimiter(fc, source, stats) - test.AssertNotError(t, err, "making limiter") - txnBuilder, err = ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") - test.AssertNotError(t, err, "making transaction composer") - } + rlSource := ratelimits.NewInmemSource() + limiter, err := ratelimits.NewLimiter(fc, rlSource, stats) + test.AssertNotError(t, err, "making limiter") + txnBuilder, err := ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") + test.AssertNotError(t, err, "making transaction composer") testKeyPolicy, err := goodkey.NewPolicy(nil, nil) test.AssertNotError(t, err, "making keypolicy") @@ -401,7 +370,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAutho ra.CA = ca ra.OCSP = &mocks.MockOCSPGenerator{} ra.PA = pa - return va, sa, ra, source, fc, cleanUp + return va, sa, ra, rlSource, fc, cleanUp } func TestValidateContacts(t *testing.T) { diff --git a/ratelimits/limiter.go b/ratelimits/limiter.go index 6231f83ca..74a81363e 100644 --- a/ratelimits/limiter.go +++ b/ratelimits/limiter.go @@ -34,7 +34,7 @@ var allowedDecision = &Decision{allowed: true, remaining: math.MaxInt64} // utilizing a leaky bucket-style approach. type Limiter struct { // source is used to store buckets. It must be safe for concurrent use. - source source + source Source clk clock.Clock spendLatency *prometheus.HistogramVec @@ -43,7 +43,7 @@ type Limiter struct { // NewLimiter returns a new *Limiter. The provided source must be safe for // concurrent use. -func NewLimiter(clk clock.Clock, source source, stats prometheus.Registerer) (*Limiter, error) { +func NewLimiter(clk clock.Clock, source Source, stats prometheus.Registerer) (*Limiter, error) { spendLatency := prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "ratelimits_spend_latency", Help: fmt.Sprintf("Latency of ratelimit checks labeled by limit=[name] and decision=[%s|%s], in seconds", Allowed, Denied), diff --git a/ratelimits/limiter_test.go b/ratelimits/limiter_test.go index 779c268ed..cf18fe271 100644 --- a/ratelimits/limiter_test.go +++ b/ratelimits/limiter_test.go @@ -21,7 +21,7 @@ import ( const tenZeroZeroTwo = "10.0.0.2" // newTestLimiter constructs a new limiter. -func newTestLimiter(t *testing.T, s source, clk clock.FakeClock) *Limiter { +func newTestLimiter(t *testing.T, s Source, clk clock.FakeClock) *Limiter { l, err := NewLimiter(clk, s, metrics.NoopRegisterer) test.AssertNotError(t, err, "should not error") return l diff --git a/ratelimits/source.go b/ratelimits/source.go index ec7983225..c69484c95 100644 --- a/ratelimits/source.go +++ b/ratelimits/source.go @@ -10,8 +10,8 @@ import ( // ErrBucketNotFound indicates that the bucket was not found. var ErrBucketNotFound = fmt.Errorf("bucket not found") -// source is an interface for creating and modifying TATs. -type source interface { +// Source is an interface for creating and modifying TATs. +type Source interface { // BatchSet stores the TATs at the specified bucketKeys (formatted as // 'name:id'). Implementations MUST ensure non-blocking operations by // either: @@ -64,9 +64,9 @@ type inmem struct { m map[string]time.Time } -var _ source = (*inmem)(nil) +var _ Source = (*inmem)(nil) -func newInmem() *inmem { +func NewInmemSource() *inmem { return &inmem{m: make(map[string]time.Time)} } diff --git a/ratelimits/source_redis.go b/ratelimits/source_redis.go index cc2631c2f..b55ee9739 100644 --- a/ratelimits/source_redis.go +++ b/ratelimits/source_redis.go @@ -12,7 +12,7 @@ import ( ) // Compile-time check that RedisSource implements the source interface. -var _ source = (*RedisSource)(nil) +var _ Source = (*RedisSource)(nil) // RedisSource is a ratelimits source backed by sharded Redis. type RedisSource struct { diff --git a/ratelimits/source_test.go b/ratelimits/source_test.go index a4f55ba87..a2347c8bc 100644 --- a/ratelimits/source_test.go +++ b/ratelimits/source_test.go @@ -7,5 +7,5 @@ import ( ) func newInmemTestLimiter(t *testing.T, clk clock.FakeClock) *Limiter { - return newTestLimiter(t, newInmem(), clk) + return newTestLimiter(t, NewInmemSource(), clk) } diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 306cb89aa..d3c243fab 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -52,7 +52,6 @@ import ( "github.com/letsencrypt/boulder/probs" rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/ratelimits" - bredis "github.com/letsencrypt/boulder/redis" "github.com/letsencrypt/boulder/revocation" sapb "github.com/letsencrypt/boulder/sa/proto" "github.com/letsencrypt/boulder/test" @@ -394,8 +393,6 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) { mockSA := mocks.NewStorageAuthorityReadOnly(fc) - log := blog.NewMock() - // Use derived nonces. rncKey := []byte("b8c758dd85e113ea340ce0b3a99f389d40a308548af94d1730a7692c1874f1f") noncePrefix := nonce.DerivePrefix("192.168.1.1:8080", rncKey) @@ -407,29 +404,7 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) { rnc := inmemNonceService // Setup rate limiting. - rc := bredis.Config{ - Username: "unittest-rw", - TLS: cmd.TLSConfig{ - CACertFile: "../test/certs/ipki/minica.pem", - CertFile: "../test/certs/ipki/localhost/cert.pem", - KeyFile: "../test/certs/ipki/localhost/key.pem", - }, - Lookups: []cmd.ServiceDomain{ - { - Service: "redisratelimits", - Domain: "service.consul", - }, - }, - LookupDNSAuthority: "consul.service.consul", - } - rc.PasswordConfig = cmd.PasswordConfig{ - PasswordFile: "../test/secrets/ratelimits_redis_password", - } - ring, err := bredis.NewRingFromConfig(rc, stats, log) - test.AssertNotError(t, err, "making redis ring client") - source := ratelimits.NewRedisSource(ring.Ring, fc, stats) - test.AssertNotNil(t, source, "source should not be nil") - limiter, err := ratelimits.NewLimiter(fc, source, stats) + limiter, err := ratelimits.NewLimiter(fc, ratelimits.NewInmemSource(), stats) test.AssertNotError(t, err, "making limiter") txnBuilder, err := ratelimits.NewTransactionBuilder("../test/config-next/wfe2-ratelimit-defaults.yml", "") test.AssertNotError(t, err, "making transaction composer")