Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add RateLimit middleware using TokenBucket algorithm #557

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

LaPetiteSouris
Copy link

TL;DR

Solve flyteorg/flyte#327

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Main changes

  • Add a simple rate limiter middleware.
  • The RateLimiter components rely on an in-memory map storage to store userID and rate per user ID. rate is a native Golang package to track total count of requests.
  • The in-memory storage is cleaned periodically to reduce memory footprint.
  • The component, which is considered as part of the security scope, relies on the identity of the user provided by auth package. In fact, the best way to uniquely identify requests to rate limit is to track usage per authenticated user.

Tradeoff

  • To keep it simple, an in-memory map storage is used to track rate per user. This becomes inaccurate if multiple instances of flyteadmin is deployed. If this is the case and we are serious about rate limit, another improvement is needed, like introduction of Redis for example, which is out of scopes for this PR.

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes flyteorg/flyte#327

Follow-up issue

NA

@welcome
Copy link

welcome bot commented May 8, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

pkg/config/config.go Show resolved Hide resolved
pkg/server/service.go Show resolved Hide resolved
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/server/service.go Show resolved Hide resolved
plugins/rate_limit.go Outdated Show resolved Hide resolved
plugins/rate_limit.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #557 (8e4b5bc) into master (a853dac) will increase coverage by 1.60%.
The diff coverage is 88.46%.

❗ Current head 8e4b5bc differs from pull request most recent head dfaf183. Consider uploading reports for the commit dfaf183 to get more accurate results

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   58.42%   60.03%   +1.60%     
==========================================
  Files         168      169       +1     
  Lines       16153    13265    -2888     
==========================================
- Hits         9438     7964    -1474     
+ Misses       5875     4461    -1414     
  Partials      840      840              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/config/config.go 20.00% <ø> (+1.25%) ⬆️
plugins/rate_limit.go 87.50% <87.50%> (ø)
pkg/config/serverconfig_flags.go 65.45% <100.00%> (+5.07%) ⬆️

... and 153 files with indirect coverage changes

@LaPetiteSouris LaPetiteSouris changed the title Add RateLimit middleware Add RateLimit middleware using TokenBucket algorithm May 10, 2023
plugins/rate_limit.go Outdated Show resolved Hide resolved
@LaPetiteSouris
Copy link
Author

Would anyone be available to re-review my PR or may be at least approve the workflows so that I have visibility on what are the remaining to fix/improve ?

Thanks a lot.

@LaPetiteSouris
Copy link
Author

Thanks for launching the CI/CD for me. The Integration tests phase failed unexpectedly, I suppose due to some error really unrelated to the PR itself. Is this a flaky tests, related to CI/CD set up, or am I wrong ?

@eapolinario
Copy link
Contributor

Thanks for launching the CI/CD for me. The Integration tests phase failed unexpectedly, I suppose due to some error really unrelated to the PR itself. Is this a flaky tests, related to CI/CD set up, or am I wrong ?

Correct. Fixing the flaky test in #563.

@LaPetiteSouris
Copy link
Author

Thanks for launching the CI/CD for me. The Integration tests phase failed unexpectedly, I suppose due to some error really unrelated to the PR itself. Is this a flaky tests, related to CI/CD set up, or am I wrong ?

Correct. Fixing the flaky test in #563.

Thanks. I synced the PR with master. Need reviews.

@eapolinario
Copy link
Contributor

@LaPetiteSouris , can you merge master instead? This last update brought commits that don't have to do with your changes.

@LaPetiteSouris
Copy link
Author

@eapolinario Sorry for that. I did not pay attention.

I rebase to master to have proper commit set up.

Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

If I understand correctly, we're essentially having a version of sync.Map with TTL. So just to make the code more readable, how about we move that logic behind a new type that does only that?

Comment on lines +48 to +50
if !accessRecord.(*accessRecords).limiter.Allow() {
return RateLimitExceeded(fmt.Errorf("rate limit exceeded"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this happen before we modify the map?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add Rate Limiter for Flyte Admin
4 participants