service config: add default method config support (#3684)

clientconn.go

@@ -860,9 +860,10 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool {
 // GetMethodConfig gets the method config of the input method.
 // If there's an exact match for input method (i.e. /service/method), we return
 // the corresponding MethodConfig.
-// If there isn't an exact match for the input method, we look for the default config
-// under the service (i.e /service/). If there is a default MethodConfig for
-// the service, we return it.
+// If there isn't an exact match for the input method, we look for the service's default
+// config under the service (i.e /service/) and then for the default for all services (empty string).
+//
+// If there is a default MethodConfig for the service, we return it.
 // Otherwise, we return an empty MethodConfig.
 func (cc *ClientConn) GetMethodConfig(method string) MethodConfig {
 	// TODO: Avoid the locking here.
@@ -871,12 +872,14 @@ func (cc *ClientConn) GetMethodConfig(method string) MethodConfig {
 	if cc.sc == nil {
 		return MethodConfig{}
 	}
-	m, ok := cc.sc.Methods[method]
-	if !ok {
-		i := strings.LastIndex(method, "/")
-		m = cc.sc.Methods[method[:i+1]]
+	if m, ok := cc.sc.Methods[method]; ok {
+		return m
+	}
+	i := strings.LastIndex(method, "/")
+	if m, ok := cc.sc.Methods[method[:i+1]]; ok {
+		return m
 	}
-	return m
+	return cc.sc.Methods[""]
 }
 
 func (cc *ClientConn) healthCheckConfig() *healthCheckConfig {

clientconn_test.go

@@ -776,6 +776,29 @@ func (s) TestDisableServiceConfigOption(t *testing.T) {
 	}
 }
 
+func (s) TestMethodConfigDefaultService(t *testing.T) {
+	addr := "nonexist:///non.existent"
+	cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(`{
+  "methodConfig": [{
+    "name": [
+      {
+        "service": ""
+      }
+    ],
+    "waitForReady": true
+  }]
+}`))
+	if err != nil {
+		t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
+	}
+	defer cc.Close()
+
+	m := cc.GetMethodConfig("/foo/Bar")
+	if m.WaitForReady == nil {
+		t.Fatalf("want: method (%q) config to fallback to the default service", "/foo/Bar")
+	}
+}
+
 func (s) TestGetClientConnTarget(t *testing.T) {
 	addr := "nonexist:///non.existent"
 	cc, err := Dial(addr, WithInsecure())

service_config.go

@@ -20,6 +20,7 @@ package grpc
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
 	"strconv"
@@ -224,19 +225,27 @@ func parseDuration(s *string) (*time.Duration, error) {
 }
 
 type jsonName struct {
-	Service *string
-	Method  *string
+	Service string
+	Method  string
 }
 
-func (j jsonName) generatePath() (string, bool) {
-	if j.Service == nil {
-		return "", false
+var (
+	errDuplicatedName             = errors.New("duplicated name")
+	errEmptyServiceNonEmptyMethod = errors.New("cannot combine empty 'service' and non-empty 'method'")
+)
+
+func (j jsonName) generatePath() (string, error) {
+	if j.Service == "" {
+		if j.Method != "" {
+			return "", errEmptyServiceNonEmptyMethod
+		}
+		return "", nil
 	}
-	res := "/" + *j.Service + "/"
-	if j.Method != nil {
-		res += *j.Method
+	res := "/" + j.Service + "/"
+	if j.Method != "" {
+		res += j.Method
 	}
-	return res, true
+	return res, nil
 }
 
 // TODO(lyuxuan): delete this struct after cleaning up old service config implementation.
@@ -288,6 +297,8 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 	if rsc.MethodConfig == nil {
 		return &serviceconfig.ParseResult{Config: &sc}
 	}
+
+	paths := map[string]struct{}{}
 	for _, m := range *rsc.MethodConfig {
 		if m.Name == nil {
 			continue
@@ -320,10 +331,20 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 				mc.MaxRespSize = newInt(int(*m.MaxResponseMessageBytes))
 			}
 		}
-		for _, n := range *m.Name {
-			if path, valid := n.generatePath(); valid {
-				sc.Methods[path] = mc
+		for i, n := range *m.Name {
+			path, err := n.generatePath()
+			if err != nil {
+				logger.Warningf("grpc: parseServiceConfig error unmarshaling %s due to methodConfig[%d]: %v", js, i, err)
+				return &serviceconfig.ParseResult{Err: err}
+			}
+
+			if _, ok := paths[path]; ok {
+				err = errDuplicatedName
+				logger.Warningf("grpc: parseServiceConfig error unmarshaling %s due to methodConfig[%d]: %v", js, i, err)
+				return &serviceconfig.ParseResult{Err: err}
 			}
+			paths[path] = struct{}{}
+			sc.Methods[path] = mc
 		}
 	}
 

service_config_test.go

@@ -372,6 +372,82 @@ func (s) TestParseMsgSize(t *testing.T) {
 
 	runParseTests(t, testcases)
 }
+func (s) TestParseDefaultMethodConfig(t *testing.T) {
+	dc := &ServiceConfig{
+		Methods: map[string]MethodConfig{
+			"": {WaitForReady: newBool(true)},
+		},
+	}
+
+	runParseTests(t, []parseTestCase{
+		{
+			`{
+  "methodConfig": [{
+    "name": [{}],
+    "waitForReady": true
+  }]
+}`,
+			dc,
+			false,
+		},
+		{
+			`{
+  "methodConfig": [{
+    "name": [{"service": null}],
+    "waitForReady": true
+  }]
+}`,
+			dc,
+			false,
+		},
+		{
+			`{
+  "methodConfig": [{
+    "name": [{"service": ""}],
+    "waitForReady": true
+  }]
+}`,
+			dc,
+			false,
+		},
+		{
+			`{
+  "methodConfig": [{
+    "name": [{"method": "Bar"}],
+    "waitForReady": true
+  }]
+}`,
+			nil,
+			true,
+		},
+		{
+			`{
+  "methodConfig": [{
+    "name": [{"service": "", "method": "Bar"}],
+    "waitForReady": true
+  }]
+}`,
+			nil,
+			true,
+		},
+	})
+}
+
+func (s) TestParseMethodConfigDuplicatedName(t *testing.T) {
+	runParseTests(t, []parseTestCase{
+		{
+			`{
+  "methodConfig": [{
+    "name": [
+      {"service": "foo"},
+      {"service": "foo"}
+    ],
+    "waitForReady": true
+  }]
+}`, nil, true,
+		},
+	})
+}
 
 func (s) TestParseDuration(t *testing.T) {
 	testCases := []struct {