xds: add xDS transport custom dial options support #7997

yousukseung · 2025-01-09T18:50:55Z

This PR extends #7586 to support multiple custom dial options.

The custom dialer option added in PR#7586 is now deprecated but not removed yet. This will be removed in a future PR after dependencies are moved to the new option.

RELEASE NOTES: N/A

yousukseung · 2025-01-09T21:25:10Z

@danielzhaotongliu FYI

internal/xds/bootstrap/bootstrap.go

test/xds/xds_client_custom_dialopts_test.go

purnesh42H · 2025-01-10T04:50:19Z

@yousukseung we are not updating transport to use this instead yet?

yousukseung · 2025-01-10T22:37:36Z

@yousukseung we are not updating transport to use this instead yet?

I removed JoinDialOptions(), the ServerConfig's DialOptions() already returns []DialOption and this was not necessary.

I don't see any change needed in transport though. All changes I'm making is internal to bootstrap.go and externally it will be the same DialOptions() API. I see they are applied in Build() in grpctransport.

codecov · 2025-01-10T22:43:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.19%. Comparing base (c879198) to head (a637cab).
Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7997      +/-   ##
==========================================
+ Coverage   82.04%   82.19%   +0.14%     
==========================================
  Files         384      384              
  Lines       38750    38929     +179     
==========================================
+ Hits        31792    31996     +204     
+ Misses       5632     5595      -37     
- Partials     1326     1338      +12

Files with missing lines	Coverage Δ
internal/xds/bootstrap/bootstrap.go	`65.43% <100.00%> (-3.27%)`	⬇️

... and 44 files with indirect coverage changes

purnesh42H · 2025-01-13T16:55:31Z

I haven't completed the review but adding @easwars as well to review in parallel

danielzhaotongliu

Tested end-to-end in prod, the current commit work as expected.

danielzhaotongliu · 2025-01-13T21:52:18Z

test/xds/xds_client_custom_dialopts_test.go

+// testCredsBuilder implements the `Credentials` interface defined in
+// package `xds/bootstrap` and encapsulates an insecure credential with a
+// custom Dialer that specifies how to dial the xDS server.
+type testCredsBuilder struct {
+	dialerCalled     atomic.Bool
+	tagRPCCalled     atomic.Bool
+	handleRPCCalled  atomic.Bool
+	tagConnCalled    atomic.Bool
+	handleConnCalled atomic.Bool
+}


I think the doc string does not align with what the struct actually is? This both a custom Dialer and stats Handler.

I presume this xds_client_custom_dialopts_test.go file was originally copied from test/xds/xds_client_custom_dialer_test.go in #7586 but some changes were not updated?

Would the plan in the next PR, once internal/prod changes were made, to delete xds_client_custom_dialer_test.go?

Yes lazy copy & paste. I removed the description, I think all we need is to tell what it extends, the rest seems obvious from reading.

danielzhaotongliu · 2025-01-13T22:04:41Z

test/xds/xds_client_custom_dialopts_test.go

