Skip to content

Commit b44a9c0

Browse files
lyuxuanlyuxuan
committed
fix reviews again
1 parent cd4f1b6 commit b44a9c0

File tree

4 files changed

+70
-49
lines changed

4 files changed

+70
-49
lines changed

Diff for: ‎clientconn.go

+30-23
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,9 @@ var (
8686
// errCredentialsConflict indicates that grpc.WithTransportCredentials()
8787
// and grpc.WithInsecure() are both called for a connection.
8888
errCredentialsConflict = errors.New("grpc: transport credentials are set for an insecure connection (grpc.WithTransportCredentials() and grpc.WithInsecure() are both called)")
89+
// errInvalidDefaultServiceConfig indicates that grpc.WithDefaultServiceConfig(string) provides
90+
// an invalid service config string.
91+
errInvalidDefaultServiceConfig = errors.New("grpc: the provided default service config is invalid")
8992
)
9093

9194
const (
@@ -173,6 +176,13 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
173176
}
174177
}
175178

179+
if cc.dopts.defaultServiceConfigRawJSON != nil {
180+
sc, err := parseServiceConfig(*cc.dopts.defaultServiceConfigRawJSON)
181+
if err != nil {
182+
return nil, errInvalidDefaultServiceConfig
183+
}
184+
cc.dopts.defaultServiceConfig = sc
185+
}
176186
cc.mkp = cc.dopts.copts.KeepaliveParams
177187

178188
if cc.dopts.copts.Dialer == nil {
@@ -457,26 +467,25 @@ func (cc *ClientConn) waitForResolvedAddrs(ctx context.Context) error {
457467
}
458468
}
459469

