Add local rate limiting acceptance test #2932
Conversation
cthain
added
pr/no-changelog
| "http://localhost:1234/regex": 5, | ||
| } | ||
|
|
||
| opts := newRateLimitOptions(t, ctx) |
There was a problem hiding this comment.
Just a thought, but maybe it would be more straightforward to create an array of opts and add the addr/rate limit to each one? Would keep the configuration all in one place.
Then you could just loop on the opts
There was a problem hiding this comment.
Yeah that might be a nice update. My only concern with that is we would still have to do bits of the opts setup in the loop. I think I'll stick with the host/rps map for now.
|
|
||
| opts := newRateLimitOptions(t, ctx) | ||
|
|
||
| // Ensure that all requests from static-client to static-server succeed. |
There was a problem hiding this comment.
| // Ensure that all requests from static-client to static-server succeed. | |
| // Ensure that all requests from static-client to static-server succeed (no rate limiting set) |
| return &assertRateLimitOptions{ | ||
| resourceType: "deploy/", | ||
| successOutput: "hello world", | ||
| rateLimitOutput: "curl: (22) The requested URL returned error: 429", |
There was a problem hiding this comment.
I might even consider changing this to just contain 429 in case any part of this string were to change in the future, 429 would be the most likely part to stay consistent
There was a problem hiding this comment.
It's possible that this could change but I think it's unlikely 🤷. If curl changes its output format/messages we'll likely have to change things elsewhere. I followed what was used in the connhelper pkg for testing failed connections:
consul-k8s/acceptance/framework/connhelper/connect_helper.go
Lines 417 to 419 in 3d3bd70
| } | ||
| } | ||
|
|
||
| func assertRateLimits(t *testing.T, opts *assertRateLimitOptions, curlArgs ...string) { |
There was a problem hiding this comment.
Just an observation that I think this mostly just tests requestMaxBursts right? Do we want to add tests for throttling based on requestsPerSecond?
There was a problem hiding this comment.
Yes this test is bursty. It was pretty difficult to setup a reliable burst test and with the current framework I think it would be even more difficult to reliably test throttling. We're using curl via kubectl exec and everything has to happen in under 1 second because we don't allow tweaking the fill_interval.
I'm open to ideas if you have thoughts on how we could do this 🤔
Changes proposed in this PR:
This PR adds a local rate limiting acceptance test to the
connectfolder. It is a follow on to #2844.How I've tested this PR:
How I expect reviewers to test this PR:
👀
Checklist: