feat: Implement bypass based on caller type in the regional frontend regional rate limiter#7662
Conversation
…rate limiter Signed-off-by: fimanishi <fimanishi@gmail.com> # Conflicts: # common/dynamicconfig/dynamicproperties/constants.go
Signed-off-by: fimanishi <fimanishi@gmail.com>
Signed-off-by: fimanishi <fimanishi@gmail.com>
fimanishi
force-pushed
the
implement-frontend-ratelimiter-bypass-based-on-caller-type
branch
from
6b518d4 to
1eb05b0
Compare
Signed-off-by: fimanishi <fimanishi@gmail.com>
fimanishi
requested review from
Assem-Uber,
Shaddoll,
abhishekj720,
adhityamamallan,
arzonus,
bowenxia,
c-warren,
davidporter-id-au,
demirkayaender,
dkrotx,
eleonoradgr,
gazi-yestemirova,
jakobht,
macrotim,
natemort,
neil-xie,
sankari165,
shijiesheng and
timl3136
as code owners
| visibilityRateLimiter quotas.Policy, | ||
| asyncRateLimiter quotas.Policy, | ||
| maxWorkerPollDelay dynamicproperties.DurationPropertyFnWithDomainFilter, | ||
| dc *dynamicconfig.Collection, |
There was a problem hiding this comment.
Is it necessary to have the entire dynamicConfig inside the ratelimit? If we are only looking for a list of whitelist operations to bypass the ratelimiter, perhaps it makes more sense to only pass that list instead.
There was a problem hiding this comment.
+1, I think minimally we should align the way that we pass the maxWorkerPollDelay. Either pull both from the dynamicconfig or pass them both as functions.
| visibilityRateLimiter quotas.Policy, | ||
| asyncRateLimiter quotas.Policy, | ||
| maxWorkerPollDelay dynamicproperties.DurationPropertyFnWithDomainFilter, | ||
| dc *dynamicconfig.Collection, |
There was a problem hiding this comment.
+1, I think minimally we should align the way that we pass the maxWorkerPollDelay. Either pull both from the dynamicconfig or pass them both as functions.
| } | ||
| } | ||
| if !policy.Allow(quotas.Info{Domain: domain}) { | ||
| callerInfo := types.GetCallerInfoFromContext(ctx) |
There was a problem hiding this comment.
Is there any way to move this logic into the Limiter/Policy to avoid repeating it here and in the persistence wrapper?
It might be really complicated to do, so "no" is a valid answer.
There was a problem hiding this comment.
I could create a wrapper that does it, but I would have to create a wrapper for policy and for limiter, which would reduce duplication but still is specific to each one. Let me give it a try
There was a problem hiding this comment.
Or I can try to just make this whole thing a method. It'll probably be better and I can use it for the wait as well
| if ok := c.rateLimiter.Allow(); !ok { | ||
| callerInfo := types.GetCallerInfoFromContext({{(index $method.Params 0).Name}}) | ||
| if c.shouldBypassRateLimit(callerInfo.GetCallerType()) { | ||
| if c.dc != nil && types.ShouldBypassRateLimit(callerInfo.GetCallerType(), c.dc.GetListProperty(dynamicproperties.RateLimiterBypassCallerTypes)()) { |
There was a problem hiding this comment.
If possible, I'd prefer for this to be in a normal Go function rather than as part of a template. The generated code tends to be less tested and can be an easy source of bugs that we would normally catch.
Signed-off-by: fimanishi <fimanishi@gmail.com>
Signed-off-by: fimanishi <fimanishi@gmail.com>
Signed-off-by: fimanishi <fimanishi@gmail.com>
Signed-off-by: fimanishi <fimanishi@gmail.com>
Signed-off-by: fimanishi <fimanishi@gmail.com>
Code Review ✅ ApprovedWell-structured refactoring that consolidates rate limiter bypass logic into a reusable OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
3 participants
What changed?
Added bypass using callerType to regional frontend rate limiter #7654
Why?
Persistence rate limiter does discriminate between some call/caller types and provide specific quotas for that, but within user requests there is no discrimination. There may be situations that we need some requests to be allowed for debugging/mitigation purposes even if the rate limit has been reached. This change allows the definition of a list of caller type to be bypassed in the frontend regional rate limiter.
How did you test it?
Added tests to service/frontend/wrappers/ratelimited/ratelimit_test.go.
Potential risks
Release notes
Documentation Changes
Reviewer Validation
PR Description Quality (check these before reviewing code):
go testinvocation)