-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
grpc: Add perTargetDialOption type and global list #7234
Conversation
873f67f
to
e90323f
Compare
e90323f
to
4dd19b1
Compare
clientconn.go
Outdated
for _, lateApplyOpt := range globalLateApplyDialOptions { | ||
lateApplyOpt.DialOption(cc.parsedTarget.URL).apply(&cc.dopts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: opt
(as above) or even o
is preferred for very narrowly scoped vars like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Switched to opt.
clientconn.go
Outdated
|
||
// Register ClientConn with channelz. | ||
cc.channelzRegistration(target) | ||
// TODO: Ideally it should be impossible to error from this function after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dialoptions.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexport? Move to internal
? I really don't want users seeing this hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed further offline; unexported and typecast from any in SetGlobalDialOptions.
internal/internal.go
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (or something) should explain what these magic things can or cannot do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. WithResolvers
doesn't work. Anything else?
} | ||
|
||
func (s) TestGlobalLateApplyDialOption(t *testing.T) { | ||
internal.AddGlobalLateApplyDialOptions.(func(opt ...LateApplyDialOption))(&testLateApplyDialOption{}) // Do I need a clear? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted. The answer is duh, otherwise other tests would pick up globals :).
if parsedTarget.Scheme == "passthrough" { | ||
return WithTransportCredentials(insecure.NewCredentials()) // credentials provided, should pass NewClient. | ||
} | ||
return WithReadBufferSize(10) // no credentials, should fail NewClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use EmptyDialOption
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Switched.
dialoptions.go
Outdated
DialOption(parsedTarget url.URL) DialOption | ||
} | ||
|
||
var globalLateApplyDialOptions []LateApplyDialOption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ditch this name since they don't have a lateApply
method? How about perTargetDialOption
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright will switch to perTarget.
return WithReadBufferSize(10) // no credentials, should fail NewClient | ||
} | ||
|
||
func (s) TestGlobalLateApplyDialOption(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explicit dns:///
would be helpful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Done.
clientconn.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
clientconn.go
Outdated
return nil, err | ||
} | ||
|
||
// Register ClientConn with channelz. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted (and switched conditional to err == nil).
clientconn.go
Outdated
} else { | ||
channelz.Infof(logger, cc.channelz, "parsed dial target is: %#v", parsedTarget) | ||
logger.Infof("parsed dial target is: %#v", parsedTarget) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Move to
NewClient
please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This" makes no sense. 😆
(Delete "this")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops haha, deleted.
if err != nil { | ||
t.Fatalf("Dialing with insecure credentials failed: %v", err) | ||
} | ||
defer cc.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just cc.Close()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed.
t.Fatalf("Dialing with insecure credentials failed: %v", err) | ||
} | ||
defer cc.Close() | ||
internal.ClearGlobalPerTargetDialOptions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defer at the start, otherwise if this fails it impacts other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, and also changed the other tests in this file I wrote for gcp observability :).
dialoptions.go
Outdated
// configuration set through a returned DialOption. | ||
type perTargetDialOption interface { | ||
// DialOption returns a Dial Option to apply. | ||
DialOption(parsedTarget url.URL) DialOption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed: DialOptionForTarget
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done.
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