Add rate limiter config and integration test #81
Conversation
* Make period optional, default to 1sec. * Reject configs with period < 100ms since we can't really guarantee correctness at lower resolutions. Work on #5
| } | ||
|
|
||
| /// A filter that implements rate limiting on packets based on | ||
| /// the token-bucket algorithm. |
There was a problem hiding this comment.
Now we have a config, can we add a configuration example like we do here: https://github.com/googleforgames/quilkin/blob/master/src/extensions/filters/debug_filter.rs#L28-L45 ?
I think it would be nice for our users 😄
| let mut buf = vec![0; 1024]; | ||
| let (size, sender) = socket.recv_from(&mut buf).await.unwrap(); | ||
| socket.send_to(&buf[..size], sender).await.unwrap(); | ||
| loop { |
There was a problem hiding this comment.
Do we need a way to shut this down at the end of a test? (we do this for other services)
There was a problem hiding this comment.
I initially thought to check any error here and break but that wasn't really needed since the server is running for the duration of the test and we don't expect it to fail so unwrapping should be okay.
Shutting down would likely be to have tests signal this loop to exit and drop the RecvHalf socket - since the loop is running for the duration of the test either way, I'm don't think an explicit shutdown adds much given the extra code it'll add to tests that use echo_server.
There was a problem hiding this comment.
Created #84 for this.
| /// local: | ||
| /// port: 7000 # the port to receive traffic to locally | ||
| /// filters: | ||
| /// - name: quilkin.extensions.filters.local_rate_limit.alphav1.LocalRateLimit |
There was a problem hiding this comment.
I didn't notice this before - but we should probably come up with a documented standard for filters?
The debug filter is: "quilkin.core.v1alpaha1.debug"
Envoy has a pattern that looks more like: "envoy.extensions.filters.http.tap.v3.Tap" (link) - which is akin to what you have.
I don't have a huge opinion on what the standard is, but we should probably document this somewhere, yeah?
There was a problem hiding this comment.
Added a comment mentioning a naming format, it follows the directory layout we're using for filters
| @@ -73,7 +73,7 @@ impl DebugFilterFactory { | |||
|
|
|||
| impl FilterFactory for DebugFilterFactory { | |||
| fn name(&self) -> String { | |||
| return String::from("quilkin.core.v1alpaha1.debug"); | |||
| return String::from("quilkin.core.v1alpha1.debug"); | |||
There was a problem hiding this comment.
if we're going to fix my typo, should probably change the whole thing.
| return String::from("quilkin.core.v1alpha1.debug"); | |
| return String::from("quilkin.extensions.filter.debug_filter.v1alpha1.DebugFilter"); |
(with update if we settle on filters vs filter)
There was a problem hiding this comment.
missed that, fixed!
markmandel
added
area/tests
really guarantee correctness at lower resolutions.
Work on #5