From bc3eca49d4a3c1bd601d2e543c306ea942808277 Mon Sep 17 00:00:00 2001 From: Renuka Piyumal Fernando Date: Sat, 5 Nov 2022 03:52:14 +0530 Subject: [PATCH] Fix data race issue in globalShadowMode variable (#370) 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 | 14 +++++++------- src/service_cmd/runner/runner.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index 621a83df..902c8084 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,7 +228,7 @@ func (this *service) shouldRateLimitWorker( } // If there is a global shadow_mode, it should always return OK - if finalCode == pb.RateLimitResponse_OVER_LIMIT && this.globalShadowMode { + if finalCode == pb.RateLimitResponse_OVER_LIMIT && globalShadowMode { finalCode = pb.RateLimitResponse_OK this.stats.GlobalShadowMode.Inc() } @@ -306,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 74a8105e..605aed0e 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()) } })