balancer/rls: allow maxAge to exceed 5m if staleAge is set (#8137)

Author: shivaspeaks

balancer/rls/config.go

@@ -220,27 +220,43 @@ func parseRLSProto(rlsProto *rlspb.RouteLookupConfig) (*lbConfig, error) {
 
 	// Validations performed here:
 	// - if `max_age` > 5m, it should be set to 5 minutes
+	//   only if stale age is not set
 	// - if `stale_age` > `max_age`, ignore it
 	// - if `stale_age` is set, then `max_age` must also be set
+	maxAgeSet := false
 	maxAge, err := convertDuration(rlsProto.GetMaxAge())
 	if err != nil {
 		return nil, fmt.Errorf("rls: failed to parse max_age in route lookup config %+v: %v", rlsProto, err)
 	}
+	if maxAge == 0 {
+		maxAge = maxMaxAge
+	} else {
+		maxAgeSet = true
+	}
+
+	staleAgeSet := false
 	staleAge, err := convertDuration(rlsProto.GetStaleAge())
 	if err != nil {
 		return nil, fmt.Errorf("rls: failed to parse staleAge in route lookup config %+v: %v", rlsProto, err)
 	}
-	if staleAge != 0 && maxAge == 0 {
+	if staleAge == 0 {
+		staleAge = maxMaxAge
+	} else {
+		staleAgeSet = true
+	}
+
+	if staleAgeSet && !maxAgeSet {
 		return nil, fmt.Errorf("rls: stale_age is set, but max_age is not in route lookup config %+v", rlsProto)
 	}
-	if staleAge >= maxAge {
-		logger.Infof("rls: stale_age %v is not less than max_age %v, ignoring it", staleAge, maxAge)
-		staleAge = 0
+	if staleAge > maxMaxAge {
+		staleAge = maxMaxAge
 	}
-	if maxAge == 0 || maxAge > maxMaxAge {
-		logger.Infof("rls: max_age in route lookup config is %v, using %v", maxAge, maxMaxAge)
+	if !staleAgeSet && maxAge > maxMaxAge {
 		maxAge = maxMaxAge
 	}
+	if staleAge > maxAge {
+		staleAge = maxAge
+	}
 
 	// `cache_size_bytes` field must have a value greater than 0, and if its
 	// value is greater than 5M, we cap it at 5M

balancer/rls/config_test.go

@@ -60,8 +60,8 @@ func (s) TestParseConfig(t *testing.T) {
 			// - A top-level unknown field should not fail.
 			// - An unknown field in routeLookupConfig proto should not fail.
 			// - lookupServiceTimeout is set to its default value, since it is not specified in the input.
-			// - maxAge is set to maxMaxAge since the value is too large in the input.
-			// - staleAge is ignore because it is higher than maxAge in the input.
+			// - maxAge is clamped to maxMaxAge if staleAge is not set.
+			// - staleAge is ignored because it is higher than maxAge in the input.
 			// - cacheSizeBytes is greater than the hard upper limit of 5MB
 			desc: "with transformations 1",
 			input: []byte(`{
@@ -87,9 +87,9 @@ func (s) TestParseConfig(t *testing.T) {
 			}`),
 			wantCfg: &lbConfig{
 				lookupService:          ":///target",
-				lookupServiceTimeout:   10 * time.Second, // This is the default value.
-				maxAge:                 5 * time.Minute,  // This is max maxAge.
-				staleAge:               time.Duration(0), // StaleAge is ignore because it was higher than maxAge.
+				lookupServiceTimeout:   10 * time.Second,  // This is the default value.
+				maxAge:                 500 * time.Second, // Max age is not clamped when stale age is set.
+				staleAge:               300 * time.Second, // StaleAge is clamped because it was higher than maxMaxAge.
 				cacheSizeBytes:         maxCacheSize,
 				defaultTarget:          "passthrough:///default",
 				childPolicyName:        "grpclb",
@@ -100,6 +100,69 @@ func (s) TestParseConfig(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "maxAge not clamped when staleAge is set",
+			input: []byte(`{
+				"routeLookupConfig": {
+					"grpcKeybuilders": [{
+						"names": [{"service": "service", "method": "method"}],
+						"headers": [{"key": "k1", "names": ["v1"]}]
+					}],
+					"lookupService": ":///target",
+					"maxAge" : "500s",
+					"staleAge": "200s",
+					"cacheSizeBytes": 100000000
+				},
+				"childPolicy": [
+					{"grpclb": {"childPolicy": [{"pickfirst": {}}]}}
+				],
+				"childPolicyConfigTargetFieldName": "serviceName"
+			}`),
+			wantCfg: &lbConfig{
+				lookupService:          ":///target",
+				lookupServiceTimeout:   10 * time.Second,  // This is the default value.
+				maxAge:                 500 * time.Second, // Max age is not clamped when stale age is set.
+				staleAge:               200 * time.Second, // This is stale age within maxMaxAge.
+				cacheSizeBytes:         maxCacheSize,
+				childPolicyName:        "grpclb",
+				childPolicyTargetField: "serviceName",
+				childPolicyConfig: map[string]json.RawMessage{
+					"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`),
+					"serviceName": json.RawMessage(childPolicyTargetFieldVal),
+				},
+			},
+		},
+		{
+			desc: "maxAge clamped when staleAge is not set",
+			input: []byte(`{
+				"routeLookupConfig": {
+					"grpcKeybuilders": [{
+						"names": [{"service": "service", "method": "method"}],
+						"headers": [{"key": "k1", "names": ["v1"]}]
+					}],
+					"lookupService": ":///target",
+					"maxAge" : "500s",
+					"cacheSizeBytes": 100000000
+				},
+				"childPolicy": [
+					{"grpclb": {"childPolicy": [{"pickfirst": {}}]}}
+				],
+				"childPolicyConfigTargetFieldName": "serviceName"
+			}`),
+			wantCfg: &lbConfig{
+				lookupService:          ":///target",
+				lookupServiceTimeout:   10 * time.Second,  // This is the default value.
+				maxAge:                 300 * time.Second, // Max age is clamped when stale age is not set.
+				staleAge:               300 * time.Second,
+				cacheSizeBytes:         maxCacheSize,
+				childPolicyName:        "grpclb",
+				childPolicyTargetField: "serviceName",
+				childPolicyConfig: map[string]json.RawMessage{
+					"childPolicy": json.RawMessage(`[{"pickfirst": {}}]`),
+					"serviceName": json.RawMessage(childPolicyTargetFieldVal),
+				},
+			},
+		},
 		{
 			desc: "without transformations",
 			input: []byte(`{