The Wayback Machine - https://web.archive.org/web/20210802173822/https://github.com/labstack/echo/pull/1724
Skip to content

/ echo

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

Jump to bottom

adds middleware for rate limiting #1724

Merged
merged 40 commits into from Jan 15, 2021
Merged

adds middleware for rate limiting #1724

merged 40 commits into from Jan 15, 2021

Conversation

@iambenkay
Copy link
Contributor

@iambenkay iambenkay commented Dec 17, 2020 •
edited

What's New?

This feature implements a store agnostic rate limiting middleware i.e configurable with any store of user's choice.

Here is a dummy snippet of how a certain (unconventional) redis store might be integrated with the rate limiting middleware

type RedisStore struct {
   client redis.Client
}

// Store config must implement Allow for rate limiting middleware
func (store *RedisStore) Allow(identifier) bool {
   // run logic here that decides if user should be permitted
   return true
}

func main(){
   e := echo.New()

   redisStore := RedisStore{
      client: redis.Client{}
   }
   
   limiterMW := middleware.RateLimiter(redisStore)
   e.Use(limiterMW)
}

I threw in an InMemory implementation for people like me who want to get on the go fast.

func main(){
   e := echo.New()

   var inMemoryStore = middleware.RateLimiterMemoryStore{
      rate: 1,
      burst: 3,
   }
   
   limiterMW := middleware.RateLimiterWithConfig(RateLimiterConfig{
      Store: &inMemoryStore,
      SourceFunc: func(ctx echo.Context) string {
         return ctx.RealIP()
      },
   })
   e.Use(limiterMW)
}

closes #1721
iambenkay added 4 commits Dec 17, 2020
@iambenkay
adds middleware for rate limiting
180d26b
@iambenkay
added comment for InMemoryStore ShouldAllow
dc04e2c
@iambenkay
removed redundant mutex declaration
e0e9688
@iambenkay
fixed lint issues
ef2377c
@codecov
Copy link

@codecov codecov bot commented Dec 17, 2020 •
edited

Codecov Report

Merging #1724 (bbfb0ab) into master (67263b5) will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1724      +/-   ##
==========================================
+ Coverage   86.88%   87.32%   +0.44%     
==========================================
  Files          30       31       +1     
  Lines        2089     2162      +73     
==========================================
+ Hits         1815     1888      +73     
  Misses        175      175              
  Partials       99       99
Impacted Files Coverage Δ
middleware/rate_limiter.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67263b5...bbfb0ab. Read the comment docs.
@iambenkay
removed sleep from tests
9b63f99
@iambenkay iambenkay force-pushed the iambenkay:feature/rate-limiter-middleware branch from a945c80 to 9b63f99 Dec 17, 2020
@iambenkay
improved coverage
34d9097
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Dec 17, 2020 •
edited

Using time.Sleep in tests produces different results on different machines due to different computation speeds on different machines for example the github action ran the tests sucessfully on ubuntu but failed on macos before I had to take it out.

TODO: Find a more reliable method for testing the rate limiting which does not involve using the time.Sleep

I am open to any suggestions on this time issue.
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter.go Outdated Show resolved Hide resolved
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter.go Show resolved Hide resolved
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter.go Outdated Show resolved Hide resolved
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter.go Outdated Show resolved Hide resolved
@iambenkay
refactor: renames Identifiers, includes default SourceFunc
02efca7
@lammel
Copy link
Contributor

@lammel lammel commented Dec 17, 2020

Lovely overall!
I'm looking forward to put that beast into action. As we are using freecache currently I'll probably implement something like a FreeCacheLimiterStore for that then. Good to know that's easily doable.
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Dec 17, 2020

TODO: Implement an expire functionality in order to automatically manage the amount of visitors that are currently active.

Added a last seen property to the visitor struct. I intend on adding an Optional AutoExpire method to the default InMemoryStore eventually.
@iambenkay
Added last seen stats for visitor
8d34f11
@iambenkay iambenkay force-pushed the iambenkay:feature/rate-limiter-middleware branch from c6e7fc6 to 8d34f11 Dec 17, 2020
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter_test.go Outdated Show resolved Hide resolved
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter.go Outdated Show resolved Hide resolved
@lammel
lammel reviewed Dec 17, 2020
View changes
Copy link
Contributor

@lammel lammel left a comment

Looks even better now...
@iambenkay
uses http Constants for improved readdability
674665e 
adds default error handler
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Dec 17, 2020

Now Custom Error Handler can be specified using RateLimiterWithConfig

mw := middleware.RateLimiterWithConfig(RateLimiterConfig{
   ErrorHandler: func(c echo.Context) error {
      return c.JSON(http.StatusTooManyRequests, nil)
   },
   Store: inMemoryStore,
})
@lammel
lammel reviewed Dec 17, 2020
View changes
middleware/rate_limiter_test.go Outdated Show resolved Hide resolved
@iambenkay
used other handler apart from default handler to mark custom error ha…
018105b 
…ndler for rate limiting
@lammel
lammel approved these changes Dec 17, 2020
View changes
Copy link
Contributor

@lammel lammel left a comment

@pafuent you missed again all the fun. Feedback welcome, ready to merge.

@iambenkay Good work. Can you prepare a PR for the docs too in echox?
@lammel lammel added this to In progress in Echo v4.2 Dec 17, 2020
@pafuent
pafuent reviewed Dec 18, 2020
View changes
Copy link
Contributor

@pafuent pafuent left a comment

@iambenkay I like the overall implementation. Please let me know if my comments makes sense for you.

