-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
implement basic DDOS protections in the HAProxy template router #9810
Conversation
@@ -226,6 +226,28 @@ backend be_edge_http_{{$cfgIdx}} | |||
{{ else }} | |||
balance leastconn | |||
{{ end }} | |||
|
|||
{{ if index $cfg.Annotations "router.openshift.io/haproxy.DDOS" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use uppercase in annotations. I don't know that these annotations have anything to do with DDOS, they're just optional fields, so they should be named appropriately.
The top level flag should be named appropriately, probably haproxy.router.openshift.io/rate-limit-connections
and the others should follow that pattern with sub names.
@openshift/networking @knobunc |
LGTM @rajatchopra PTAL |
e805acb
to
c8747d9
Compare
We need a test case for this. Set connection rate limit to 3 and open 4th connection. extended tests may be the right place still. |
@@ -231,6 +231,29 @@ backend be_edge_http_{{$cfgIdx}} | |||
timeout server {{$value}} | |||
{{ end }} | |||
{{ end }} | |||
|
|||
{{ if index $cfg.Annotations "router.openshift.io/haproxy.rate-limit-connections" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the haproxy.router.openshift.io/
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the same format for annotations as Rajat (line 222). If I change mine the naming will be inconsistent, should I change Rajat's as well? @rajatchopra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (router-name).router.openshift.io/(feature) sounds better scoped. @JacobTanenbaum change the one that I wrote too then maybe (will need testcase changes too). Thanks.
c8747d9
to
7662831
Compare
[Test] |
Using annotations implement basic DDOS protections in the HAProxy router - ability to limit the number of concurrent TCP connections - limit the rate at which a client can request TCP connections - limit the rate at which HTTP requests can be made These are enabled on a per route bases because apps could have extremely different traffic patterns setting "router.openshift.io/haproxy.DDOS" enables the settings to be configured setting "router.openshift.io/haproxy.DDOS.concurrentTCP" changes the number concurrent TCP connections that can be made by the same IP address on this route. setting "router.openshift.io/haproxy.DDOS.rateTCP" changes the number of TCP connections that can be opened by a client IP. setting "router.openshift.io/haproxy.DDOS.rateHTTP" changes the number of HTTP requests that a client IP can make in a 3s period. Since traffic patterns can vary wildly I provided no default values for these protections If the DDOS conditions are violated the router closes the connection to the client.
7662831
to
2b4703a
Compare
Evaluated for origin test up to 2b4703a |
LGTM. @rajatchopra if you like it, I think we are good to merge. |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6812/) |
LGTM. But we need a test somewhere. [merge], the test looks flaky. |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6947/) (Image: devenv-rhel7_4671) |
Evaluated for origin merge up to 2b4703a |
Using annotations implement basic DDOS protections in the HAProxy router
These are enabled on a per route bases because apps could have extremely
different traffic patterns
setting "router.openshift.io/haproxy.DDOS" enables the settings to be
configured
setting "router.openshift.io/haproxy.DDOS.concurrentTCP" changes the
number concurrent TCP connections that can be made by the same IP
address on this route.
setting "router.openshift.io/haproxy.DDOS.rateTCP" changes the number of
TCP connections that can be opened by a client IP.
setting "router.openshift.io/haproxy.DDOS.rateHTTP" changes the number
of HTTP requests that a client IP can make in a 3s period.
Since traffic patterns can vary wildly I provided no default values for
these protections
If the DDOS conditions are violated the router closes the connection to
the client.