Skip to content
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

Add local rate limiting #69

Merged
merged 11 commits into from
Aug 10, 2020
Merged

Add local rate limiting #69

merged 11 commits into from
Aug 10, 2020

Conversation

iffyio
Copy link
Collaborator

@iffyio iffyio commented Jul 9, 2020

This adds a local rate limiter filter that applies to
traffic destined to a proxy's endpoints.

Work on #5

Since we don't currently have logic to initialize filters, the filter isn't used outside of the tests so there are a few #[allow(dead_code)] to silence the unused warnings

This adds a local rate limiter filter that applies to
traffic destined to a proxy's endpoints.

Fixes #5
@iffyio iffyio requested a review from markmandel July 9, 2020 19:38
@markmandel
Copy link
Contributor

We could add this to: https://github.com/googleforgames/quilkin/blob/master/src/extensions/mod.rs#L31-L35 default_filters

(As a side note, this is starting to feel like we really need some docs!!!)

Did we want to write an integration test for this in this PR, or will it be coming later?

@iffyio
Copy link
Collaborator Author

iffyio commented Jul 9, 2020

We could add this to: https://github.com/googleforgames/quilkin/blob/master/src/extensions/mod.rs#L31-L35 default_filters

I'm thinking rate limiting isn't something a user would want as default since then we need to know what's a reasonable value? Or do we add it there only in the meantime?

(As a side note, this is starting to feel like we really need some docs!!!)

Do we have plans/suggestions regarding what framework/format etc to use for docs we can start looking into (Not really versed with requirements to get a docs site up and running 😅)?

Did we want to write an integration test for this in this PR, or will it be coming later?

tbh I didn't have any integration test plans for this 😅 mostly because I'm lazy but also didn't think an integration test adds something that we can't cover with a unit test since the functionality is limited to this module. Did you have something in mind?

@markmandel
Copy link
Contributor

We could add this to: https://github.com/googleforgames/quilkin/blob/master/src/extensions/mod.rs#L31-L35 default_filters

I'm thinking rate limiting isn't something a user would want as default since then we need to know what's a reasonable value? Or do we add it there only in the meantime?

Blerg! I think that needs to be a clearer name then. That method exposes what filters are available to be used from a configuration. Basically it registers them with the FilterRegistry (hence also the need for docs, i.e. "how do I write a filter") .Any filter we write that can be used from the YAML (and eventually gRPC) is registered with the FilterRegistry, so the system has somewhere to look for the list of available filters.

(As a side note, this is starting to feel like we really need some docs!!!)

Do we have plans/suggestions regarding what framework/format etc to use for docs we can start looking into (Not really versed with requirements to get a docs site up and running )?

I would start with a /docs folder and some .md files. If we need more complicated than that, then we can explore those options later.

Did we want to write an integration test for this in this PR, or will it be coming later?

tbh I didn't have any integration test plans for this mostly because I'm lazy but also didn't think an integration test adds something that we can't cover with a unit test since the functionality is limited to this module. Did you have something in mind?

Something simple that sends over the number of packets per second that a rate limit has, and it makes sure it only get the set amount would be nice.

I always like integration tests, as it gives extra confidence that the thing actually works as expected when in conjunction with the whole piece together. (I need to write one for the DebugFilter for instance, make sure it actually sends everything though as expected).

@iffyio
Copy link
Collaborator Author

iffyio commented Jul 10, 2020

Any filter we write that can be used from the YAML (and eventually gRPC) is registered with the FilterRegistry, so the system has somewhere to look for the list of available filters.

https://github.com/googleforgames/quilkin/blob/master/src/extensions/filter_registry.rs#L67
@markmandel FilterRegistry currently is implemented as a filter store, that's what I was referring to by needing to provide default values if we were to add a rate limiter to it because we need to create a new instance of the filter upfront.
I'm thinking what we want here is to register something that knows how to create the filter, given a configuration. rather than the filter itself.

@markmandel
Copy link
Contributor

https://github.com/googleforgames/quilkin/blob/master/src/extensions/filter_registry.rs#L67
@markmandel FilterRegistry currently is implemented as a filter store, that's what I was referring to by needing to provide default values if we were to add a rate limiter to it because we need to create a new instance of the filter upfront.
I'm thinking what we want here is to register something that knows how to create the filter, given a configuration. rather than the filter itself.

I missed much of the implementation code on first pass through, and I see this takes a config -- all making much more sense 👍

I've got a PR for lazily creating filters, with their configs incoming shortly, your point was extremely valid, and actually made me find a way to get configs passed to Filters - so thanks! Looking forward to your feedback on the approach.

src/config.rs Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Outdated Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Outdated Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Show resolved Hide resolved
src/extensions/filters/local_rate_limit.rs Show resolved Hide resolved
@markmandel
Copy link
Contributor

This looks great, the only things I was going to mention:

  1. Would be nice to configure which endpoint is going to be rate limited. This might be for a subsequent PR (we should note this on the issue) - but I leave that up to you.
  2. Can we mark this issue as "Work on Filter Idea: Rate limiting #5" - as it looks like there will be some follow up items, and we shouldn't close the ticket just yet.

Other than that, happy to merge.

@iffyio
Copy link
Collaborator Author

iffyio commented Jul 18, 2020

Can we mark this issue as "Work on #5" - as it looks like there will be some follow up items, and we shouldn't close the ticket just yet.

Not sure how github handles this, the only way I can find is to rebase and change the commit message which will be painful since I've merged from master into this branch.
If there isn't a clear way, I can update the message before squash merging into master or worst case reopen the issue if it gets closed after merging

@markmandel
Copy link
Contributor

If there isn't a clear way, I can update the message before squash merging into master or worst case reopen the issue if it gets closed after merging

I fixed it by editing the comment for you 😄 I'll respond to the rest on the ticket.

Copy link
Contributor

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

LGTM!

@markmandel markmandel merged commit 8b488a4 into master Aug 10, 2020
@markmandel markmandel deleted the iu/local-rate-limiter branch August 10, 2020 18:08
@markmandel markmandel added area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc kind/feature New feature or request labels Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc cla: yes kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants