Skip to content

Commit 90b260d

Browse files
lyuxuanlyuxuan
committed
fix reviews again
1 parent cd4f1b6 commit 90b260d

File tree

5 files changed

+93
-70
lines changed

5 files changed

+93
-70
lines changed

Diff for: ‎clientconn.go

+27-24
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,27 +467,22 @@ 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
}
480-
return false
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+
return err != nil
481486
}
482487

483488
func (cc *ClientConn) updateResolverState(s resolver.State) error {
@@ -490,14 +495,14 @@ func (cc *ClientConn) updateResolverState(s resolver.State) error {
490495
return nil
491496
}
492497

493-
if cc.shouldApplyDefaultServiceConfig(s.ServiceConfig) {
494-
if cc.sc.rawJSONString == nil {
498+
if cc.fallbackToDefaultServiceConfig(s.ServiceConfig) {
499+
if cc.dopts.defaultServiceConfig != nil && cc.sc.rawJSONString == nil {
495500
cc.applyServiceConfig(cc.dopts.defaultServiceConfig)
496501
}
497502
} else {
498503
// TODO: the parsing logic below will be moved inside resolver.
499504
sc, err := parseServiceConfig(s.ServiceConfig)
500-
if err != nil && s.ServiceConfig != "" { // s.ServiceConfig != "" is a temporary special case.
505+
if err != nil {
501506
return err
502507
}
503508
if cc.sc.rawJSONString == nil || *cc.sc.rawJSONString != s.ServiceConfig {
@@ -763,11 +768,9 @@ func (cc *ClientConn) getTransport(ctx context.Context, failfast bool, method st
763768
return t, done, nil
764769
}
765770

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.
769771
func (cc *ClientConn) applyServiceConfig(sc *ServiceConfig) error {
770772
if sc == nil {
773+
// should never reach here.
771774
return fmt.Errorf("got nil pointer for service config")
772775
}
773776
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-
}

Diff for: ‎service_config_test.go

+26-20
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
func (s) TestParseLoadBalancer(t *testing.T) {
3030
testcases := []struct {
3131
scjs string
32-
wantSC ServiceConfig
32+
wantSC *ServiceConfig
3333
wantErr bool
3434
}{
3535
{
@@ -47,7 +47,7 @@ func (s) TestParseLoadBalancer(t *testing.T) {
4747
}
4848
]
4949
}`,
50-
ServiceConfig{
50+
&ServiceConfig{
5151
LB: newString("round_robin"),
5252
Methods: map[string]MethodConfig{
5353
"/foo/Bar": {
@@ -72,23 +72,23 @@ func (s) TestParseLoadBalancer(t *testing.T) {
7272
}
7373
]
7474
}`,
75-
ServiceConfig{},
75+
nil,
7676
true,
7777
},
7878
}
7979

8080
for _, c := range testcases {
8181
sc, err := parseServiceConfig(c.scjs)
82-
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
83-
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, *sc, err, c.wantSC, c.wantErr)
82+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(sc, c.wantSC) {
83+
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
8484
}
8585
}
8686
}
8787

8888
func (s) TestParseWaitForReady(t *testing.T) {
8989
testcases := []struct {
9090
scjs string
91-
wantSC ServiceConfig
91+
wantSC *ServiceConfig
9292
wantErr bool
9393
}{
9494
{
@@ -105,7 +105,7 @@ func (s) TestParseWaitForReady(t *testing.T) {
105105
}
106106
]
107107
}`,
108-
ServiceConfig{
108+
&ServiceConfig{
109109
Methods: map[string]MethodConfig{
110110
"/foo/Bar": {
111111
WaitForReady: newBool(true),
@@ -128,7 +128,7 @@ func (s) TestParseWaitForReady(t *testing.T) {
128128
}
129129
]
130130
}`,
131-
ServiceConfig{
131+
&ServiceConfig{
132132
Methods: map[string]MethodConfig{
133133
"/foo/Bar": {
134134
WaitForReady: newBool(false),
@@ -160,14 +160,14 @@ func (s) TestParseWaitForReady(t *testing.T) {
160160
}
161161
]
162162
}`,
163-
ServiceConfig{},
163+
nil,
164164
true,
165165
},
166166
}
167167

168168
for _, c := range testcases {
169169
sc, err := parseServiceConfig(c.scjs)
170-
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
170+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(sc, c.wantSC) {
171171
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
172172
}
173173
}
@@ -176,7 +176,7 @@ func (s) TestParseWaitForReady(t *testing.T) {
176176
func (s) TestPraseTimeOut(t *testing.T) {
177177
testcases := []struct {
178178
scjs string
179-
wantSC ServiceConfig
179+
wantSC *ServiceConfig
180180
wantErr bool
181181
}{
182182
{
@@ -193,7 +193,7 @@ func (s) TestPraseTimeOut(t *testing.T) {
193193
}
194194
]
195195
}`,
196-
ServiceConfig{
196+
&ServiceConfig{
197197
Methods: map[string]MethodConfig{
198198
"/foo/Bar": {
199199
Timeout: newDuration(time.Second),
@@ -216,7 +216,7 @@ func (s) TestPraseTimeOut(t *testing.T) {
216216
}
217217
]
218218
}`,
219-
ServiceConfig{},
219+
nil,
220220
true,
221221
},
222222
{
@@ -242,14 +242,14 @@ func (s) TestPraseTimeOut(t *testing.T) {
242242
}
243243
]
244244
}`,
245-
ServiceConfig{},
245+
nil,
246246
true,
247247
},
248248
}
249249

250250
for _, c := range testcases {
251251
sc, err := parseServiceConfig(c.scjs)
252-
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
252+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(sc, c.wantSC) {
253253
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
254254
}
255255
}
@@ -258,7 +258,7 @@ func (s) TestPraseTimeOut(t *testing.T) {
258258
func (s) TestPraseMsgSize(t *testing.T) {
259259
testcases := []struct {
260260
scjs string
261-
wantSC ServiceConfig
261+
wantSC *ServiceConfig
262262
wantErr bool
263263
}{
264264
{
@@ -276,7 +276,7 @@ func (s) TestPraseMsgSize(t *testing.T) {
276276
}
277277
]
278278
}`,
279-
ServiceConfig{
279+
&ServiceConfig{
280280
Methods: map[string]MethodConfig{
281281
"/foo/Bar": {
282282
MaxReqSize: newInt(1024),
@@ -311,14 +311,14 @@ func (s) TestPraseMsgSize(t *testing.T) {
311311
}
312312
]
313313
}`,
314-
ServiceConfig{},
314+
nil,
315315
true,
316316
},
317317
}
318318

319319
for _, c := range testcases {
320320
sc, err := parseServiceConfig(c.scjs)
321-
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(*sc, c.wantSC) {
321+
if c.wantErr != (err != nil) || !scCompareWithRawJSONSkipped(sc, c.wantSC) {
322322
t.Fatalf("parseServiceConfig(%s) = %+v, %v, want %+v, %v", c.scjs, sc, err, c.wantSC, c.wantErr)
323323
}
324324
}
@@ -385,7 +385,13 @@ func newString(b string) *string {
385385
return &b
386386
}
387387

388-
func scCompareWithRawJSONSkipped(s1, s2 ServiceConfig) bool {
388+
func scCompareWithRawJSONSkipped(s1, s2 *ServiceConfig) bool {
389+
if s1 == nil && s2 == nil {
390+
return true
391+
}
392+
if (s1 == nil) != (s2 == nil) {
393+
return false
394+
}
389395
s1.rawJSONString = nil
390396
s2.rawJSONString = nil
391397
return reflect.DeepEqual(s1, s2)

0 commit comments

Comments
 (0)