diff --git a/README.md b/README.md index 3b2b218c0..099b8afb7 100644 --- a/README.md +++ b/README.md @@ -347,7 +347,8 @@ descriptors: requests_per_unit: 100 ``` -For an unlimited descriptor, the request will not be sent to the underlying cache (Redis/Memcached), but will be quickly returned locally by the ratelimit instance. +For an unlimited descriptor, the request will not be sent to the underlying cache (Redis/Memcached), but will be quickly returned locally by the ratelimit instance. +This can be useful for collecting statistics, or if one wants to define a descriptor that has no limit but the client wants to distinguish between such descriptor and one that does not exist. ## Loading Configuration diff --git a/src/config/config_impl.go b/src/config/config_impl.go index 20d30a6c0..0664e5b2e 100644 --- a/src/config/config_impl.go +++ b/src/config/config_impl.go @@ -119,7 +119,15 @@ func (this *rateLimitDescriptor) loadDescriptors(config RateLimitConfigToLoad, p value, present := pb.RateLimitResponse_RateLimit_Unit_value[strings.ToUpper(descriptorConfig.RateLimit.Unit)] - if (!present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN)) && !unlimited { + validUnit := present && value != int32(pb.RateLimitResponse_RateLimit_UNKNOWN) + + if unlimited { + if validUnit { + panic(newRateLimitConfigError( + config, + fmt.Sprintf("should not specify rate limit unit when unlimited"))) + } + } else if !validUnit { panic(newRateLimitConfigError( config, fmt.Sprintf("invalid rate limit unit '%s'", descriptorConfig.RateLimit.Unit))) diff --git a/src/memcached/cache_impl.go b/src/memcached/cache_impl.go index dc8090b10..4b21af331 100644 --- a/src/memcached/cache_impl.go +++ b/src/memcached/cache_impl.go @@ -136,7 +136,7 @@ func (this *rateLimitMemcacheImpl) increaseAsync(cacheKeys []limiter.CacheKey, i limits []*config.RateLimit, hitsAddend uint64) { defer this.waitGroup.Done() for i, cacheKey := range cacheKeys { - if cacheKey.Key == "" || isOverLimitWithLocalCache[i] || limits[i].Unlimited { + if cacheKey.Key == "" || isOverLimitWithLocalCache[i] { continue } diff --git a/src/service/ratelimit.go b/src/service/ratelimit.go index 63273a913..8ac20019d 100644 --- a/src/service/ratelimit.go +++ b/src/service/ratelimit.go @@ -3,6 +3,7 @@ package ratelimit import ( "fmt" "github.com/envoyproxy/ratelimit/src/stats" + "math" "strings" "sync" @@ -76,8 +77,6 @@ func checkServiceErr(something bool, msg string) { } } - - func (this *service) constructLimitsToCheck(request *pb.RateLimitRequest, ctx context.Context) ([]*config.RateLimit, []bool) { snappedConfig := this.GetCurrentConfig() checkServiceErr(snappedConfig != nil, "no rate limit configuration loaded") @@ -103,7 +102,7 @@ func (this *service) constructLimitsToCheck(request *pb.RateLimitRequest, ctx co } else { if limitsToCheck[i].Unlimited { logger.Debugf("descriptor is unlimited, not passing to the cache") - } else { + } else { logger.Debugf( "applying limit: %d requests per %s", limitsToCheck[i].Limit.RequestsPerUnit, @@ -138,7 +137,8 @@ func (this *service) shouldRateLimitWorker( for i, descriptorStatus := range responseDescriptorStatuses { if isUnlimited[i] { response.Statuses[i] = &pb.RateLimitResponse_DescriptorStatus{ - Code: pb.RateLimitResponse_OK, + Code: pb.RateLimitResponse_OK, + LimitRemaining: math.MaxUint32, } } else { response.Statuses[i] = descriptorStatus diff --git a/test/config/config_test.go b/test/config/config_test.go index 7bc05be74..d6d79c5e1 100644 --- a/test/config/config_test.go +++ b/test/config/config_test.go @@ -375,3 +375,14 @@ func TestNonMapList(t *testing.T) { }, "non_map_list.yaml: config error, yaml file contains list of type other than map: a") } + +func TestUnlimitedWithRateLimitUnit(t *testing.T) { + expectConfigPanic( + t, + func() { + config.NewRateLimitConfigImpl( + loadFile("unlimited_with_unit.yaml"), + mockstats.NewMockStatManager(stats.NewStore(stats.NewNullSink(), false))) + }, + "unlimited_with_unit.yaml: should not specify rate limit unit when unlimited") +} diff --git a/test/config/unlimited_with_unit.yaml b/test/config/unlimited_with_unit.yaml new file mode 100644 index 000000000..4b6f8b7fa --- /dev/null +++ b/test/config/unlimited_with_unit.yaml @@ -0,0 +1,7 @@ +domain: test-domain +descriptors: + - key: foo + rate_limit: + unlimited: true + unit: day + requests_per_unit: 25 diff --git a/test/service/ratelimit_test.go b/test/service/ratelimit_test.go index 1c7c28ac4..64c44ba16 100644 --- a/test/service/ratelimit_test.go +++ b/test/service/ratelimit_test.go @@ -2,6 +2,7 @@ package ratelimit_test import ( "github.com/envoyproxy/ratelimit/src/stats" + "math" "sync" "testing" @@ -273,7 +274,7 @@ func TestUnlimited(test *testing.T) { Statuses: []*pb.RateLimitResponse_DescriptorStatus{ {Code: pb.RateLimitResponse_OK, CurrentLimit: limits[0].Limit, LimitRemaining: 9}, {Code: pb.RateLimitResponse_OK, CurrentLimit: nil, LimitRemaining: 0}, - {Code: pb.RateLimitResponse_OK, CurrentLimit: nil, LimitRemaining: 0}, + {Code: pb.RateLimitResponse_OK, CurrentLimit: nil, LimitRemaining: math.MaxUint32}, }}, response) t.assert.Nil(err)