+
+// noopStatsHandler implements `stats.Handler`. It's a no-op mock handler.
+type noopStatsHandler struct {
+	tagRPCCalled     *atomic.Bool


What would be the reason for using *atomic.Bool instead of just atomic.Bool?
Also see below comment regarding using sync/atomic package vs channel

I believe the way things are setup instantiates atomics in testCredsBuilder, then I saw no reason to copy. Or anything I missed?

danielzhaotongliu · 2025-01-13T22:12:06Z

test/xds/xds_client_custom_dialopts_test.go

+// stats handler.
+type testCredsBundle struct {
+	credentials.Bundle
+	dialerCalled      *atomic.Bool


We switched from using chan struct{} (in xds_client_custom_dialer_test.go) to the atomic package.

Channel are used for communication between goroutines (but it can also be used as mutex/atomic synchronization). In this case, I think we should be OK as we are using it for "one-time" signaling / synchronization between goroutines. Looking at https://go.dev/ref/mem, I don't see any issues.

@easwars what would be the preferred approach here? Would there be any issues switching from channel to atomic? From Go tips/best practices, it is recommended to use channel for one-time signaling.

AFAICT channel seems to be the obvious choice if you need to block on the event you expect. But here we can ensure expected events (both the dialer and stats handler) should have happened before we validate as we call cc.Close() first (commented). Plus for stats handler atomic simply works better as we expect multiple events but all we want to check is the presence of any.

yousukseung · 2025-01-16T18:19:16Z

@purnesh42H @easwars kindly ping to unblock prod monitoring visibility.

internal/xds/bootstrap/bootstrap.go

test/xds/xds_client_custom_dialopts_test.go

purnesh42H · 2025-01-17T07:58:59Z

@purnesh42H @easwars kindly ping to unblock prod monitoring visibility.

Overall it looks good to me. Few comments regarding removing some extra stuff that we have right now.

internal/xds/bootstrap/bootstrap.go

xds/internal/xdsclient/tests/client_custom_dialopts_test.go

easwars · 2025-01-28T17:31:32Z

xds/internal/xdsclient/tests/client_custom_dialopts_test.go

+	// data plane communication with the test backend.
+	cc, err := grpc.NewClient(fmt.Sprintf("xds:///%s", serviceName), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(resolverBuilder))
+	if err != nil {
+		t.Fatalf("failed to dial local test server: %v", err)


Nit: s/failed/Failed

The non-capitalization of error strings don't apply to test error messages.

easwars · 2025-01-28T17:33:27Z

@purnesh42H : Do you want to take another look?

purnesh42H

lgtm

yousukseung added 3 commits January 9, 2025 10:49

xds: add xDS transport custom dial options support

404a3a6

removed unused const

499d283

trigger test again

466fb00

purnesh42H added Type: Internal Cleanup Refactors, etc Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Jan 10, 2025

purnesh42H reviewed Jan 10, 2025

View reviewed changes

internal/xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

internal/xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

test/xds/xds_client_custom_dialopts_test.go Outdated Show resolved Hide resolved

comments addressed

6e7639d

don't use join

5b9e9d7

purnesh42H assigned easwars Jan 13, 2025

purnesh42H requested a review from easwars January 13, 2025 16:55

yousukseung added 3 commits January 13, 2025 10:00

vet.sh fix

4288b8b

retest

5767b77

Merge branch 'master' into xdsclient-dialopts

1b41034

danielzhaotongliu reviewed Jan 13, 2025

View reviewed changes

update comment

3768fef

purnesh42H reviewed Jan 17, 2025

View reviewed changes

purnesh42H assigned yousukseung Jan 17, 2025

yousukseung added 5 commits January 17, 2025 17:02

test with mock dialopts

21cb7c9

Merge branch 'master' into xdsclient-dialopts

dbc1062

vet.sh fix

cda2ca1

vet fix, renamed

21e4e0f

more vet fix

681038c

easwars added this to the 1.71 Release milestone Jan 22, 2025

easwars reviewed Jan 22, 2025

View reviewed changes

easwars removed their assignment Jan 22, 2025

yousukseung added 3 commits January 22, 2025 11:02

comments addressed

5b5c6dd

vet fix

3189c43

comment updated

3e21df2

easwars approved these changes Jan 28, 2025

View reviewed changes

easwars assigned purnesh42H and unassigned yousukseung Jan 28, 2025

test message capitalization

a637cab

purnesh42H self-requested a review January 29, 2025 12:29

purnesh42H approved these changes Jan 29, 2025

View reviewed changes

purnesh42H merged commit 59411f2 into grpc:master Jan 29, 2025
15 checks passed

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Jan 30, 2025

xds: add xDS transport custom dial options support (grpc#7997)

722b775

yousukseung mentioned this pull request Feb 12, 2025

xds: remove obsolete xDS transport custom dialer option #8079

Merged

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 13, 2025

xds: add xDS transport custom dial options support (grpc#7997)

90b1784

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 13, 2025

xds: add xDS transport custom dial options support (grpc#7997)

55a5908

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 15, 2025

xds: add xDS transport custom dial options support (grpc#7997)

73c95ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: add xDS transport custom dial options support #7997

xds: add xDS transport custom dial options support #7997

yousukseung commented Jan 9, 2025 •

edited by purnesh42H

Loading

yousukseung commented Jan 9, 2025

purnesh42H commented Jan 10, 2025

yousukseung commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

purnesh42H commented Jan 13, 2025

danielzhaotongliu left a comment

danielzhaotongliu Jan 13, 2025

yousukseung Jan 13, 2025

danielzhaotongliu Jan 13, 2025

yousukseung Jan 13, 2025

danielzhaotongliu Jan 13, 2025

yousukseung Jan 13, 2025

yousukseung commented Jan 16, 2025

purnesh42H commented Jan 17, 2025

easwars Jan 28, 2025

yousukseung Jan 28, 2025

easwars commented Jan 28, 2025

purnesh42H left a comment

xds: add xDS transport custom dial options support #7997

xds: add xDS transport custom dial options support #7997

Conversation

yousukseung commented Jan 9, 2025 • edited by purnesh42H Loading

yousukseung commented Jan 9, 2025

purnesh42H commented Jan 10, 2025

yousukseung commented Jan 10, 2025 • edited Loading

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

purnesh42H commented Jan 13, 2025

danielzhaotongliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yousukseung commented Jan 16, 2025

purnesh42H commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jan 28, 2025

purnesh42H left a comment

Choose a reason for hiding this comment

yousukseung commented Jan 9, 2025 •

edited by purnesh42H

Loading

yousukseung commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading