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

feat: preserve rate limiters in memory across configuration reloads #1792

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

cstockton
Copy link
Contributor

The goal with this change is to persist the rate limiters across reloads with the least invasive changes possible. To do this I first added a LimiterOptions structure to hold the current set of rate limiters. I then identified each call of tollbooth.New and moved the construction of the limiter without modifications to options.go. I assigned each limiter a distinct field to be referenced during the API object creation.

Next I needed to add an optional parameter to the NewAPI methods so I could store the new LimiterOptions onto the API object for reference during route construction. To do this without breaking all existing calls to NewAPI I used the options pattern. This makes the method accept a parametric set of values implementing a common interface, so future problems of similar nature can also be added as options.

I then replaced all the local anonymous limiter.Limiters made inline within the API route construction with each corresponding newly added field within the LimiterOptions struct.

What kind of change does this PR introduce?

Feature

What is the current behavior?

Currently we do not persist rate limiters when the config is reloaded.

What is the new behavior?

We persist rate limiters in memory across configuration reloads.

The goal with this change is to persist the rate limiters across
reloads with the least invasive changes possible. To do this I
first added a LimiterOptions structure to hold the current set
of rate limiters. I then identified each call of tollbooth.New
and moved the construction of the limiter without modifications
to options.go. I assigned each limiter a distinct field to be
referenced during the API object creation.

Next I needed to add an optional parameter to the NewAPI methods
so I could store the new LimiterOptions onto the API object for
reference during route construction. To do this without breaking
all existing calls to NewAPI I used the options pattern. This
makes the method accept a parametric set of values implementing
a common interface, so future problems of similar nature can
also be added as options.

I then replaced all the local anonymous limiter.Limiters made
inline within the API route construction with each corresponding
newly added field within the LimiterOptions struct.
@cstockton cstockton requested a review from a team as a code owner October 8, 2024 21:23
@coveralls
Copy link

Pull Request Test Coverage Report for Build 11244024882

Details

  • 114 of 120 (95.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 58.03%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/serve_cmd.go 0 6 0.0%
Totals Coverage Status
Change from base Build 11195341479: 0.04%
Covered Lines: 9366
Relevant Lines: 16140

💛 - Coveralls

Copy link
Contributor

@J0 J0 left a comment

Choose a reason for hiding this comment

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

Code changes look fine to me

@J0
Copy link
Contributor

J0 commented Oct 9, 2024

Oh also, what are the implications of this PR on #1746 (if any)? Will we adjust #1746 against the changes made in this PR?

@cstockton
Copy link
Contributor Author

@J0 Good question, I think both pull requests aim to pull in the same direction. This one may have more impact, I'll let @hf chime in on that. It may be worth hand merging the good bits into main after this is merged in.

Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

Maybe change the title to say "preservation" or similar instead of "persistence." The latter to me sounds like its getting persisted somewhere (disk, database, etc.) when in fact this just preserves the values across config changes.

Titles are important as they show up in the changelog.

@hf
Copy link
Contributor

hf commented Oct 10, 2024

Also feel free to remove the headings from the PR description -- those are mostly to guide external contributors.

@cstockton cstockton changed the title feat: rate limiter persistence feat: preserve rate limiters in memory across configuration reloads Oct 10, 2024
@cstockton cstockton merged commit 0a3968b into master Oct 10, 2024
3 checks passed
@cstockton cstockton deleted the cs/feat-rate-limiter-persistence branch October 10, 2024 15:18
cstockton pushed a commit that referenced this pull request Oct 15, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.163.0](v2.162.2...v2.163.0)
(2024-10-15)


### Features

* add mail header support via `GOTRUE_SMTP_HEADERS` with `$messageType`
([#1804](#1804))
([99d6a13](99d6a13))
* add MFA for WebAuthn
([#1775](#1775))
([8cc2f0e](8cc2f0e))
* configurable email and sms rate limiting
([#1800](#1800))
([5e94047](5e94047))
* mailer logging ([#1805](#1805))
([9354b83](9354b83))
* preserve rate limiters in memory across configuration reloads
([#1792](#1792))
([0a3968b](0a3968b))


### Bug Fixes

* add twilio verify support on mfa
([#1714](#1714))
([aeb5d8f](aeb5d8f))
* email header setting no longer misleading
([#1802](#1802))
([3af03be](3af03be))
* enforce authorized address checks on send email only
([#1806](#1806))
([c0c5b23](c0c5b23))
* fix `getExcludedColumns` slice allocation
([#1788](#1788))
([7f006b6](7f006b6))
* Fix reqPath for bypass check for verify EP
([#1789](#1789))
([646dc66](646dc66))
* inline mailme package for easy development
([#1803](#1803))
([fa6f729](fa6f729))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants