grpc: Add perTargetDialOption type and global list #7234

zasweq · 2024-05-14T22:20:37Z

This PR adds an internal only (exported, but can only set through internal/) LateApplyDialOption type. This will be used in CSM Observability.

RELEASE NOTES: N/A

dfawley · 2024-05-15T17:44:38Z

clientconn.go

+	for _, lateApplyOpt := range globalLateApplyDialOptions {
+		lateApplyOpt.DialOption(cc.parsedTarget.URL).apply(&cc.dopts)


Nit: opt (as above) or even o is preferred for very narrowly scoped vars like this.

Ah, right. Switched to opt.

dfawley · 2024-05-15T17:49:43Z

clientconn.go

+
+	// Register ClientConn with channelz.
+	cc.channelzRegistration(target)
+	// TODO: Ideally it should be impossible to error from this function after


This diff makes the problem this TODO describes even worse. And there are places where we error from this now that don't call channelz.RemoveEntry, leaking resources.

Please make parseTargetBlah stop doing channelz logs (maybe move some useful channelz messages into this function after channelz registration), and move channelz registration back down to where it was.

If you're feeling extra helpful, you could also extend that to the other initialization functions that might do channelz logs, and make sure we don't create the channelz representation of this channel until immediately before the function returns.

Switched to solution discussed offline; make parseTarget log at info, and log authority in NewClient body. Only create channelz after we know any errors can occur.

dfawley · 2024-05-15T17:50:20Z

dialoptions.go

@@ -89,6 +96,14 @@ type DialOption interface {

 var globalDialOptions []DialOption

+// LateApplyDialOption takes a parsed target and returns a dial option to apply.
+type LateApplyDialOption interface {


Unexport? Move to internal? I really don't want users seeing this hack.

Will need to move to internal/ since the csm/ package needs to typecast the any in internal. But yeah makes sense I didn't want this to be in exported namespace (at least right now the user can't set it :)) but forgot I can just put in internal/.

Discussed further offline; unexported and typecast from any in SetGlobalDialOptions.

dfawley · 2024-05-15T17:51:47Z

internal/internal.go

@@ -106,6 +106,14 @@ var (
 	// This is used in the 1.0 release of gcp/observability, and thus must not be
 	// deleted or changed.
 	ClearGlobalDialOptions func()
+
+	// AddGlobalLateApplyDialOptions adds a slice of lateApplyDialOptions that
+	// will be configured for newly created ClientConns.


This (or something) should explain what these magic things can or cannot do.

I don't know what you mean by "can or cannot do". This is an arbitrary hack we added for one specific purpose, so I feel like scoping out what it's properties are and it's constraints doesn't apply? I added "This gets called after NewClient() parses the target, and allows per target
configuration set through a returned DialOption." to the PerTargetDialOption type (which I moved to internal/ to hide from users). Hopefully that's around what you meant by this comment?

E.g. WithResolvers doesn't work. Anything else?

dfawley · 2024-05-15T17:52:02Z

default_dial_option_server_option_test.go

+}
+
+func (s) TestGlobalLateApplyDialOption(t *testing.T) {
+	internal.AddGlobalLateApplyDialOptions.(func(opt ...LateApplyDialOption))(&testLateApplyDialOption{}) // Do I need a clear?


Deleted. The answer is duh, otherwise other tests would pick up globals :).

dfawley · 2024-05-15T17:54:26Z

default_dial_option_server_option_test.go

+	if parsedTarget.Scheme == "passthrough" {
+		return WithTransportCredentials(insecure.NewCredentials()) // credentials provided, should pass NewClient.
+	}
+	return WithReadBufferSize(10) // no credentials, should fail NewClient


Can we use EmptyDialOption instead?

Good point. Switched.

dfawley · 2024-05-15T17:55:41Z

dialoptions.go

+	DialOption(parsedTarget url.URL) DialOption
+}
+
+var globalLateApplyDialOptions []LateApplyDialOption


Should we ditch this name since they don't have a lateApply method? How about perTargetDialOption or something?

Alright will switch to perTarget.

dfawley · 2024-05-15T17:57:09Z

default_dial_option_server_option_test.go

