feat: Add client-side SDK rate limiting via --sdk-max-tps flag - #264
feat: Add client-side SDK rate limiting via --sdk-max-tps flag#264NicholasBlaskey wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NicholasBlaskey The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @NicholasBlaskey. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ack-prow
Bot
added
the
needs-ok-to-test
| 0, | ||
| "Maximum AWS SDK requests per second (client-side rate limit). Set to 0 to disable. "+ | ||
| "Useful for services with low account-level API rate limits (e.g. Route53 at 5 req/s). "+ | ||
| "Can also be set via ACK_SDK_MAX_TPS environment variable.", |
There was a problem hiding this comment.
where do we consume the environment variable?
There was a problem hiding this comment.
removed these. My mistake I thought the flag library was like vulcan and automatically converted these.
| if burst <= 0 { | ||
| burst = int(c.cfg.SDKMaxTPS) + 1 |
There was a problem hiding this comment.
not sure if this is okay..if burst is a negative number it may still remain negative. Can we instead validate these flags here: https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/config/config.go#L344
| limiter *rate.Limiter | ||
| } | ||
|
|
||
| func (c *rateLimitedHTTPClient) Do(r *http.Request) (*http.Response, error) { |
There was a problem hiding this comment.
is the rate limiting specific per API? (eg. create and read have separate token buckets)
There was a problem hiding this comment.
Right now its specific to client
To be fair, for Route53 the read and write APIs take out of the same quota but yes for other services it might be per write or per API
Adds a configurable client-side rate limiter that wraps the AWS SDK HTTP client with a token bucket. This enforces a maximum request rate (TPS) across all AWS API calls, preventing controllers from exceeding service- level rate limits. New flags: --sdk-max-tps=N Maximum requests per second (0 = disabled, default) --sdk-max-burst=N Burst size for rate limiter (default: 5) Also configurable via environment variables: ACK_SDK_MAX_TPS ACK_SDK_MAX_BURST Use case: services like Route53 enforce a hard 5 req/s account-level limit shared across all API actions. Setting --sdk-max-tps=3 ensures the controller never consumes more than 3 req/s, leaving headroom for other consumers (external-dns, Terraform, etc.) sharing the same account. The rate limiter applies to all AWS API calls from the controller's SDK client (the primary service), but not to STS calls for credential refresh which use a separately constructed client.
e2dfa30 to
d95140d
Compare
2 participants
Issue #, if available:
Description of changes:
Adds a configurable client-side rate limiter that wraps the AWS SDK HTTP client with a token bucket. This enforces a maximum request rate (TPS) across all AWS API calls, preventing controllers from exceeding service- level rate limits.
New flags:
--sdk-max-tps=N Maximum requests per second (0 = disabled, default)
--sdk-max-burst=N Burst size for rate limiter (default: 5)
Use case: services like Route53 enforce a hard 5 req/s account-level limit shared across all API actions. Setting --sdk-max-tps=3 ensures the controller never consumes more than 3 req/s, leaving headroom for other consumers (external-dns, Terraform, etc.) sharing the same account.
The rate limiter applies to all AWS API calls from the controller's SDK client (the primary service), but not to STS calls for credential refresh which use a separately constructed client.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Tested and verified Route53 runs with no more than 3 TPS with a 3 limit