@lammel Yep, it seems that I missed the fun 😢 (it was hard to resist the temptation to stop working and have some fun when I saw the emails back and forth)
middleware/rate_limiter.go Outdated Show resolved Hide resolved
middleware/rate_limiter.go Outdated Show resolved Hide resolved
middleware/rate_limiter.go Outdated Show resolved Hide resolved
middleware/rate_limiter_test.go Outdated Show resolved Hide resolved
middleware/rate_limiter_test.go Outdated Show resolved Hide resolved
middleware/rate_limiter.go Outdated Show resolved Hide resolved
@iambenkay iambenkay requested a review from lammel Jan 5, 2021
@pafuent
Copy link
Contributor

@pafuent pafuent commented Jan 6, 2021

@iambenkay please address this two comments:
#1724 (comment)
#1724 (comment)
Besides of those two LGTM, so after addressing those I'll approve the PR. Thanks for the hard work and patience.
iambenkay and others added 7 commits Jan 6, 2021
@iambenkay
updated errorHandler and denyHandler to use echo.HTTPError
4e32a58
@lammel
Improve wording for error and comments
7dc77bb
@lammel
Remove duplicate lastSeen marking for Allow
8c7eac7
@iambenkay
Merge branch 'master' of github.com:iambenkay/echo into feature/rate-…
bcc7fe2 
…limiter-middleware
@lammel
Improve wording for comments
5374337
@lammel
Add disclaimer on perf characteristics of memory store
5f731d6
@lammel
Merge branch 'feature/rate-limiter-middleware' of https://github.com/…
e58f4fd 
…iambenkay/echo into feature/rate-limiter-middleware
@lammel
lammel reviewed Jan 6, 2021
View changes
Copy link
Contributor

@lammel lammel left a comment

Very nice. We should just discuss the Allow signature.
middleware/rate_limiter.go Outdated
// RateLimiterStore is the interface to be implemented by custom stores.
RateLimiterStore interface {
// Stores for the rate limiter have to implement the Allow method
Allow(identifier string) bool

This comment has been minimized.

Sign in to view
@lammel

lammel Jan 6, 2021
Contributor

Allow may have internal errors due to hash limits or other issues.
In this case it may make sense to make the signature to be Allow(identifier string) bool, error to be able to handle this special cases as we will be able to pass the error also to the DenyHandler. Use case would be for example to allow access (not apply rate limit) for DB connection errors (or other internal errors).

Not sure if this is needed, but it was also suggested by @aldas

This comment has been minimized.

Sign in to view
@iambenkay

iambenkay Jan 7, 2021
Author Contributor

Okay let me include this. it is a valid point.

@lammel
Copy link
Contributor

@lammel lammel commented Jan 6, 2021

@iambenkay Pushed some comment improvements to resolve some remaining comments.
@lammel
Copy link
Contributor

@lammel lammel commented Jan 6, 2021

@iambenkay Can you do a documentation PR in labstack/echox too?
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Jan 6, 2021

Yes!
@iambenkay
changes Allow signature on rate limiter to return err too
d210158
@iambenkay iambenkay mentioned this pull request Jan 7, 2021
Merged
@iambenkay
Merge branch 'feature/rate-limiter-middleware' of github.com:iambenka…
bbfb0ab 
…y/echo into feature/rate-limiter-middleware
@lammel
lammel approved these changes Jan 7, 2021
View changes
Copy link
Contributor

@lammel lammel left a comment

@pafuent Approved from my side. Feel free to merge.
Thanks (especially for your patience) @iambenkay.

Btw. tested the limiter locally with a simple hello world and it worked as expected with up to 30000 req/s and my laptop (from single local IP)
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Jan 8, 2021

That's super!
@iambenkay
Copy link
Contributor Author

@iambenkay iambenkay commented Jan 12, 2021

@pafuent calling your attention to this!
@pafuent
pafuent approved these changes Jan 15, 2021
View changes
Copy link
Contributor

@pafuent pafuent left a comment

@iambenkay Thanks for this new middleware and thanks for your patience
@pafuent pafuent merged commit 7c8592a into labstack:master Jan 15, 2021
15 checks passed
15 checks passed
@github-actions
ubuntu-latest @ Go 1.12
Details
@github-actions
ubuntu-latest @ Go 1.13
Details
@github-actions
ubuntu-latest @ Go 1.14
Details
@github-actions
ubuntu-latest @ Go 1.15
Details
@github-actions
macos-latest @ Go 1.12
Details
@github-actions
macos-latest @ Go 1.13
Details
@github-actions
macos-latest @ Go 1.14
Details
@github-actions
macos-latest @ Go 1.15
Details
@github-actions
windows-latest @ Go 1.12
Details
@github-actions
windows-latest @ Go 1.13
Details
@github-actions
windows-latest @ Go 1.14
Details
@github-actions
windows-latest @ Go 1.15
Details
@github-actions
Benchmark comparison ubuntu-latest @ Go 1.15
Details
@codecov
codecov/patch 100.00% of diff hit (target 86.88%)
Details
@codecov
codecov/project 87.32% (+0.44%) compared to 67263b5
Details
@lammel lammel moved this from In progress to Done in Echo v4.2 Feb 8, 2021
This was referenced Mar 14, 2021
Closed
Closed
Open
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldas aldas

@lammel lammel

@pafuent pafuent

Assignees
No one assigned
Labels
discussion enhancement
Projects
No open projects
Echo v4.2
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Rate Limiting for Echo using golang.org/x/time/rate
4 participants
@iambenkay @lammel @aldas @pafuent