client: implement maxAttempts for retryPolicy (#7229)

Author: imoore76

clientconn.go

@@ -170,7 +170,7 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error)
 	}
 
 	if cc.dopts.defaultServiceConfigRawJSON != nil {
-		scpr := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON)
+		scpr := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON, cc.dopts.maxCallAttempts)
 		if scpr.Err != nil {
 			return nil, fmt.Errorf("%s: %v", invalidDefaultServiceConfigErrPrefix, scpr.Err)
 		}
@@ -693,7 +693,7 @@ func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error {
 var emptyServiceConfig *ServiceConfig
 
 func init() {
-	cfg := parseServiceConfig("{}")
+	cfg := parseServiceConfig("{}", defaultMaxCallAttempts)
 	if cfg.Err != nil {
 		panic(fmt.Sprintf("impossible error parsing empty service config: %v", cfg.Err))
 	}

dialoptions.go

@@ -37,6 +37,11 @@ import (
 	"google.golang.org/grpc/stats"
 )
 
+const (
+	// https://github.com/grpc/proposal/blob/master/A6-client-retries.md#limits-on-retries-and-hedges
+	defaultMaxCallAttempts = 5
+)
+
 func init() {
 	internal.AddGlobalDialOptions = func(opt ...DialOption) {
 		globalDialOptions = append(globalDialOptions, opt...)
@@ -89,6 +94,7 @@ type dialOptions struct {
 	idleTimeout                 time.Duration
 	recvBufferPool              SharedBufferPool
 	defaultScheme               string
+	maxCallAttempts             int
 }
 
 // DialOption configures how we set up the connection.
@@ -677,6 +683,7 @@ func defaultDialOptions() dialOptions {
 		idleTimeout:     30 * time.Minute,
 		recvBufferPool:  nopBufferPool{},
 		defaultScheme:   "dns",
+		maxCallAttempts: defaultMaxCallAttempts,
 	}
 }
 
@@ -734,6 +741,23 @@ func WithIdleTimeout(d time.Duration) DialOption {
 	})
 }
 
+// WithMaxCallAttempts returns a DialOption that configures the maximum number
+// of attempts per call (including retries and hedging) using the channel.
+// Service owners may specify a higher value for these parameters, but higher
+// values will be treated as equal to the maximum value by the client
+// implementation. This mitigates security concerns related to the service
+// config being transferred to the client via DNS.
+//
+// A value of 5 will be used if this dial option is not set or n < 2.
+func WithMaxCallAttempts(n int) DialOption {
+	return newFuncDialOption(func(o *dialOptions) {
+		if n < 2 {
+			n = defaultMaxCallAttempts
+		}
+		o.maxCallAttempts = n
+	})
+}
+
 // WithRecvBufferPool returns a DialOption that configures the ClientConn
 // to use the provided shared buffer pool for parsing incoming messages. Depending
 // on the application's workload, this could result in reduced memory allocation.

resolver_wrapper.go

@@ -171,7 +171,7 @@ func (ccr *ccResolverWrapper) NewAddress(addrs []resolver.Address) {
 // ParseServiceConfig is called by resolver implementations to parse a JSON
 // representation of the service config.
 func (ccr *ccResolverWrapper) ParseServiceConfig(scJSON string) *serviceconfig.ParseResult {
-	return parseServiceConfig(scJSON)
+	return parseServiceConfig(scJSON, ccr.cc.dopts.maxCallAttempts)
 }
 
 // addChannelzTraceEvent adds a channelz trace event containing the new

service_config.go

@@ -164,9 +164,11 @@ type jsonSC struct {
 }
 
 func init() {
-	internal.ParseServiceConfig = parseServiceConfig
+	internal.ParseServiceConfig = func(js string) *serviceconfig.ParseResult {
+		return parseServiceConfig(js, defaultMaxCallAttempts)
+	}
 }
-func parseServiceConfig(js string) *serviceconfig.ParseResult {
+func parseServiceConfig(js string, maxAttempts int) *serviceconfig.ParseResult {
 	if len(js) == 0 {
 		return &serviceconfig.ParseResult{Err: fmt.Errorf("no JSON service config provided")}
 	}
@@ -219,7 +221,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 			WaitForReady: m.WaitForReady,
 			Timeout:      (*time.Duration)(m.Timeout),
 		}
-		if mc.RetryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil {
+		if mc.RetryPolicy, err = convertRetryPolicy(m.RetryPolicy, maxAttempts); err != nil {
 			logger.Warningf("grpc: unmarshalling service config %s: %v", js, err)
 			return &serviceconfig.ParseResult{Err: err}
 		}
@@ -265,7 +267,7 @@ func parseServiceConfig(js string) *serviceconfig.ParseResult {
 	return &serviceconfig.ParseResult{Config: &sc}
 }
 
-func convertRetryPolicy(jrp *jsonRetryPolicy) (p *internalserviceconfig.RetryPolicy, err error) {
+func convertRetryPolicy(jrp *jsonRetryPolicy, maxAttempts int) (p *internalserviceconfig.RetryPolicy, err error) {
 	if jrp == nil {
 		return nil, nil
 	}
@@ -279,17 +281,16 @@ func convertRetryPolicy(jrp *jsonRetryPolicy) (p *internalserviceconfig.RetryPol
 		return nil, nil
 	}
 
+	if jrp.MaxAttempts < maxAttempts {
+		maxAttempts = jrp.MaxAttempts
+	}
 	rp := &internalserviceconfig.RetryPolicy{
-		MaxAttempts:          jrp.MaxAttempts,
+		MaxAttempts:          maxAttempts,
 		InitialBackoff:       time.Duration(jrp.InitialBackoff),
 		MaxBackoff:           time.Duration(jrp.MaxBackoff),
 		BackoffMultiplier:    jrp.BackoffMultiplier,
 		RetryableStatusCodes: make(map[codes.Code]bool),
 	}
-	if rp.MaxAttempts > 5 {
-		// TODO(retry): Make the max maxAttempts configurable.
-		rp.MaxAttempts = 5
-	}
 	for _, code := range jrp.RetryableStatusCodes {
 		rp.RetryableStatusCodes[code] = true
 	}

service_config_test.go

@@ -60,7 +60,7 @@ func runParseTests(t *testing.T, testCases []parseTestCase) {
 	t.Helper()
 	for i, c := range testCases {
 		t.Run(fmt.Sprint(i), func(t *testing.T) {
-			scpr := parseServiceConfig(c.scjs)
+			scpr := parseServiceConfig(c.scjs, defaultMaxCallAttempts)
 			var sc *ServiceConfig
 			sc, _ = scpr.Config.(*ServiceConfig)
 			if !c.wantErr {

test/retry_test.go

@@ -477,6 +477,97 @@ func (s) TestRetryStreaming(t *testing.T) {
 	}
 }
 
+func (s) TestMaxCallAttempts(t *testing.T) {
+	testCases := []struct {
+		serviceMaxAttempts int
+		clientMaxAttempts  int
+		expectedAttempts   int
+	}{
+		{serviceMaxAttempts: 9, clientMaxAttempts: 4, expectedAttempts: 4},
+		{serviceMaxAttempts: 9, clientMaxAttempts: 7, expectedAttempts: 7},
+		{serviceMaxAttempts: 3, clientMaxAttempts: 10, expectedAttempts: 3},
+		{serviceMaxAttempts: 8, clientMaxAttempts: -1, expectedAttempts: 5}, // 5 is default max
+		{serviceMaxAttempts: 3, clientMaxAttempts: 0, expectedAttempts: 3},
+	}
+
+	for _, tc := range testCases {
+		clientOpts := []grpc.DialOption{
+			grpc.WithMaxCallAttempts(tc.clientMaxAttempts),
+			grpc.WithDefaultServiceConfig(fmt.Sprintf(`{
+				"methodConfig": [{
+					"name": [{"service": "grpc.testing.TestService"}],
+					"waitForReady": true,
+					"retryPolicy": {
+						"MaxAttempts": %d,
+						"InitialBackoff": ".01s",
+						"MaxBackoff": ".01s",
+						"BackoffMultiplier": 1.0,
+						"RetryableStatusCodes": [ "UNAVAILABLE" ]
+					}
+					}]}`, tc.serviceMaxAttempts),
+			),
+		}
+
+		streamCallCount := 0
+		unaryCallCount := 0
+
+		ss := &stubserver.StubServer{
+			FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error {
+				streamCallCount++
+				return status.New(codes.Unavailable, "this is a test error").Err()
+			},
+			EmptyCallF: func(context.Context, *testpb.Empty) (r *testpb.Empty, err error) {
+				unaryCallCount++
+				return nil, status.New(codes.Unavailable, "this is a test error").Err()
+			},
+		}
+
+		func() {
+
+			if err := ss.Start([]grpc.ServerOption{}, clientOpts...); err != nil {
+				t.Fatalf("Error starting endpoint server: %v", err)
+			}
+			defer ss.Stop()
+			ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+			defer cancel()
+
+			for {
+				if ctx.Err() != nil {
+					t.Fatalf("Timed out waiting for service config update")
+				}
+				if ss.CC.GetMethodConfig("/grpc.testing.TestService/FullDuplexCall").WaitForReady != nil {
+					break
+				}
+				time.Sleep(time.Millisecond)
+			}
+
+			// Test streaming RPC
+			stream, err := ss.Client.FullDuplexCall(ctx)
+			if err != nil {
+				t.Fatalf("Error while creating stream: %v", err)
+			}
+			if got, err := stream.Recv(); err == nil {
+				t.Fatalf("client: Recv() = %s, %v; want <nil>, error", got, err)
+			} else if status.Code(err) != codes.Unavailable {
+				t.Fatalf("client: Recv() = _, %v; want _, Unavailable", err)
+			}
+			if streamCallCount != tc.expectedAttempts {
+				t.Fatalf("stream expectedAttempts = %v; want %v", streamCallCount, tc.expectedAttempts)
+			}
+
+			// Test unary RPC
+			if ugot, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); err == nil {
+				t.Fatalf("client: EmptyCall() = %s, %v; want <nil>, error", ugot, err)
+			} else if status.Code(err) != codes.Unavailable {
+				t.Fatalf("client: EmptyCall() = _, %v; want _, Unavailable", err)
+			}
+			if unaryCallCount != tc.expectedAttempts {
+				t.Fatalf("unary expectedAttempts = %v; want %v", unaryCallCount, tc.expectedAttempts)
+			}
+		}()
+	}
+}
+
 type retryStatsHandler struct {
 	mu sync.Mutex
 	s  []stats.RPCStats