NLnetLabs / domain

A DNS library for Rust.
https://nlnetlabs.nl/projects/domain/about/
BSD 3-Clause "New" or "Revised" License
341 stars 57 forks source link

Simple rate limiting by requests/second/client IP address #291

Closed ximon18 closed 5 months ago

ximon18 commented 5 months ago

Proof of concept for review and discussion.

Philip-NLnetLabs commented 5 months ago

The fundamental problem we need to solve is that new requests may come in faster than a service can handle them. Especially when the service is unexpectedly slow. I think the obvious solution is to configure some maximum on the total number of service invocations that can be running in parallel.

This PR has per IP address rate limits, which is nice be does not address the fundamental problem.

ximon18 commented 5 months ago

The easy quick approach to that is for the Service implementor to use a Tokio Semaphore to limit concurrent execution, though that would come after all request pre-processing has already been done.

It could be done at the server layer but when you have multiple servers (e.g. UDP and TCP and TLS and whatever) and you want a total limit then that is the wrong layer to apply the limit at.

Adding a limit to the lowest layer of middleware would allow it to be used across server instances as a single total limit, but currently preprocessing is sync only so a Tokio Semaphore can't be used and Rust core doesn't offer a sync semaphore type.

We could extend examples/server-transports.rs to show use of a Semaphore in the Service.

Alternatively, the code in this PR can be changed to use the governor but without IP basis, I'll update it to show that as it is v quick to do, and is then part of the middleware.

ximon18 commented 5 months ago

Alternatively, the code in this PR can be changed to use the governor but without IP basis, I'll update it to show that as it is v quick to do, and is then part of the middleware.

Though that is still not the same as limiting concurrent Service execution.

Philip-NLnetLabs commented 5 months ago

The important part is to limit the total number of service invocations that execute at a time.

ximon18 commented 5 months ago

That has to be done in the Service itself using a Semaphore as it is a future scheduled for execution at some unknown time in the future by the async runtime. It could be wrapped into say service_fn() to provide it as part of the library.

Philip-NLnetLabs commented 5 months ago

I don't see why it would be good to wait until service is called to implement rate limiting, and I don't quite see how that would work without blocking the entire system. Obviously, a service cannot use an async semaphore so it has to block the thread that is calling service.

It is not great interface design to hide stuff in service_fn that has to be implemented manually in other cases.