Skip to content

WFE: Add new key-value ratelimits implementation#7089

Merged
beautifulentropy merged 15 commits into
mainfrom
ratelimits-add-to-wfe
Oct 4, 2023
Merged

WFE: Add new key-value ratelimits implementation#7089
beautifulentropy merged 15 commits into
mainfrom
ratelimits-add-to-wfe

Conversation

@beautifulentropy

@beautifulentropy beautifulentropy commented Sep 18, 2023

Copy link
Copy Markdown
Member

Integrate the key-value rate limits from #6947 into the WFE. Rate limits are backed by the Redis source added in #7016, and use the SRV record shard discovery added in #7042.

Part of #5545


Notes for reviewers:

There were a lot of small changes necessary to complete this change:

Changes in ratelimits:

  • Make Name an fmt.Stringer and utilize the new String() method throughout.
  • Improve documentation for NewRegistrationsPerIPv6Range.
  • Add metrics similar to those added to our existing rate limits in ratelimit: Overhaul metrics for the our existing rate limits #7054
  • Changes to Redis Source:
    • Register go-redis client metrics at *redis.Ring construction, outside of the Redis Source. These metrics are germane to client itself, not the source implementation in ratelimits.
    • Remove ratelimits.RedisSource.timeout. We should instead rely on the go-redis read and write timeouts.

Changes in bredis:

  • Copy rocsp_config.RedisConfig to bredis.Config.
  • Add utility function MustRegisterClientMetricsCollector() for registering go-redis client metrics.
  • Add SRV lookup fields to the new bredis.Config and a new constructor NewRingFromConfig() to handle go-redis client and service discovery construction. This is much cleaner than constructing a client inside of the WFE and will be useful if/when we add more Redis caches to the WFE.
  • lookup.updateNow() now re-registers client metrics when new shards are discovered.

Changes in rocsp/config:

  • Move redis client metrics registration from NewReadingClient() to MakeReadClient()
  • MakeReadClient() calls the new bredis.MustRegisterClientMetricsCollector() for consistency.

Change in wfe2:

  • Spend against the NewRegistrationsPerIP/v6Range inside the new checkNewAccountLimits() func and if the subsequent ra.NewRegistration() call fails, refund the spent limit quota using the new refundNewAccountLimits().

Changes in cmd/boulder-wfe:

  • Construct the *redis.Ring client, the bredis.Lookup (optionally), and add the resulting Limiter to the resulting WebFrontEndImpl.

@github-actions

Copy link
Copy Markdown
Contributor

@beautifulentropy, this PR appears to contain configuration changes. Please ensure that a corresponding deployment ticket has been filed with the new configuration values.

@beautifulentropy beautifulentropy marked this pull request as ready for review September 18, 2023 22:54
@beautifulentropy beautifulentropy requested a review from a team as a code owner September 18, 2023 22:54

@pgporada pgporada left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//rocsp/rocsp.go NewReadingClient states rdb must be non-nil, but shouldn't there be a nil check inside the constructor for that rather than just a comment?

Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread wfe2/wfe.go Outdated
Comment thread wfe2/wfe.go Outdated
Comment thread ratelimits/names_test.go Outdated
Comment thread ratelimits/source_redis.go Outdated
Comment thread ratelimits/source_redis.go Outdated
@beautifulentropy beautifulentropy requested review from a team and pgporada September 19, 2023 22:25
@beautifulentropy

beautifulentropy commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

//rocsp/rocsp.go NewReadingClient states rdb must be non-nil, but shouldn't there be a nil check inside the constructor for that rather than just a comment?

I didn't actually write that comment. It shows green because I removed the content in that comment which came directly after it and was no longer applicable due to a change I made inside the function.

@aarongable aarongable left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mkay, I've

Comment thread redis/config.go Outdated
Comment thread redis/config.go Outdated
Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread cmd/boulder-wfe2/main.go Outdated
Comment thread test/secrets/wfe_ratelimits_redis_password
Comment thread wfe2/wfe.go Outdated
Comment thread ratelimits/limiter.go
Comment thread wfe2/wfe.go Outdated
Comment thread wfe2/wfe.go Outdated
@beautifulentropy beautifulentropy force-pushed the ratelimits-add-to-wfe branch 3 times, most recently from 39fbdf5 to e546830 Compare September 28, 2023 17:44
Comment thread ratelimits/limiter.go
Comment thread ratelimits/names.go Outdated
pgporada
pgporada previously approved these changes Oct 2, 2023

@aarongable aarongable left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the exception of just one question about the new context cancellation mechanism.

Comment thread ratelimits/limiter.go
@beautifulentropy beautifulentropy merged commit 9aef583 into main Oct 4, 2023
@beautifulentropy beautifulentropy deleted the ratelimits-add-to-wfe branch October 4, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants