-
Notifications
You must be signed in to change notification settings - Fork 458
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
Explicit unlimited ratelimits #261
Changes from 7 commits
a3f32e1
0ef96da
7f577aa
8f007c7
651aca7
df4f71a
6a81d9b
01de07f
2d18c1c
8988873
0862dd8
c8bf7e3
e3cc523
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ import ( | |
type yamlRateLimit struct { | ||
RequestsPerUnit uint32 `yaml:"requests_per_unit"` | ||
Unit string | ||
Unlimited bool `yaml:"unlimited"` | ||
} | ||
|
||
type yamlDescriptor struct { | ||
|
@@ -51,17 +52,19 @@ var validKeys = map[string]bool{ | |
"rate_limit": true, | ||
"unit": true, | ||
"requests_per_unit": true, | ||
"unlimited": true, | ||
} | ||
|
||
// Create a new rate limit config entry. | ||
// @param requestsPerUnit supplies the requests per unit of time for the entry. | ||
// @param unit supplies the unit of time for the entry. | ||
// @param rlStats supplies the stats structure associated with the RateLimit | ||
// @param unlimited supplies whether the rate limit is unlimited | ||
// @return the new config entry. | ||
func NewRateLimit( | ||
requestsPerUnit uint32, unit pb.RateLimitResponse_RateLimit_Unit, rlStats stats.RateLimitStats) *RateLimit { | ||
requestsPerUnit uint32, unit pb.RateLimitResponse_RateLimit_Unit, rlStats stats.RateLimitStats, unlimited bool) *RateLimit { | ||
|
||
return &RateLimit{FullKey: rlStats.GetKey(), Stats: rlStats, Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}} | ||
return &RateLimit{FullKey: rlStats.GetKey(), Stats: rlStats, Limit: &pb.RateLimitResponse_RateLimit{RequestsPerUnit: requestsPerUnit, Unit: unit}, Unlimited: unlimited} | ||
} | ||
|
||
// Dump an individual descriptor for debugging purposes. | ||
|
@@ -112,19 +115,21 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p | |
var rateLimit *RateLimit = nil | ||
var rateLimitDebugString string = "" | ||
if descriptorConfig.RateLimit != nil { | ||
unlimited := descriptorConfig.RateLimit.Unlimited | ||
|
||
value, present := | ||
pb.RateLimitResponse_RateLimit_Unit_value[strings.ToUpper(descriptorConfig.RateLimit.Unit)] | ||
if !present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN) { | ||
if (!present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN)) && !unlimited { | ||
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 split this logic out so that we actually check if unlimited is set with a unit at all and have an error message (and test) for that? I think that would be more clear to the user. 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. (Ping on this) 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. I wasn't too sure what exactly you meant, so please review this again :) |
||
panic(newRateLimitConfigError( | ||
config, | ||
fmt.Sprintf("invalid rate limit unit '%s'", descriptorConfig.RateLimit.Unit))) | ||
} | ||
|
||
rateLimit = NewRateLimit( | ||
descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), statsManager.NewStats(newParentKey)) | ||
descriptorConfig.RateLimit.RequestsPerUnit, pb.RateLimitResponse_RateLimit_Unit(value), statsManager.NewStats(newParentKey), unlimited) | ||
rateLimitDebugString = fmt.Sprintf( | ||
" ratelimit={requests_per_unit=%d, unit=%s}", rateLimit.Limit.RequestsPerUnit, | ||
rateLimit.Limit.Unit.String()) | ||
" ratelimit={requests_per_unit=%d, unit=%s, unlimited=%t}", rateLimit.Limit.RequestsPerUnit, | ||
rateLimit.Limit.Unit.String(), rateLimit.Unlimited) | ||
} | ||
|
||
logger.Debugf( | ||
|
@@ -167,6 +172,8 @@ func validateYamlKeys(config RateLimitConfigToLoad, config_map map[interface{}]i | |
case string: | ||
// int is a leaf type in ratelimit config. No need to keep validating. | ||
case int: | ||
// bool is a leaf type in ratelimit config. No need to keep validating. | ||
case bool: | ||
// nil case is an incorrectly formed yaml. However, because this function's purpose is to validate | ||
// the yaml's keys we don't panic here. | ||
case nil: | ||
|
@@ -240,7 +247,8 @@ func (this *rateLimitConfigImpl) GetLimit( | |
rateLimit = NewRateLimit( | ||
descriptor.GetLimit().GetRequestsPerUnit(), | ||
rateLimitOverrideUnit, | ||
this.statsManager.NewStats(rateLimitKey)) | ||
this.statsManager.NewStats(rateLimitKey), | ||
false) | ||
return rateLimit | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,3 +56,7 @@ descriptors: | |
rate_limit: | ||
unit: day | ||
requests_per_unit: 25 | ||
|
||
- key: key6 | ||
rate_limit: | ||
unlimited: true |
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.
Can you mention why someone would want to do this? I assume for stats, etc.?
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.
Well, stats yes. But also, in our case, our client will not allow a request to go through if a descriptor is not found, so we do have to have it defined.
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'll add something to the readme
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.
Added