Add RateLimit middleware using TokenBucket algorithm
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
userIDandrateper user ID.rateis 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
authpackage. 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
flyteadminis 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
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).
Codecov Report
Merging #557 (8e4b5bc) into master (a853dac) will increase coverage by
1.60%. The diff coverage is88.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: |
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.
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 ?
Thanks for launching the CI/CD for me. The
Integration testsphase 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.
Thanks for launching the CI/CD for me. The
Integration testsphase 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 , can you merge master instead? This last update brought commits that don't have to do with your changes.
@eapolinario Sorry for that. I did not pay attention.
I rebase to master to have proper commit set up.