-
Notifications
You must be signed in to change notification settings - Fork 234
feat(router): enable fail open on rate limit redis availability failure #1659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
37f742a
89923f0
f7aa17c
f5b23f0
30fdbc0
5206888
91bd663
36fab7d
ce3bb3c
ea3002c
28bc3bb
2caeb38
68beaf5
77c3909
7086c1e
ee001b7
6fbebc3
b92c17e
0b42772
128fcdc
72df63c
20b983e
42ad877
4f2bddb
9648ead
cf6e041
e1277bd
b95f851
3d09e8c
9546cd7
0cbe10d
8a3ddc3
df7eb96
3da170a
ba9dc95
896f9c0
3898392
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,12 @@ import ( | |
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" | ||
| "io" | ||
| "reflect" | ||
| "sync" | ||
|
|
||
| rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" | ||
|
|
||
| "github.com/expr-lang/expr/vm" | ||
| "github.com/go-redis/redis_rate/v10" | ||
| "github.com/wundergraph/cosmo/router/internal/expr" | ||
|
|
@@ -28,6 +29,7 @@ type CosmoRateLimiterOptions struct { | |
| RejectStatusCode int | ||
|
|
||
| KeySuffixExpression string | ||
| FailOpen bool | ||
| ExprManager *expr.Manager | ||
| } | ||
|
|
||
|
|
@@ -38,6 +40,7 @@ func NewCosmoRateLimiter(opts *CosmoRateLimiterOptions) (rl *CosmoRateLimiter, e | |
| limiter: limiter, | ||
| debug: opts.Debug, | ||
| rejectStatusCode: opts.RejectStatusCode, | ||
| failOpen: opts.FailOpen, | ||
| } | ||
| if rl.rejectStatusCode == 0 { | ||
| rl.rejectStatusCode = 200 | ||
|
|
@@ -55,10 +58,11 @@ type CosmoRateLimiter struct { | |
| client rd.RDCloser | ||
| limiter *redis_rate.Limiter | ||
| debug bool | ||
|
|
||
| rejectStatusCode int | ||
|
|
||
| keySuffixProgram *vm.Program | ||
|
|
||
| failOpen bool | ||
| } | ||
|
|
||
| func (c *CosmoRateLimiter) RateLimitPreFetch(ctx *resolve.Context, info *resolve.FetchInfo, input json.RawMessage) (result *resolve.RateLimitDeny, err error) { | ||
|
|
@@ -72,11 +76,15 @@ func (c *CosmoRateLimiter) RateLimitPreFetch(ctx *resolve.Context, info *resolve | |
| Period: ctx.RateLimitOptions.Period, | ||
| } | ||
| key, err := c.generateKey(ctx) | ||
| if err != nil { | ||
| if err != nil && c.failOpen{ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. generateKey is not interacting with redis, what's the reason you skip here as well?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will spin up my test environment and sort this out then get back to you! |
||
| return nil, nil | ||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
| allow, err := c.limiter.AllowN(ctx.Context(), key, limit, requestRate) | ||
| if err != nil { | ||
| if err != nil && c.failOpen{ | ||
| return nil, nil | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check here if redis is truly unavailable?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an entirely fair callout, we should be concerned with treating all errors equal here. When implementing this my thoughts were anything is better then dropping the request with an internal service error. I didn't encounter unexpected side effects at that time. I wonder if strict integration testing would be enough or if adding complexity to this solution is the best path forward? |
||
| } else if err != nil { | ||
| return nil, err | ||
| } | ||
| c.setRateLimitStats(ctx, key, requestRate, allow.Remaining, allow.RetryAfter.Milliseconds(), allow.ResetAfter.Milliseconds()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1810,6 +1810,11 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "default": "RATE_LIMIT_EXCEEDED" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fail_open": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "type": "boolean", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "description": "Enable Rate Limit fail open on redis availability failure. This interacts with Redis timeout configuration parameters, essentially adding to each requests latency in failure.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "default": false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1813
to
1818
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add conditional schema logic to prevent silent mis-configuration
Guard against that class of error by expressing the relationship directly in the "fail_open": {
"type": "boolean",
"description": "Enable Rate Limit fail open on redis availability failure. This interacts with Redis timeout configuration parameters, essentially adding to each requests latency in failure.",
"default": false
}
+ },
+ "allOf": [
+ {
+ "if": {
+ "properties": {
+ "enabled": { "const": true },
+ "fail_open": { "const": false }
+ }
+ },
+ "then": {
+ "required": ["storage"]
+ },
+ "else": {
+ "not": {
+ "anyOf": [
+ { "required": ["storage"] }
+ ]
+ }
+ }
+ }
+ ]This ensures:
Keeps configuration errors in CI instead of production. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this a "generic" FailOpen (could be fine) or do we want to make the feature more specific?
Naming is hard, but something like: IgnoreRedisUnavailableErrors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am game for naming this however you think would best fit the specific use case. More specificity would almost certainly be a good idea. I like IgnoreRedisUnavailableErrors, IgnoreRateLimitsWhenRedisUnavailable, or FailOpenRateLimitsDependencyError.