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

feat: add rate limiting per user email for admin #113

Merged
merged 11 commits into from Jul 18, 2020

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Jul 15, 2020

Towards #76.

@googlebot googlebot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Jul 15, 2020
cmd/server/main.go Outdated Show resolved Hide resolved
Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

My thoughts: this is going to be rather expensive and database intensive. Can we leverage Redis and our go-limiter installation with a different key? It will have the same effect with much less load on the database. @mikehelmick what are your thoughts?

@crwilcox
Copy link
Contributor Author

@sethvargo I like using a cache for this, but not sure if that works if we also want to track keys issued? Though it would stop the hug of death that will come if someone tries to mass issue since they will get quota'd early on.

@sethvargo
Copy link
Member

track keys issued

What kind of tracking are you thinking?

@crwilcox
Copy link
Contributor Author

Part of the original ask was to know how many keys a given account has issued.

pkg/database/migrations.go Outdated Show resolved Hide resolved
pkg/controller/middleware/auth.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/controller/middleware/auth.go Outdated Show resolved Hide resolved
pkg/controller/middleware/auth.go Outdated Show resolved Hide resolved
pkg/controller/middleware/auth.go Outdated Show resolved Hide resolved
@crwilcox crwilcox changed the title feat: add issuance quota for codes feat: add rate limiting per user email for admin Jul 17, 2020
Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

/lgtm

Minor performance consideration commented - we can clean that up in a followup though.


func userEmailKeyFunc() httplimit.KeyFunc {
return func(r *http.Request) (string, error) {
ipKeyFunc := httplimit.IPKeyFunc("X-Forwarded-For")
Copy link
Member

Choose a reason for hiding this comment

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

This is a nitty performance thing. We should move this outside of the return so it's only allocated once. Here it will allocate on each request.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crwilcox, sethvargo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit 3902a5e into google:main Jul 18, 2020
@google google locked and limited conversation to collaborators Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Auto: added by CLA bot when all committers have signed a CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants