BZ 1368050: add ability to change connection limits for reencrypt and… #10513
Conversation
|
@knobunc PTAL |
|
@ramr @rajatchopra PTAL |
|
If one were to just put 'haproxy.router.openshift.io/rate-limit-connections' as TRUE, but not provide the actual limits, the stick table kind of just hangs busy with no real use. I am wondering how do we document this stuff. Looks good otherwise. We should work on a test for this stuff also. |
|
Added trello card for documentation and testing. I couldn't think of a better flow for the this code, because up to three different settings can use the table and an application may care about any combination of them. |
|
@JacobTanenbaum I am fine with the current flow. We are good to go since we have the documentation TODO in the queue. Thanks. |
|
LGTM [merge] |
|
[Test]ing while waiting on the merge queue |
| #TCP connection rate not restricted | ||
| {{ end }} | ||
|
|
||
| {{ if (isInteger (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections.rate-http")) }} |
There was a problem hiding this comment.
haproxy.router.openshift.io/rate-limit-connections.rate-http is not needed for pass through routes (since we don't parse/decipher the http request - http req rate will always be 0).
Edited this to haproxy.router.openshift.io/rate-limit-connections.rate-http for clarity.
There was a problem hiding this comment.
this is the be_secure block, which is reencrypt. It looks like it's now not present in the passthrough section.
|
[test] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8234/) (Image: devenv-rhel7_4882) |
… passthrough routes Extend the DDoS protection annotations from PR9810 to extend to reencrypt and passthrough routes https://bugzilla.redhat.com/show_bug.cgi?id=1368050
JacobTanenbaum
force-pushed
the
BZ1368050
branch
from
92a93ef to
43c0285
Compare
|
Evaluated for origin test up to 43c0285 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8234/) |
|
LGTM |
|
@ramr are you willing to tag for merge? |
| @@ -328,6 +328,23 @@ backend be_tcp_{{$cfgIdx}} | |||
| timeout tunnel {{$value}} | |||
| {{ end }} | |||
| {{ end }} | |||
|
|
|||
| {{ if matchPattern "true|TRUE" (index $cfg.Annotations "haproxy.router.openshift.io/rate-limit-connections") }} | |||
There was a problem hiding this comment.
Nit (should not block merge): indentation appears off here (should match the same level as immediately above)
|
I think this LGTM as well 👍 |
|
[merge] |
|
Evaluated for origin merge up to 43c0285 |
… passthrough routes
Extend the DDoS protection annotations from PR9810 to extend to
reencrypt and passthrough routes
Bug 1368050