Add RateLimit middleware using TokenBucket algorithm

[LaPetiteSouris]

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]

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

[EngHabu]

Thank you for your contribution!

[codecov]

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]

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]

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]

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]

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]

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

[LaPetiteSouris]

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

I rebase to master to have proper commit set up.

[eapolinario]

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?

[eapolinario]

shouldn't this happen before we modify the map?