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

Feature request: rate limiting (limit_req_zone, limit_req) #1134

Closed
khaefeli opened this issue Oct 19, 2017 · 8 comments
Closed

Feature request: rate limiting (limit_req_zone, limit_req) #1134

khaefeli opened this issue Oct 19, 2017 · 8 comments
Labels
enhancement New feature or request

Comments

@khaefeli
Copy link

(D)DoS, brute force attacks etc are everywhere.
We want to protect our backends from excessive requests and load.
Nginx is supporting rate limiting in the ngx_http_limit_req_module module:
https://www.nginx.com/blog/rate-limiting-nginx/

Bring this feature as default to the puppet nginx core and help people to protect their backends.

@juniorsysadmin juniorsysadmin added the enhancement New feature or request label Oct 19, 2017
@wyardley
Copy link
Collaborator

wyardley commented Nov 3, 2017

I agree that we'll need a better way of handling modules at some point. I'm not sure that this should be the default configuration, though.
For now, installing the module itself may need to be handled outside of the module, though configuring its directives should be possible with the existing tooling.

@khaefeli
Copy link
Author

khaefeli commented Nov 6, 2017

sorry, I was a little bit inaccurate in my description:
support it with the puppet nginx module, but don't enable it by default. 

Actually, it's not a 3rd party module.
It's nginx core - so you just need to enable limit_req_zone in the http section and map the limit_req zone=mylimit in the location section. I could create a PR if you want.

@ubellavance
Copy link

You'd want to set a limit of connections per IP address by default? I don't think it is the role of a module to take care of that. By the way, I implemented it easily this way in a hiera location section:

...
  location_custom_cfg_append :
     limit_conn: addr 10;
     limit_conn_log_level: error;
...

But I agree that it could be better if the options were real options and not having to use location_custom_cfg*, but it is currently working.

@khaefeli
Copy link
Author

khaefeli commented Apr 23, 2018

no, that's limit_conn
I'm talking about limit_req, which is implemented a different way :)

@ubellavance
Copy link

ubellavance commented Apr 23, 2018

Ok, can you achieve it using a location_custom_cfg_append? But in any case, my definition of a puppet module is that it should allow the user do do a lot of stuff (ideally everything that the target service allows), but not change the default settings.

@khaefeli
Copy link
Author

khaefeli commented May 1, 2018

sure. you could link the zone to the location with location_custom_cfg_append (including options) and add the zones via server_cfg_append. I could also create some config files and include them. But that's more a bad workaround from my point of view.
currently there are ~120 server and ~110 location parameters. Why not supporting this important core option? :)

@ubellavance
Copy link

Hi @khaefeli,

I have nothing against the fact that this core feature should be supported. You even offered to create a PR, which is very positive. My only opinion is that limit_req should not be enabled by default by the module. But again, this is very good news, thanks!

@absltkaos
Copy link
Contributor

I submitted a PR, it doesn't enable request limiting by default, but it does expose it for configuring now.

bastelfreak added a commit that referenced this issue Nov 18, 2019
Add support for limit_req_zone in main nginx config and limit_req: Fixes #1134
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this issue Oct 19, 2020
Add support for `limit_req_zone` in main nginx config and `limit_req`
for `nginx::resource::location`.

In init.pp
`limit_req_zone` can be a String, or an array of String

In resource/location.pp
`limit_zone` can be a String and should point to a zone defined from
`limit_req_zone` in init.pp
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this issue Oct 19, 2020
Add support for limit_req_zone in main nginx config and limit_req: Fixes voxpupuli#1134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants