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

Make ping rate limit configurable #108

Merged

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Feb 22, 2024

I'm dealing with some gRPC servers (written in golang) that routine exceed the limit of 4 pings/sec (increasing the limit to 10 seems to do the trick). This PR makes the ping rate limit configurable.

@edsko edsko force-pushed the edsko/configurable-ping-rate-limit branch 4 times, most recently from d119d3a to 11a166c Compare February 22, 2024 15:32
@@ -41,12 +43,13 @@ baseSettings =
, initialWindowSize = defaultWindowSize -- 64K (65,535)
, maxFrameSize = defaultPayloadLength -- 2^14 (16,384)
, maxHeaderListSize = Nothing
, pingRateLimit = 4
Copy link
Owner

Choose a reason for hiding this comment

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

If you think 4 is too small, you can increase the value.

@edsko
Copy link
Contributor Author

edsko commented Feb 27, 2024

@kazu-yamamoto , you asked a difficult question 😅

If you think 4 is too small, you can increase the value.

I didn't change the default as I wasn't sure what to change it to, and I wasn't comfortable suggesting something. But since you asked, I spent some time trying to see what other people do. Unfortunately, it is surprisingly difficult to find some concrete guidelines about this. Plenty of sites that tell you "one way to mitigate a ping flood attack is to impose rate limits", but what those rate limits should be, nobody seems to want to say.

A summary of what I found:

Alternative approaches

It seems most applications impose rate limits in a different manner than http2 currently does. Some examples:

  1. Limit on the maximum number of control frames per connection (rather than per second). Examples:

  2. Compare overall traffic to traffic on the individual streams.

    • For example, in nginx: "as long as total traffic is many times larger than stream traffic, we consider this to be a flood" (patch).
  3. Limit outbound control packets (the response to the ping, as opposed to the ping itself).

  4. Impose a minimum time between control packets

  5. Most relevant for my own needs, since I am working on a gRPC library: the gRPC Keepalive guide defines a server parameter PERMIT_KEEPALIVE_TIME which sets the "minimum allowed time between a server receiving successive ping frames without sending any data/header frame" and defaults to 5 minutes -- so this is some hybrid of (2) and (4).

Actual ping rate limiting

For actual rate limiting, I only found two suggestions:

Underlying cause of the many pings

I suspect that the reason I am seeing more pings than the 4/second that http2 currently allows is somehow related to the rate of requests (for example -- speculating -- perhaps some logic is a bit broken and new requests can cause a new keepalive ping to be sent, irrespective of when the previous one was sent). If true, that would mean that any specific value of pings/second is potentially problematic. I'd have to start digging through the golong source of the server to know for sure, but it at least feels plausible as a client "broken" in this way would still work with the recommendation from the guide on rate limiting (option (5), above) -- even if it doesn't quite explain everything, at least not when compared to the guidelines (but that is no surprise in the gRPC ecosystem, unfortunately) -- it certainly doesn't seem to match the documented behaviour of the golang client 🤷

Conclusion

I'm not sure what to suggest. Quite frankly, I made the ping rate limit a parameter in my library too, defaulting to 10/second for now, but of course that is basically passing the bucket further down the line to the application 😁 I agree with you that sensible defaults would be better.

Option (5) from the gRPC guidelines -- that is, imposing a minimum time between successive ping frames without sending data/header frames -- does seem quite sensible, not just for gRPC but also for other applications. If you want, I can try to implement that and see if it works for me, although of course that would be a bigger change to http2 than the current PR.

Alternatively, I could imagine some kind of pluggable "rate limiting API" which could be instantiated to any of the 5 options above; I'd have to spend some time thinking about what such an API should look like.

Let me know what you'd like me to do.

@kazu-yamamoto
Copy link
Owner

@edsko Thank you for this research!

Perhaps, it is easy for many programmers to measure time between packets and to count the number of a specific frame but it is hard to implement rate limiting (number of packets per second).
It took some days for me to hit upon the idea on how to implement this rate limiting.
Yes, it looks very easy if you read the code but it would be hard to implement it without reference code.

I'm not sure it's worth exploring other approaches.
I would suggest to just increase the value from 4 to 10 with the configurable scheme.

@edsko edsko force-pushed the edsko/configurable-ping-rate-limit branch from 11a166c to 4389142 Compare February 28, 2024 16:22
@edsko
Copy link
Contributor Author

edsko commented Feb 28, 2024

Ok, I've discussed this with my client and we're okay with this approach. I've force-pushed a commit that changes the default to 10 (and still keeps it configurable).

@edsko edsko force-pushed the edsko/configurable-ping-rate-limit branch from 4389142 to ac3e68e Compare February 28, 2024 16:26
@edsko
Copy link
Contributor Author

edsko commented Feb 28, 2024

Also changed the test suite so that the ping protection test is a bit more aggressive so that it still exceeds the limit.

Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

LGTM

@kazu-yamamoto kazu-yamamoto merged commit 4478e6e into kazu-yamamoto:main Feb 29, 2024
10 checks passed
@kazu-yamamoto
Copy link
Owner

Merged.
Thank you for your contributions!
A new version has been released.

@edsko edsko deleted the edsko/configurable-ping-rate-limit branch February 29, 2024 05:38
@edsko
Copy link
Contributor Author

edsko commented Feb 29, 2024

Thanks @kazu-yamamoto !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants