Address comments

Signed-off-by: lmajercak-wish <[email protected]>

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
 

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)))

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
 		}
 

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

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")
+}

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

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)