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.

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

WFE: Add new key-value ratelimits implementation #7089

Merged
merged 15 commits into from
Oct 4, 2023

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Sep 18, 2023

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
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
Copy link
Member

@pgporada pgporada left a comment

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?

cmd/boulder-wfe2/main.go Outdated Show resolved Hide resolved
cmd/boulder-wfe2/main.go Outdated Show resolved Hide resolved
cmd/boulder-wfe2/main.go Outdated Show resolved Hide resolved
wfe2/wfe.go Outdated Show resolved Hide resolved
wfe2/wfe.go Outdated Show resolved Hide resolved
ratelimits/names_test.go Outdated Show resolved Hide resolved
ratelimits/source_redis.go Outdated Show resolved Hide resolved
ratelimits/source_redis.go Outdated Show resolved Hide resolved
@beautifulentropy beautifulentropy requested review from pgporada and a team September 19, 2023 22:25
@beautifulentropy
Copy link
Member Author

beautifulentropy commented Sep 20, 2023

//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.

Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Mkay, I've

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

@aarongable aarongable left a comment

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.

ratelimits/limiter.go Show resolved Hide resolved
@beautifulentropy beautifulentropy merged commit 9aef583 into main Oct 4, 2023
13 checks passed
@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