AaronErhardt/actix-governor

const_per_second missing

Pat-1337 opened this issue · 11 comments

// Create governor config with recovery time and burst size.
fn create_config(
    s: u64,
    bs: u32,
) -> GovernorConfig<RateLimit, NoOpMiddleware<QuantaInstant>> {
    GovernorConfigBuilder::default()
        .key_extractor(RateLimit)
        .const_per_second(s)
        .burst_size(bs)
        .finish()
        .unwrap()
}
error[E0599]: no method named `const_per_second` found for struct `GovernorConfigBuilder<RateLimit, NoOpMiddleware<QuantaInstant>>` in the current scope
  --> src/lib.rs:50:10
   |
50 |         .const_per_second(s)
   |          ^^^^^^^^^^^^^^^^ help: there is a method with a similar name: `per_second`

For more information about this error, try `rustc --explain E0599`.

Was it removed or am I missing something? It says it's still available in v0.4 in docs.
I am trying to rate limit register endpoint to 2 successful requests in 10 minutes.

fn users() -> actix_web::Scope {
    let mut sc = scope("/api/v1/users");
    let governor_conf = create_config(600, 2);

    sc = sc.service(
        resource("")
            .route(get().to(users::all))
            .route(post().to(users::register))
            .wrap(Governor::new(&governor_conf)),
    );
 ...
[dependencies]
actix-governor = "0.4.0-beta.3"

This is not a regression, the const methods are only implemented when using PeerIpKeyExtractor:

impl<M: RateLimitingMiddleware<QuantaInstant>> GovernorConfigBuilder<PeerIpKeyExtractor, M> { ... }

In your case (and most other scenarios), there's no real benefit of using const anyway.
Also, you might consider upgrading to 0.4.0, even if the code hasn't changed significantly since the beta version.

Thanks for the answer, mind if I do a follow up, after doing the conf for my users route with 600 sec and 2 burst window, I get 2 requests in as it should be, get the message that I need to wait another 600 seconds but it stays that way for another 10 seconds or so, but after that, I get to send request again without timeout, if someone is spamming the route, they can just wait 10 seconds and they won't be rate limited again even tho message clearly says ~600 seconds are left
image

Is that expected behavior?

No, that sounds like a bug. We should definitely investigate this.

Alright, if you need any extra info, let me know

Could you share your rate-limit config? A small example that reproduces the problem would also be great.

The code is exactly the same as one in original question

Helper function that creates config

// Create governor config with recovery time and burst size.
fn create_config(
    s: u64,
    bs: u32,
) -> GovernorConfig<RateLimit, NoOpMiddleware<QuantaInstant>> {
    GovernorConfigBuilder::default()
        .key_extractor(RateLimit)
        .const_per_second(s)
        .burst_size(bs)
        .finish()
        .unwrap()
}

RateLimit is extracting cookie / auth header, I checked, keys are fine, it works

Endpoints in lib.rs:

pub fn configure_app(cfg: &mut web::ServiceConfig) {
    cfg.service(healthcheck());
    cfg.service(users());
...
    cfg.service(client());
}
fn users() -> actix_web::Scope {
    let mut sc = scope("/api/v1/users");
    let governor_conf = create_config(600, 2);

    sc = sc.service(
        resource("")
            .route(get().to(users::all))
            .route(post().to(users::register))
            .wrap(Governor::new(&governor_conf)),
    );

    sc = sc.service(
        resource("/me")
            .route(get().to(users::me)),
    );

    sc = sc.service(
        resource("/{user_id}")
            .route(patch().to(users::user::modify))
            .route(get().to(users::user::get)),
    );
...

So it turns out that it's not a bug of actic_governor but rather actix calls the configure_app method once for each thread. Therefore, the middleware is initialized multiple times which makes it possible to get more request through before each rate limiter blocks individually.

is there a way to do it with the configure_app or i need to use different library for that?

As you can see in the example in the README, it is possible to make sure that only one rate-limiter is created for all threads by initializing in the main function. Apart from that, you should also be able to use once_cell::sync::Lazy to create a GovernorConfig so only one config is created.

@AaronErhardt thanks, I used lazy_static for my project, one more question, is it possible to ignore OK responses for the ratelimit? I am implementing register endpoint where I could have lots of 409 conflict responses for same username, but I dont want to limit those users from registering, is there a way to implement that with this library?

Technically, it might be possible, but with the underlying governor crate this will be quite tough to implement. I suggest adding another endpoint for checking for existing usernames which sounds like a much better solution to me regardless of rate-limiting.