From d2d437c9a0550d07aaa6b7417ed70c833f809cf0 Mon Sep 17 00:00:00 2001 From: Renuka Fernando Date: Wed, 19 Oct 2022 21:47:12 +0530 Subject: [PATCH 1/2] Fix data race issue in globalShadowMode variable globalShadowMode is written when config is reloaded and read in here, which may lead to data race condition Signed-off-by: Renuka Fernando --- src/service/ratelimit.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index 621a83df3..ee9ae96f0 100644 --- a/src/service/ratelimit.go +++ b/src/service/ratelimit.go @@ -228,7 +228,10 @@ func (this *service) shouldRateLimitWorker( } // If there is a global shadow_mode, it should always return OK - if finalCode == pb.RateLimitResponse_OVER_LIMIT && this.globalShadowMode { + this.configLock.RLock() + globalShadowMode := this.globalShadowMode + this.configLock.RUnlock() + if finalCode == pb.RateLimitResponse_OVER_LIMIT && globalShadowMode { finalCode = pb.RateLimitResponse_OK this.stats.GlobalShadowMode.Inc() } From cb6afa32476b39f89ff2db7cf7ea2e0be34302b9 Mon Sep 17 00:00:00 2001 From: Renuka Fernando Date: Wed, 19 Oct 2022 22:38:02 +0530 Subject: [PATCH 2/2] Use the existing lock instead of making a new read lock Signed-off-by: Renuka Fernando --- src/service/ratelimit.go | 15 ++++++--------- src/service_cmd/runner/runner.go | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index ee9ae96f0..902c8084f 100644 --- a/src/service/ratelimit.go +++ b/src/service/ratelimit.go @@ -32,7 +32,7 @@ var tracer = otel.Tracer("ratelimit") type RateLimitServiceServer interface { pb.RateLimitServiceServer - GetCurrentConfig() config.RateLimitConfig + GetCurrentConfig() (config.RateLimitConfig, bool) } type service struct { @@ -107,8 +107,7 @@ func checkServiceErr(something bool, msg string) { } } -func (this *service) constructLimitsToCheck(request *pb.RateLimitRequest, ctx context.Context) ([]*config.RateLimit, []bool) { - snappedConfig := this.GetCurrentConfig() +func (this *service) constructLimitsToCheck(request *pb.RateLimitRequest, ctx context.Context, snappedConfig config.RateLimitConfig) ([]*config.RateLimit, []bool) { checkServiceErr(snappedConfig != nil, "no rate limit configuration loaded") limitsToCheck := make([]*config.RateLimit, len(request.Descriptors)) @@ -180,7 +179,8 @@ func (this *service) shouldRateLimitWorker( checkServiceErr(request.Domain != "", "rate limit domain must not be empty") checkServiceErr(len(request.Descriptors) != 0, "rate limit descriptor list must not be empty") - limitsToCheck, isUnlimited := this.constructLimitsToCheck(request, ctx) + snappedConfig, globalShadowMode := this.GetCurrentConfig() + limitsToCheck, isUnlimited := this.constructLimitsToCheck(request, ctx, snappedConfig) responseDescriptorStatuses := this.cache.DoLimit(ctx, request, limitsToCheck) assert.Assert(len(limitsToCheck) == len(responseDescriptorStatuses)) @@ -228,9 +228,6 @@ func (this *service) shouldRateLimitWorker( } // If there is a global shadow_mode, it should always return OK - this.configLock.RLock() - globalShadowMode := this.globalShadowMode - this.configLock.RUnlock() if finalCode == pb.RateLimitResponse_OVER_LIMIT && globalShadowMode { finalCode = pb.RateLimitResponse_OK this.stats.GlobalShadowMode.Inc() @@ -309,10 +306,10 @@ func (this *service) ShouldRateLimit( return response, nil } -func (this *service) GetCurrentConfig() config.RateLimitConfig { +func (this *service) GetCurrentConfig() (config.RateLimitConfig, bool) { this.configLock.RLock() defer this.configLock.RUnlock() - return this.config + return this.config, this.globalShadowMode } func NewService(runtime loader.IFace, cache limiter.RateLimitCache, diff --git a/src/service_cmd/runner/runner.go b/src/service_cmd/runner/runner.go index 74a8105e0..605aed0ec 100644 --- a/src/service_cmd/runner/runner.go +++ b/src/service_cmd/runner/runner.go @@ -130,7 +130,7 @@ func (runner *Runner) Run() { "/rlconfig", "print out the currently loaded configuration for debugging", func(writer http.ResponseWriter, request *http.Request) { - if current := service.GetCurrentConfig(); current != nil { + if current, _ := service.GetCurrentConfig(); current != nil { io.WriteString(writer, current.Dump()) } })