Add nginx rate limiting #188

scottlinux · 2017-10-25T15:18:26Z

codecov · 2017-10-25T15:32:25Z

Codecov Report

Merging #188 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #188   +/-   ##
=======================================
  Coverage   45.83%   45.83%           
=======================================
  Files           2        2           
  Lines          48       48           
  Branches        3        3           
=======================================
  Hits           22       22           
  Misses         25       25           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4822c8...b3511fe. Read the comment docs.

robled · 2017-11-16T16:57:53Z

cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_http.conf

@@ -1,3 +1,6 @@
+limit_req_zone $binary_remote_addr zone=jenkins:10m rate={{ nginx_request_limit }};


What does this '10m' number mean? Is this something that should be parameterized?

This value is the size of the rate limiting state cache for this rate limit zone in nginx. 10m comes to about 160,000 ip addresses. I don't see a need to parameterize this one but am up for it if that would be valuable.

I don't see a downside to parameterizing all the number values. It gives users an opportunity to tweak those values if needed, and even if they are never changed by anyone, having the values documented with comments in our vars files (with links to specific nginx docs) makes it easier for cinch users to understand what's going on.

robled · 2017-11-16T16:58:20Z

cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_http.conf

@@ -19,7 +22,11 @@ server {
    proxy_set_header        X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header        X-Forwarded-Proto $scheme;
    proxy_pass              http://jenkins;
-  }
+    limit_req zone=jenkins burst=100 nodelay;


What does this 'burst=100' number mean? Is this something that should be parameterized?

burst gives a little 'padding' by putting requests into a queue if one hits a limit. So here adding an additional 100 slot queue to lessen the impact of being rate limited. Seems to be recommended on nginx.com's blog and docs. Maybe we can parameterize this one.

I don't see a downside to parameterizing all the number values. It gives users an opportunity to tweak those values if needed, and even if they are never changed by anyone, having the values documented with comments in our vars files (with links to specific nginx docs) makes it easier for cinch users to understand what's going on.

robled · 2017-11-16T16:58:31Z

cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_https.conf

@@ -38,7 +41,11 @@ server {
    proxy_set_header        X-Forwarded-Proto $scheme;
    proxy_redirect http:// https://;
    proxy_pass              http://jenkins;
-  }
+    limit_req zone=jenkins burst=100 nodelay;


What does this 'burst=100' number mean? Is this something that should be parameterized?

robled · 2017-11-16T17:01:52Z

cinch/roles/jenkins_master/defaults/main.yml

@@ -245,6 +245,17 @@ firewall_tcp_ports:
  - "22/tcp"
  - "80/tcp"
  - "{{ https_enabled | ternary('443/tcp', omit) }}"
+# The following values are used in nginx to prevent undesired impact of the


I think there is a use case for optionally setting all these values to nginx defaults, for the purposes of debugging or testing without any rate limiting in the mix. Can we parameterize this in such a way that all rate limiting is turned off based on some flag? Or alternatively, we could document in the comments what the default values are, and from there a user could set those to turn off the rate limiting. Ideally we could auto-inherit the defaults from the nginx upstream configuration.

Agreed on this comment, across the board.

It appears there are no default nginx values for these, curiously enough. Where applicable I am using info from nginx.com/blog posts about rate limit features, and also things I have tested.

I will make rate limiting opt-in only and document values in place if users want to customize from suggested values.

scottlinux · 2017-11-16T18:48:41Z

Thanks - good points all, let me work on this.

robled · 2018-01-08T15:40:18Z

cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_http.conf

+    limit_rate_after {{ nginx_bandwidth_limit_after_size }};
+    limit_rate       {{ nginx_max_bandwidth_outbound }};
+    {% endif %}
+	}


Is the indentation level here correct?

robled · 2018-01-08T15:40:31Z

cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_https.conf

