Populate request headers in shouldRateLimit rpc response#1168
Open
yusofg2 wants to merge 15 commits into
Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Adds RateLimitRequestHeadersEnabled and three HeaderRequestRatelimit* fields to Settings, mirroring the existing response-header block, so the service can later inject rate-limit headers into upstream requests. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add four requestHeaders* fields to the service struct and populate them from settings in SetConfig, mirroring the existing customHeaders pattern for response headers. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Assign rlSettings.RateLimitRequestHeadersEnabled unconditionally so that a hot-reload with LIMIT_REQUEST_HEADERS_ENABLED unset/false properly clears the flag. Previously the if-only guard meant the field could only ever transition from false → true, making the disable path a no-op. Also adds a white-box test (src/service/ratelimit_test.go) that directly exercises the SetConfig toggle path and a black-box test stub in test/service/ratelimit_test.go that was superseded by the white-box test.
When LIMIT_REQUEST_HEADERS_ENABLED is true, attach RateLimit-Limit, RateLimit-Remaining, and RateLimit-Reset as RequestHeadersToAdd on the response, mirroring the existing ResponseHeadersToAdd logic. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add 3 test functions after TestServiceWithDefaultRequestHeaders covering custom header names, within-limit behaviour, and simultaneous request+response headers with different names. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Assign customHeadersEnabled unconditionally from rlSettings, matching the existing pattern for requestHeadersEnabled. Previously the field was only ever set to true inside an if-block, so a hot-reload with LIMIT_RESPONSE_HEADERS_ENABLED=false had no effect and response headers continued to be added indefinitely.
Mirrors the existing rateLimitLimitHeader/rateLimitRemainingHeader/rateLimitResetHeader pattern used for ResponseHeadersToAdd. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Relative paths (./examples/...) resolve relative to the compose file
location (integration-test/) rather than the repo root, causing mount
failures. Use ${PWD} consistently, matching the existing ratelimit and
tester service mounts.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ation tests - Add access log format to envoy-mock that logs RateLimit-* request headers, allowing end-to-end verification that Envoy injects request_headers_to_add onto the upstream request - Enable LIMIT_REQUEST_HEADERS_ENABLED in integration test compose Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Remove duplicate pb_struct import alias in src/service/ratelimit_test.go; use existing ratelimitv3 alias throughout - Add comment to examples/envoy/mock.yaml noting the hardcoded default header names and how to update them if custom LIMIT_REQUEST_*_HEADER env vars are used - Remove local absolute paths from implementation plan docs Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The service that uses global rate limits does not have visibility into rate limiting buckets. Some services require the knowledge of rate limit quota usage.
Use-case
A service sets up rate limits in shadow mode and needs to serve requests that are over limit differently than regularly admitted requests.
Solution
The
shouldRateLimitAPI definition already includesrequest_headers_to_addin the response to be forwarded to the upstream service but the ratelimit service does not populate the field. We implement population of those headers very similar to the existingresponse_headers_to_add.The new configuration parameters are listed in README.md while the design specification and implementation plan are included under
docs/superpowers.In the implementation, a minor bug related to config reload when response headers are disabled was found and fixed in commit 85fb09a.
Testing
For local integration testing:
LIMIT_REQUEST_HEADERS_ENABLEDenabled.Here is a sample output:
TBD