fix: disable rate limiting when Redis backend is not configured#1979
fix: disable rate limiting when Redis backend is not configured#1979NJX-njx wants to merge 1 commit into
Conversation
When Redis is not configured (REDIS_HOST is empty or connection fails), the gateway plugin now gracefully falls back to a NoOpRateLimiter instead of crashing with a fatal error or rejecting all requests. Changes: - Add NoOpRateLimiter implementation that always allows requests - Add TryGetRedisClient() that returns nil instead of fatally crashing - Modify NewServer to use NoOpRateLimiter when redisClient is nil - Update gateway plugin main to use TryGetRedisClient - Add unit tests for NoOpRateLimiter Fixes vllm-project#1874
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the gateway plugin would crash fatally if the Redis backend for rate limiting was not configured or became unreachable. By introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19077b0e12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| redisClient := utils.GetRedisClient() | ||
| redisClient := utils.TryGetRedisClient() |
There was a problem hiding this comment.
Switching startup to TryGetRedisClient() allows redisClient to be nil, but request handling still assumes Redis is always present: HandleRequestHeaders calls utils.GetUser(..., s.redisClient) whenever a user header is set, and GetUser dereferences the client directly. In deployments without Redis (the new supported mode), any request carrying user will panic the gateway process instead of gracefully continuing with rate limiting disabled.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a graceful fallback mechanism for the rate limiting feature. When a Redis backend is not configured or unavailable, the system now uses a NoOpRateLimiter which effectively disables rate limiting, preventing the application from crashing. The changes include a new TryGetRedisClient function that doesn't cause a fatal error on connection failure, a NoOpRateLimiter implementation, and updates to the gateway and main application logic to use these new components. The changes are logical and well-tested. I have one suggestion to improve maintainability by reducing code duplication in the Redis utility functions.
| // TryGetRedisClient attempts to connect to Redis and returns the client if successful. | ||
| // Returns nil if Redis is not configured or connection fails, instead of fatally crashing. | ||
| func TryGetRedisClient() *redis.Client { | ||
| redisHost := LoadEnv("REDIS_HOST", "") | ||
| if redisHost == "" { | ||
| klog.Info("REDIS_HOST is not set, Redis client will not be initialized") | ||
| return nil | ||
| } | ||
|
|
||
| redisPort := LoadEnv("REDIS_PORT", "6379") | ||
| redisPassword := LoadEnv("REDIS_PASSWORD", "") | ||
| client := redis.NewClient(&redis.Options{ | ||
| Addr: redisHost + ":" + redisPort, | ||
| Password: redisPassword, | ||
| DB: 0, | ||
| }) | ||
|
|
||
| pong, err := client.Ping(context.Background()).Result() | ||
| if err != nil { | ||
| klog.Warningf("Failed to connect to Redis at %s:%s: %v. Rate limiting will be disabled.", redisHost, redisPort, err) | ||
| client.Close() | ||
| return nil | ||
| } | ||
| klog.Infof("Connected to Redis: %s", pong) | ||
| return client | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you could extract the Redis client creation and connection logic into a private helper function. This new function could then be used by both TryGetRedisClient and the existing GetRedisClient function. This would centralize the connection logic and make future changes easier.
// _newRedisClient creates and pings a new Redis client.
// It returns the client, the PONG response, and an error if connection fails.
func _newRedisClient(host, port, password string) (*redis.Client, string, error) {
client := redis.NewClient(&redis.Options{
Addr: host + ":" + port,
Password: password,
DB: 0,
})
pong, err := client.Ping(context.Background()).Result()
if err != nil {
// It's important to close the client on failure to avoid resource leaks.
client.Close()
return nil, "", err
}
return client, pong, nil
}
// TryGetRedisClient attempts to connect to Redis and returns the client if successful.
// Returns nil if Redis is not configured or connection fails, instead of fatally crashing.
func TryGetRedisClient() *redis.Client {
redisHost := LoadEnv("REDIS_HOST", "")
if redisHost == "" {
klog.Info("REDIS_HOST is not set, Redis client will not be initialized")
return nil
}
redisPort := LoadEnv("REDIS_PORT", "6379")
redisPassword := LoadEnv("REDIS_PASSWORD", "")
client, pong, err := _newRedisClient(redisHost, redisPort, redisPassword)
if err != nil {
klog.Warningf("Failed to connect to Redis at %s:%s: %v. Rate limiting will be disabled.", redisHost, redisPort, err)
return nil
}
klog.Infof("Connected to Redis: %s", pong)
return client
}There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1874 by making the gateway plugin resilient to missing/unavailable Redis: instead of crashing or inadvertently enforcing limits, it falls back to a no-op rate limiter when Redis isn’t configured or can’t be reached.
Changes:
- Add a
NoOpRateLimiterimplementation (and unit tests) that effectively disables rate limiting. - Introduce
utils.TryGetRedisClient()which returnsnil(and logs) when Redis is not configured/unreachable. - Update gateway plugin startup to use the no-op limiter when
redisClientisnil, and safely handlenilin the deferred Redis close.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/redis.go | Adds TryGetRedisClient() to avoid fatal startup failures when Redis is absent/unreachable. |
| pkg/plugins/gateway/ratelimiter/noop.go | Introduces a no-op RateLimiter implementation used as a fallback. |
| pkg/plugins/gateway/ratelimiter/noop_test.go | Adds unit tests validating NoOpRateLimiter behavior and interface conformance. |
| pkg/plugins/gateway/gateway.go | Selects Redis-backed vs no-op rate limiter based on whether redisClient is nil. |
| cmd/plugins/main.go | Switches to TryGetRedisClient() and guards defer-close for nil Redis client. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| pong, err := client.Ping(context.Background()).Result() | ||
| if err != nil { | ||
| klog.Warningf("Failed to connect to Redis at %s:%s: %v. Rate limiting will be disabled.", redisHost, redisPort, err) |
There was a problem hiding this comment.
In TryGetRedisClient the warning log hard-codes "Rate limiting will be disabled", but this helper is in pkg/utils and the returned Redis client is also used for other features (e.g., cache/tracing). Consider making the message Redis-generic (e.g., "continuing without Redis") and letting callers log the feature-specific impact (rate limiting, tracing, etc.).
| klog.Warningf("Failed to connect to Redis at %s:%s: %v. Rate limiting will be disabled.", redisHost, redisPort, err) | |
| klog.Warningf("Failed to connect to Redis at %s:%s: %v. Continuing without Redis.", redisHost, redisPort, err) |
| // NewNoOpRateLimiter creates a rate limiter that performs no rate limiting. | ||
| // This is used as a fallback when Redis is not available or not configured. | ||
| func NewNoOpRateLimiter() RateLimiter { | ||
| klog.Info("Rate limiting is disabled: no Redis backend configured") |
There was a problem hiding this comment.
The log message in NewNoOpRateLimiter says "no Redis backend configured", but this fallback is also used when Redis is configured but unreachable (TryGetRedisClient ping failure). Consider rewording to something accurate for both cases (e.g., "Redis unavailable"), or moving the log to the selection point so the reason (not configured vs connection failed) can be reflected correctly.
| klog.Info("Rate limiting is disabled: no Redis backend configured") | |
| klog.Info("Rate limiting is disabled: Redis backend unavailable or not configured") |
| // Use NoOpRateLimiter when Redis is not configured, effectively disabling rate limiting | ||
| var r ratelimiter.RateLimiter | ||
| if redisClient != nil { | ||
| r = ratelimiter.NewRedisAccountRateLimiter("aibrix", redisClient, 1*time.Minute) | ||
| } else { | ||
| r = ratelimiter.NewNoOpRateLimiter() | ||
| } |
There was a problem hiding this comment.
New behavior introduces an important runtime branch (Redis-backed limiter vs NoOp). There are already unit tests for other Server behaviors in this package, but no test that NewServer selects a NoOpRateLimiter when redisClient is nil. Adding a small unit test would help prevent regressions (e.g., nil Redis leading to panics or accidental enforcement).
|
If redis backend is not configured then rate limiting is not applied. |
Summary
Fixes #1874
When Redis is not configured (
REDIS_HOSTis empty or Redis connection fails), the gateway plugin now gracefully falls back to aNoOpRateLimiterinstead of crashing with a fatal error or rejecting all requests.Changes
pkg/plugins/gateway/ratelimiter/noop.go: AddNoOpRateLimiterimplementation that always allows requests through, effectively disabling rate limitingpkg/plugins/gateway/ratelimiter/noop_test.go: Add unit tests forNoOpRateLimiterpkg/utils/redis.go: AddTryGetRedisClient()that returnsnilinstead of callingklog.Fatalfwhen Redis is unavailablepkg/plugins/gateway/gateway.go: ModifyNewServerto useNoOpRateLimiterwhenredisClientis nilcmd/plugins/main.go: UseTryGetRedisClient()and handle nil redis client in deferBehavior