+    limit_rate_after {{ nginx_bandwidth_limit_after_size }};
+    limit_rate       {{ nginx_max_bandwidth_outbound }};
+    {% endif %}
+	}


Is the indentation level here correct?

Thanks - should be fixed now if you can double check me

robled · 2018-01-08T15:40:54Z

cinch/roles/jenkins_master/defaults/main.yml

+# Limit maximum number of simultaneous connections per ip:
+nginx_max_connections_per_ip: 25
+# Enable bandwidth limiting for files this size or larger:
+nginx_bandwidth_limit_after_size: "100m"


What unit is 'm'?

robled · 2018-01-08T15:41:03Z

cinch/roles/jenkins_master/defaults/main.yml

+# Enable bandwidth limiting for files this size or larger:
+nginx_bandwidth_limit_after_size: "100m"
+# Set bandwidth limit for downloading large files:
+nginx_max_bandwidth_outbound: "5m"


What unit is 'm'?

scottlinux · 2018-01-08T19:25:31Z

Ok updated comments should be resolved now. I likely made a mess of this PR via multiple amends. :/

robled · 2018-01-08T20:25:01Z

May have missed the indentation fix here:

https://github.com/scottlinux/cinch/blob/5616178d5eecb6ab62bdfd8e36866028d3bb12aa/cinch/roles/jenkins_master/templates/etc/nginx/conf.d/jenkins_https.conf#L52

For RedHatQE#143

greg-hellings · 2018-01-09T20:56:25Z

Let's please remember to add this to the CHANGELOG file before the next release that it's included in.

robled · 2018-01-09T21:26:57Z

I'll take care of the CHANGELOG.

robled suggested changes Nov 16, 2017

View reviewed changes

scottlinux force-pushed the nginx-rate-limit branch 2 times, most recently from fb04bd7 to 27dead1 Compare January 5, 2018 20:44

robled reviewed Jan 8, 2018

View reviewed changes

scottlinux force-pushed the nginx-rate-limit branch 2 times, most recently from eab1d80 to 5616178 Compare January 8, 2018 18:54

Add nginx rate limiting

b3511fe

For RedHatQE#143

scottlinux force-pushed the nginx-rate-limit branch from 5616178 to b3511fe Compare January 9, 2018 13:22

robled approved these changes Jan 9, 2018

View reviewed changes

greg-hellings approved these changes Jan 9, 2018

View reviewed changes

robled merged commit 925f0f7 into RedHatQE:master Jan 9, 2018

scottlinux mentioned this pull request Jan 11, 2018

Add nginx rate limiting #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nginx rate limiting #188

Add nginx rate limiting #188

scottlinux commented Oct 25, 2017

codecov bot commented Oct 25, 2017 •

edited

Loading

robled Nov 16, 2017

scottlinux Jan 5, 2018

robled Jan 5, 2018

robled Nov 16, 2017

scottlinux Jan 5, 2018

robled Jan 5, 2018

robled Nov 16, 2017

robled Nov 16, 2017

greg-hellings Nov 22, 2017

scottlinux Jan 5, 2018

scottlinux commented Nov 16, 2017

robled Jan 8, 2018

robled Jan 8, 2018

scottlinux Jan 9, 2018

robled Jan 8, 2018

robled Jan 8, 2018

scottlinux commented Jan 8, 2018

robled commented Jan 8, 2018

greg-hellings commented Jan 9, 2018

robled commented Jan 9, 2018

		@@ -1,3 +1,6 @@
		limit_req_zone $binary_remote_addr zone=jenkins:10m rate={{ nginx_request_limit }};

Add nginx rate limiting #188

Add nginx rate limiting #188

Conversation

scottlinux commented Oct 25, 2017

codecov bot commented Oct 25, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlinux commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlinux commented Jan 8, 2018

robled commented Jan 8, 2018

greg-hellings commented Jan 9, 2018

robled commented Jan 9, 2018

codecov bot commented Oct 25, 2017 •

edited

Loading