flyteadmin icon indicating copy to clipboard operation
flyteadmin copied to clipboard

Add RateLimit middleware using TokenBucket algorithm

Open LaPetiteSouris opened this issue 2 years ago • 8 comments

TL;DR

Solve https://github.com/flyteorg/flyte/issues/327

Type

  • [ ] Bug Fix
  • [x] Feature
  • [ ] Plugin

Are all requirements met?

  • [ ] Code completed
  • [ ] Smoke tested
  • [x] 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 https://github.com/flyteorg/flyte/issues/327

Follow-up issue

NA

LaPetiteSouris avatar May 08 '23 13:05 LaPetiteSouris

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

welcome[bot] avatar May 08 '23 13:05 welcome[bot]

Codecov Report

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

:exclamation: 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%) :arrow_up:
plugins/rate_limit.go 87.50% <87.50%> (ø)
pkg/config/serverconfig_flags.go 65.45% <100.00%> (+5.07%) :arrow_up:

... and 153 files with indirect coverage changes

codecov[bot] avatar May 08 '23 16:05 codecov[bot]

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 avatar May 12 '23 17:05 LaPetiteSouris

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 ?

LaPetiteSouris avatar May 13 '23 06:05 LaPetiteSouris

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 https://github.com/flyteorg/flyteadmin/pull/563.

eapolinario avatar May 16 '23 01:05 eapolinario

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.

LaPetiteSouris avatar May 16 '23 06:05 LaPetiteSouris

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

eapolinario avatar May 16 '23 20:05 eapolinario

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

I rebase to master to have proper commit set up.

LaPetiteSouris avatar May 16 '23 21:05 LaPetiteSouris