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

limit_zone should take Array in addition to String and Undef #1569

Closed
ltning opened this issue Jul 18, 2023 · 1 comment · Fixed by #1570
Closed

limit_zone should take Array in addition to String and Undef #1569

ltning opened this issue Jul 18, 2023 · 1 comment · Fixed by #1570

Comments

@ltning
Copy link
Contributor

ltning commented Jul 18, 2023

Affected Puppet, Ruby, OS and module versions/distributions

  • Puppet: v6, v7
  • Ruby: 3.1
  • Distribution: ?
  • Module version: Any

How to reproduce (e.g Puppet code you use)

limit_zone:
    - 'remote_ip_20 burst=250'
    - 'request_line_20 burst=50'

What are you seeing

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Nginx::Resource::Location[.....]: parameter 'limit_zone' expects a value of type Undef or String, got Tuple (file: /usr/local/etc/puppet/modules/nginx/manifests/resource/server.pp, line: 637) on node ......

What behaviour did you expect instead

Multiple limit_req statements in resulting nginx configuration

Any additional information you'd like to impart

From https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req:

 There could be several limit_req directives. For example, the following configuration will limit the processing rate of requests coming from a single IP address and, at the same time, the request processing rate by the virtual server:

    limit_req_zone $binary_remote_addr zone=perip:10m rate=1r/s;
    limit_req_zone $server_name zone=perserver:10m rate=10r/s;

    server {
        ...
        limit_req zone=perip burst=5 nodelay;
        limit_req zone=perserver burst=10;
    }
@ltning
Copy link
Contributor Author

ltning commented Jul 18, 2023

Suggested patch against 3.3.0:

diff --git a/modules/nginx/manifests/resource/location.pp b/modules/nginx/manifests/resource/location.pp
index 4d7e245c..b7ac3116 100644
--- a/modules/nginx/manifests/resource/location.pp
+++ b/modules/nginx/manifests/resource/location.pp
@@ -265,7 +265,7 @@ define nginx::resource::location (
   Boolean $ssl                                                     = false,
   Boolean $ssl_only                                                = false,
   Optional[String] $location_alias                                 = undef,
-  Optional[String[1]] $limit_zone                                  = undef,
+  Optional[Variant[String[1],Array[String[1],1]]] $limit_zone      = undef,
   Optional[Enum['any', 'all']] $location_satisfy                   = undef,
   Optional[Array] $location_allow                                  = undef,
   Optional[Array] $location_deny                                   = undef,
diff --git a/modules/nginx/templates/server/location_header.erb b/modules/nginx/templates/server/location_header.erb
index 7e526d11..f8c84c81 100644
--- a/modules/nginx/templates/server/location_header.erb
+++ b/modules/nginx/templates/server/location_header.erb
@@ -78,7 +78,13 @@
     <%- end -%>
 <% end -%>
 <% if @limit_zone -%>
+  <%- if @limit_zone.is_a?(Array) then -%>
+    <%- for lz in @limit_zone -%>
+    limit_req zone=<%= lz %>;
+    <%- end -%>
+  <%- else -%>
     limit_req zone=<%= @limit_zone %>;
+  <%- end -%>
 <% end -%>
 <% if @reset_timedout_connection -%>
   reset_timedout_connection <%= @reset_timedout_connection %>;

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

Successfully merging a pull request may close this issue.

2 participants