460-
// Apply default service config when default service config is configured and:
470+
// gRPC should resort to default service config when:
461471
// * resolver service config is disabled
462472
// * or, resolver does not return a service config or returns an invalid one.
463-
func (cc *ClientConn) shouldApplyDefaultServiceConfig(sc string) bool {
464-
if cc.dopts.defaultServiceConfig != nil {
465-
if cc.dopts.disableServiceConfig {
466-
return true
467-
}
468-
// The logic below is temporary, will be removed once we change the resolver.State ServiceConfig field type.
469-
// Right now, we assume that empty service config string means resolver does not return a config.
470-
if sc == "" {
471-
return false
472-
}
473-
// TODO: the logic below is temporary. Once we finish the logic to validate service config
474-
// in resolver, we will replace the logic below.
475-
_, err := parseServiceConfig(sc)
476-
if err != nil {
477-
return true
478-
}
473+
func (cc *ClientConn) fallbackToDefaultServiceConfig(sc string) bool {
474+
if cc.dopts.disableServiceConfig {
475+
return true
479476
}
477+
// The logic below is temporary, will be removed once we change the resolver.State ServiceConfig field type.
478+
// Right now, we assume that empty service config string means resolver does not return a config.
479+
if sc == "" {
480+
return true
481+
}
482+
// TODO: the logic below is temporary. Once we finish the logic to validate service config
483+
// in resolver, we will replace the logic below.
484+
_, err := parseServiceConfig(sc)
485+
if err != nil {
486+
return true
487+
}
488+
480489
return false
481490
}
482491

@@ -490,14 +499,14 @@ func (cc *ClientConn) updateResolverState(s resolver.State) error {
490499
return nil
491500
}
492501

493-
if cc.shouldApplyDefaultServiceConfig(s.ServiceConfig) {
494-
if cc.sc.rawJSONString == nil {
502+
if cc.fallbackToDefaultServiceConfig(s.ServiceConfig) {
503+
if cc.dopts.defaultServiceConfig != nil && cc.sc.rawJSONString == nil {
495504
cc.applyServiceConfig(cc.dopts.defaultServiceConfig)
496505
}
497506
} else {
498507
// TODO: the parsing logic below will be moved inside resolver.
499508
sc, err := parseServiceConfig(s.ServiceConfig)
500-
if err != nil && s.ServiceConfig != "" { // s.ServiceConfig != "" is a temporary special case.
509+
if err != nil {
501510
return err
502511
}
503512
if cc.sc.rawJSONString == nil || *cc.sc.rawJSONString != s.ServiceConfig {
@@ -763,11 +772,9 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st
763772
return t, done, nil
764773
}
765774

766-
// Parse and apply the service config. If sc is passed as a non-nil pointer, which indicates we have
767-
// a parsed service config, we will skip the parsing. It will also skip the whole processing if
768-
// the new service config is the same as the old one.
769775
func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig) error {
770776
if sc == nil {
777+
// should never reach here.
771778
return fmt.Errorf("got nil pointer for service config")
772779
}
773780
cc.sc = *sc

Diff for: ‎clientconn_test.go

+24
Original file line numberDiff line numberDiff line change
@@ -1250,8 +1250,10 @@ func (s) TestDefaultServiceConfig(t *testing.T) {
12501250
}
12511251
]
12521252
}`
1253+
testInvalidDefaultServiceConfig(t)
12531254
testDefaultServiceConfigWhenResolverServiceConfigDisabled(t, r, addr, js)
12541255
testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t, r, addr, js)
1256+
testDefaultServiceConfigWhenResolverReturnInvalidServiceConfig(t, r, addr, js)
12551257
}
12561258

12571259
func verifyWaitForReadyEqualsTrue(cc *ClientConn) bool {
@@ -1266,6 +1268,13 @@ func verifyWaitForReadyEqualsTrue(cc *ClientConn) bool {
12661268
return i != 10
12671269
}
12681270

1271+
func testInvalidDefaultServiceConfig(t *testing.T) {
1272+
_, err := Dial("fake.com", WithInsecure(), WithDefaultServiceConfig(""))
1273+
if err != errInvalidDefaultServiceConfig {
1274+
t.Fatalf("Dial got err: %v, want: %v", err, errInvalidDefaultServiceConfig)
1275+
}
1276+
}
1277+
12691278
func testDefaultServiceConfigWhenResolverServiceConfigDisabled(t *testing.T, r resolver.Resolver, addr string, js string) {
12701279
cc, err := Dial(addr, WithInsecure(), WithDisableServiceConfig(), WithDefaultServiceConfig(js))
12711280
if err != nil {
@@ -1295,3 +1304,18 @@ func testDefaultServiceConfigWhenResolverDoesNotReturnServiceConfig(t *testing.T
12951304
t.Fatal("default service config failed to be applied after 1s")
12961305
}
12971306
}
1307+
1308+
func testDefaultServiceConfigWhenResolverReturnInvalidServiceConfig(t *testing.T, r resolver.Resolver, addr string, js string) {
1309+
cc, err := Dial(addr, WithInsecure(), WithDefaultServiceConfig(js))
1310+
if err != nil {
1311+
t.Fatalf("Dial(%s, _) = _, %v, want _, <nil>", addr, err)
1312+
}
1313+
defer cc.Close()
1314+
r.(*manual.Resolver).UpdateState(resolver.State{
1315+
Addresses: []resolver.Address{{Addr: addr}},
1316+
ServiceConfig: "{something wrong,}",
1317+
})
1318+
if !verifyWaitForReadyEqualsTrue(cc) {
1319+
t.Fatal("default service config failed to be applied after 1s")
1320+
}
1321+
}

Diff for: ‎dialoptions.go

+13-17
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,16 @@ type dialOptions struct {
5555
// balancer, and also by WithBalancerName dial option.
5656
balancerBuilder balancer.Builder
5757
// This is to support grpclb.
58-
resolverBuilder resolver.Builder
59-
reqHandshake envconfig.RequireHandshakeSetting
60-
channelzParentID int64
61-
disableServiceConfig bool
62-
disableRetry bool
63-
disableHealthCheck bool
64-
healthCheckFunc internal.HealthChecker
65-
minConnectTimeout func() time.Duration
66-
defaultServiceConfig *ServiceConfig
58+
resolverBuilder resolver.Builder
59+
reqHandshake envconfig.RequireHandshakeSetting
60+
channelzParentID int64
61+
disableServiceConfig bool
62+
disableRetry bool
63+
disableHealthCheck bool
64+
healthCheckFunc internal.HealthChecker
65+
minConnectTimeout func() time.Duration
66+
defaultServiceConfig *ServiceConfig
67+
defaultServiceConfigRawJSON *string
6768
}
6869

6970
// DialOption configures how we set up the connection.
@@ -452,16 +453,11 @@ func WithDisableServiceConfig() DialOption {
452453
// will be used in cases where:
453454
// 1. WithDisableServiceConfig is called.
454455
// 2. Resolver does not return service config or if the resolver gets and invalid config.
455-
// It is strongly recommended that caller of this function verifies the validity of the input string
456-
// by using the grpc.ValidateServiceConfig function.
456+
//
457+
// This API is EXPERIMENTAL.
457458
func WithDefaultServiceConfig(s string) DialOption {
458459
return newFuncDialOption(func(o *dialOptions) {
459-
sc, err := parseServiceConfig(s)
460-
if err != nil {
461-
grpclog.Warningf("the provided service config is invalid, err: %v", err)
462-
return
463-
}
464-
o.defaultServiceConfig = sc
460+
o.defaultServiceConfigRawJSON = &s
465461
})
466462
}
467463

Diff for: ‎service_config.go

+3-9
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) {
247247
err := json.Unmarshal([]byte(js), &rsc)
248248
if err != nil {
249249
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
250-
return &ServiceConfig{}, err
250+
return nil, err
251251
}
252252
sc := ServiceConfig{
253253
LB: rsc.LoadBalancingPolicy,
@@ -267,7 +267,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) {
267267
d, err := parseDuration(m.Timeout)
268268
if err != nil {
269269
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
270-
return &ServiceConfig{}, err
270+
return nil, err
271271
}
272272

273273
mc := MethodConfig{
@@ -276,7 +276,7 @@ func parseServiceConfig(js string) (*ServiceConfig, error) {
276276
}
277277
if mc.retryPolicy, err = convertRetryPolicy(m.RetryPolicy); err != nil {
278278
grpclog.Warningf("grpc: parseServiceConfig error unmarshaling %s due to %v", js, err)
279-
return &ServiceConfig{}, err
279+
return nil, err
280280
}
281281
if m.MaxRequestMessageBytes != nil {
282282
if *m.MaxRequestMessageBytes > int64(maxInt) {
@@ -372,9 +372,3 @@ func getMaxSize(mcMax, doptMax *int, defaultVal int) *int {
372372
func newInt(b int) *int {
373373
return &b
374374
}
375-
376-
// ValidateServiceConfig validates the input service config json string and returns the error.
377-
func ValidateServiceConfig(js string) error {
378-
_, err := parseServiceConfig(js)
379-
return err
380-
}

0 commit comments

Comments
 (0)