+	return WithReadBufferSize(10) // no credentials, should fail NewClient
+}
+
+func (s) TestGlobalLateApplyDialOption(t *testing.T) {


It took a good bit of learning this to figure out what it's even doing. A short comment to say "configure a global late dial option that produces TransportCredentials for channels using the passthrough scheme, which makes those channels valid but others invalid" would be really helpful.

Added comment: "TestGlobalLateApplyDialOption configures a global dial option that produces transport credentials for channels using "passthrough" scheme. Channels that use the passthrough scheme should this be successfully created due to picking up transport credentials, whereas other channels should fail at creation due to not having transport credentials."

dfawley · 2024-05-15T17:57:20Z

default_dial_option_server_option_test.go

+func (s) TestGlobalLateApplyDialOption(t *testing.T) {
+	internal.AddGlobalLateApplyDialOptions.(func(opt ...LateApplyDialOption))(&testLateApplyDialOption{}) // Do I need a clear?
+	noTSecStr := "no transport security set"
+	if _, err := NewClient("fake"); !strings.Contains(fmt.Sprint(err), noTSecStr) {


An explicit dns:/// would be helpful here.

Good point. Done.

dfawley · 2024-05-16T20:14:09Z

clientconn.go

@@ -152,6 +152,16 @@ func NewClient(target string, opts ...DialOption) (conn *ClientConn, err error)
 	for _, opt := range opts {
 		opt.apply(&cc.dopts)
 	}
+
+	// Determine the resolver to use.
+	if err := cc.parseTargetAndFindResolver(); err != nil {


Would you mind converting this and cc.determineAuthority so they are not cc methods? Otherwise they might forget they're not supposed to be using cc.channelz.

This reads and writes a lot of stuff from cc. Unless you want me to pass in the target, dial options, and return a bunch of things to NewClient to write to cc's stuff. For the sake of readability I think it's better to keep it on cc.

This used to all be in the function body inline and Easwar rewrote it (which I reviewed) to be on cc to factor out functionality into readable helpers and keep this function body clean.

I guess let's rename these to initParsedTargetAndResolverBuilder and initAuthority and then at least it's obvious they're being called before the cc is fully initialized.

dfawley · 2024-05-16T20:15:07Z

clientconn.go

 		return nil, err
 	}

+	// Register ClientConn with channelz.


Maybe something that says "Note that this is only done after channel creation cannot fail", so that nobody adds error cases after this. Maybe also move immediately before the return to make it doubly sure.

Added comment. I don't think I can move it right before return because the component creation reads the channelz pointer of the channel to log in future, and if I move to right before return would read off a nil pointer and not log anything.

clientconn.go

dfawley · 2024-05-16T20:17:31Z

clientconn.go


 	var rb resolver.Builder
 	parsedTarget, err := parseTarget(cc.target)
 	if err != nil {
-		channelz.Infof(logger, cc.channelz, "dial target %q parse failed: %v", cc.target, err)
+		logger.Infof("dial target %q parse failed: %v", cc.target, err)


This doesn't seem worth keeping. We're going to try some more stuff that might make it valid, and something as simple as hostname:port will trigger this.

Deleted (and switched conditional to err == nil).

dfawley · 2024-05-16T20:17:48Z

clientconn.go

 	} else {
-		channelz.Infof(logger, cc.channelz, "parsed dial target is: %#v", parsedTarget)
+		logger.Infof("parsed dial target is: %#v", parsedTarget)


Move to NewClient please.

dfawley · 2024-05-16T20:19:01Z

clientconn.go

@@ -1838,6 +1837,5 @@ func (cc *ClientConn) determineAuthority() error {
 	} else {
 		cc.authority = encodeAuthority(endpoint)
 	}
-	channelz.Infof(logger, cc.channelz, "Channel authority set to %q", cc.authority)


dfawley · 2024-05-16T20:19:52Z

default_dial_option_server_option_test.go

+
+// TestGlobalPerTargetDialOption configures a global per target dial option that
+// produces transport credentials for channels using "passthrough" scheme.
+// Channels that use the passthrough scheme should this be successfully created


"This" makes no sense. 😆

(Delete "this")

Whoops haha, deleted.

dfawley · 2024-05-16T20:20:49Z

default_dial_option_server_option_test.go

+	if err != nil {
+		t.Fatalf("Dialing with insecure credentials failed: %v", err)
+	}
+	defer cc.Close()


Nit: just cc.Close()?

Sure, changed.

dfawley · 2024-05-16T20:21:03Z

default_dial_option_server_option_test.go

+		t.Fatalf("Dialing with insecure credentials failed: %v", err)
+	}
+	defer cc.Close()
+	internal.ClearGlobalPerTargetDialOptions()


Defer at the start, otherwise if this fails it impacts other tests.

Changed, and also changed the other tests in this file I wrote for gcp observability :).

dfawley · 2024-05-16T20:21:53Z

dialoptions.go

+// configuration set through a returned DialOption.
+type perTargetDialOption interface {
+	// DialOption returns a Dial Option to apply.
+	DialOption(parsedTarget url.URL) DialOption


Bikeshed: DialOptionForTarget?

Sure, done.

zasweq requested a review from dfawley May 14, 2024 22:20

zasweq assigned dfawley May 14, 2024

zasweq added the Type: Feature New features or improvements in behavior label May 14, 2024

zasweq added this to the 1.65 Release milestone May 14, 2024

zasweq force-pushed the late-apply-dial-option branch 2 times, most recently from 873f67f to e90323f Compare May 14, 2024 22:45

Add LateApplyDialOption and global list

4dd19b1

zasweq force-pushed the late-apply-dial-option branch from e90323f to 4dd19b1 Compare May 14, 2024 22:46

dfawley reviewed May 15, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley May 15, 2024

Responded to Doug's comments

d8a39a4

zasweq assigned dfawley and unassigned zasweq May 15, 2024

dfawley reviewed May 16, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley May 16, 2024

Responded to Doug's comments

0e061b3

zasweq assigned dfawley and unassigned zasweq May 17, 2024

Fix defer

1621bbd

dfawley assigned zasweq and unassigned dfawley May 20, 2024

Responded to Doug's comments

8120f8e

dfawley approved these changes May 20, 2024

View reviewed changes

zasweq changed the title ~~grpc: Add LateApplyDialOption type and global list~~ grpc: Add perTargetDialOption type and global list May 20, 2024

zasweq merged commit aea78bd into grpc:master May 21, 2024
12 checks passed

		for _, lateApplyOpt := range globalLateApplyDialOptions {
		lateApplyOpt.DialOption(cc.parsedTarget.URL).apply(&cc.dopts)

grpc: Add perTargetDialOption type and global list #7234

grpc: Add perTargetDialOption type and global list #7234

Conversation

zasweq commented May 14, 2024

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

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

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

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

zasweq May 16, 2024 • edited

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

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

zasweq May 16, 2024 •

edited