VOIP-1302-Redis-customer-rate-limit - #1179
Merged
Merged
Conversation
- bin-api-manager: Add pkg/ratelimithandler wrapping redis_rate/v9 behind a local RateLimiter interface - bin-api-manager: Add mockgen-generated mock and miniredis-backed integration test - bin-api-manager: Add redis_rate and miniredis dependencies to go.mod/go.sum
- bin-api-manager: Split middleware.Authenticate() into Authenticate() + EnforceAccountStatus() so v1 route group can rate-limit before the frozen-account RPC check - bin-api-manager: Add middleware.CustomerRateLimit() enforcing per-customer GCRA quotas (tiers v1_customer, v1_customer_direct, v1_customer_delegate) keyed by customer_id, applied only to the v1 route group - bin-api-manager: Add Retry-After header and details[0].limited_by to both the IP-based and customer-based 429 responses, using Reserve()+Cancel() on the IP limiter to avoid double-consuming budget on rejection - bin-api-manager: Add nil-CustomerID warning logs to the AuthIdentity constructors in models/auth/auth.go (log only, no behavior/signature change) - bin-api-manager: Add RATE_LIMIT_CUSTOMER_V1_*/_DIRECT_*/_DELEGATE_*/_REDIS_TIMEOUT_MS config fields with VOIP-1302 initial values - bin-api-manager: Wire a dedicated rate-limiter Redis client in cmd/api-manager/main.go (runDaemon), separate from pkg/cachehandler, with its own Close() lifecycle
- bin-api-manager: Update docsdev/source/architecture_security.rst rate limiting table with v1_customer/v1_customer_direct/v1_customer_delegate tiers, remove stale VOIP-1302-not-implemented footnote - bin-api-manager: Update docsdev/source/restful_api_errors.rst RATE_LIMIT_EXCEEDED entry with Retry-After header and details[0].limited_by field - bin-api-manager: Update docsdev/source/common_overview.rst customer-facing rate limit section to describe the per-customer global quota and Retry-After handling - bin-api-manager: Update docs/architecture.md middleware stack section for the Authenticate()/EnforceAccountStatus() split and CustomerRateLimit() ordering - bin-api-manager: Update docs/operations.md config table with the new RATE_LIMIT_CUSTOMER_* env vars - bin-api-manager: Clean rebuild docsdev/build/ HTML output
- bin-api-manager: math.Round(rps) could produce a GCRA Rate of 0 for 0 < rps < 0.5, sending redis_rate's Lua script into an undefined division-by-zero state instead of a well-defined restrictive limit. Clamp the rounded rate to a minimum of 1. Found in code review round 1 (MEDIUM).
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
1 participant
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.
Add Redis-backed per-customer rate limit quotas alongside the existing per-IP
tiers, keyed on customer_id after JWT/accesskey authentication, with
Retry-After support and updated RST/service docs. Design finalized through
6 rounds of independent architecture review and 3 rounds of code review
(general + security), all APPROVE, no CRITICAL/HIGH findings.
go-redis/redis_rate/v9 (GCRA) behind a local RateLimiter interface with a
mockgen mock and a miniredis integration test
v1_customer (agent/accesskey, shared bucket), v1_customer_direct, and
v1_customer_delegate, keyed ratelimit:{tier}:{customer_id}; skips on nil
CustomerID; fails open with a 50ms Redis timeout
EnforceAccountStatus() so CustomerRateLimit can run before the frozen-
account RPC check on the v1 group only; authProtected keeps its original
Authenticate()->EnforceAccountStatus() order unchanged (regression-tested
against frozen accounts calling POST /auth/delegate)
middleware via Reserve()+immediate Cancel() so rejected requests don't
consume extra bucket budget; add details[0].limited_by ("ip"/"customer")
to the 429 error envelope
constructors (models/auth/auth.go), no signature changes
and RATE_LIMIT_CUSTOMER_REDIS_TIMEOUT_MS config, defaulting to enforce
mode with the ticket's example values (16.7/33, 50/100, 8.3/16); rps<=0
disables a tier, matching the existing IP-tier safe-rollback convention
from cachehandler) in runDaemon with a deferred Close()
new tier labels; add a per-customer, per-minute sampled WARN logger for
repeated rejections, reusing ratelimit.go's cleanup() goroutine pattern
restful_api_errors.rst, common_overview.rst and docs/architecture.md,
docs/operations.md; clean RST rebuild included
Operational note: confirmed via kubectl against the production Redis
instance (infrastructure/redis-6466f86c48-dkqf4) that maxmemory-policy is
allkeys-lru with maxmemory 256MB, currently ~1.7% utilized. Under this
policy, rate-limit keys can be evicted under memory pressure alongside
cache keys, since this Redis instance is shared between caching and rate
limiting. Per design decision, this is absorbed into the already-accepted
fail-open risk rather than requiring a policy change or dedicated
instance; worth revisiting if cache memory pressure grows materially.