-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
xds: add xDS transport custom Dialer support #7586
base: master
Are you sure you want to change the base?
xds: add xDS transport custom Dialer support #7586
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7586 +/- ##
==========================================
+ Coverage 81.74% 81.87% +0.12%
==========================================
Files 361 361
Lines 27813 27822 +9
==========================================
+ Hits 22736 22778 +42
+ Misses 3868 3849 -19
+ Partials 1209 1195 -14
|
99a59fc
to
354e088
Compare
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.
Could you add a test that works at an even higher level? I.e. registers the creds builder from outside of the xdsclient package and confirms that a grpc client created using grpc.NewClient
will invoke the proper dialer?
EDIT: finished comments; oops!
(Sorry, hit send too soon; updated comment above.) |
Done. The test setup is inspired by other tests under |
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 aside from the two small comments -- thanks!
defer cc.Close() | ||
|
||
client := testgrpc.NewTestServiceClient(cc) | ||
if _, err := client.EmptyCall(ctx, &testpb.Empty{}, grpc.WaitForReady(true)); 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.
I don't think (?) this should require WaitForReady
-- we shouldn't be expecting connections to fail as we start the server before the client.
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.
Removed grpc.WaitForReady(true)
call option. It makes sense as the server is indeed started before the client.
Tested with thread sanitizer and ran test 100 times and ensured there are no flakes.
} | ||
|
||
func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) { | ||
customDialerCalled = false |
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 this be a field in the creds bundle instead of a global? I think we should be able to do the RegisterCredentials
here instead of in an init
.
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.
Same for the mgmtServerAddress
global. Or if you want to be a little fancy, it can be passed through the bootstrap config, and testDialerCredsBuilder.Build
can parse it and pass it to the testDialerCredsBundle
that it builds.
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.
And the testDialerCredsBuilder
needs to have a channel on which it sends the testDialerCredsBundle
that it creates. That way, the test can get access to the creds bundle and can check if the custom dialer was called.
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.
Removed all global variables (customDialerCalled
and mgmtServerAddress
) in this test.
- The
mgmtServerAddress
is passed through the bootstrapconfig
wheretestDialerCredsBuilder.Build
unmarshals the JSON config and passes it to thetestDialerCredsBundle
. - Added a
chan struct{}
totestDialerCredsBuilder
andtestDialerCredsBundle
such that the test can check whether the custom dialer has been called. Followed Go Tip 79 for one-time signaling and simply usedclose(chan)
along withselect
statements. Furthermore, added a case forctx.Done()
in theselect
to gracefully handle the case if the dial failed due to timeout.
Called bootstrap.RegisterCredentials()
within this test and removed init()
.
+@easwars for a second review |
internal/xds/bootstrap/bootstrap.go
Outdated
// DialerOption returns the first supported Dialer function that specifies how | ||
// to dial the xDS server from the configuration, as a dial option. |
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 this comment be rephrased to indicate that it is actually the first supported credentials that determines this dialer function. There is really no such thing as the first supported dialer function.
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.
Rephrased comment to include that this Dialer function is determined by the first supported credentials from the configuration.
// dialer captures the Dialer method specified via the credentials bundle. | ||
type dialer interface { | ||
// Dialer specifies how to dial the xDS server. | ||
Dialer(context.Context, string) (net.Conn, error) |
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 this method name be more specific? Maybe something like DialXDS
or DialXDSServer
or something else?
The reason I'm suggesting this is because the API to register a credentials type is public. So, users could register their own credentials using bootstrap.RegisterCredentials
, and if they end up having a Dialer
method on their creds implementation type (which is maybe doing something else), then, they would be in for a surprise.
Or at least, we should document this in the public API in package xds/bootstrap
?
Thoughts @dfawley
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.
Acknowledged.
Users could definitely have a the Dialer
method on their creds bundle implementation and it is a valid concern. We have no idea or control of what users could implement. Using another "less common" name compared to Dialer
would greatly reduce this risk.
I think DialXDS
would be appropriate and not too verbose, although this would introduce slight repetition as the XDS
suffix is already implied (per the Repetition section of the Google Go Style Decisions guide).
What are your thoughts @dfawley? If everyone is in agreement, then I will make the change.
} | ||
|
||
func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) { | ||
customDialerCalled = false |
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.
Same for the mgmtServerAddress
global. Or if you want to be a little fancy, it can be passed through the bootstrap config, and testDialerCredsBuilder.Build
can parse it and pass it to the testDialerCredsBundle
that it builds.
} | ||
|
||
func (s) TestClientCustomDialerFromCredentialsBundle(t *testing.T) { | ||
customDialerCalled = false |
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.
And the testDialerCredsBuilder
needs to have a channel on which it sends the testDialerCredsBundle
that it creates. That way, the test can get access to the creds bundle and can check if the custom dialer was called.
func (sc *ServerConfig) DialerOption() grpc.DialOption { | ||
return sc.dialerOption | ||
} |
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.
Hmm ... I'm thinking if we should just have a method to return a slice of dial options from this type:
func (sc *ServerConfig) DialOptions() []grpc.DialOption {
// return a slice of dial options that contains the creds dial option and optionally one for the dialer
}
With this approach, the transport can be oblivious to the dial options, and tomorrow if another dial option needs to be added here, then the transport won't have to change.
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.
Acknowledged.
I think this is a good abstraction, although it might slightly reduce readability and intent of what each dial option does (only two dial options passed to the transport).
I think this abstraction would be appropriate in a follow-up PR as it would involve removing the existing func (sc *ServerConfig) CredsDialOption()
, introduce the new API above, and refactoring the transport, which is unrelated to the current PR of adding a custom Dialer used by the xDS transport and it would reduce this PR's focus/intent/readability.
// testDialerCredsBundle implements the `Bundle` interface defined in package | ||
// `credentials` and encapsulates an insecure credential with a custom Dialer | ||
// that specifies how to dial the xDS server. | ||
type testDialerCredsBundle struct{} |
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 embedding an credentials.Bundle
here and setting it to insecure.NewBundle
from testDialerCredsBuilder.Build
, help to get rid of the implementation of the three methods below?
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.
Great find and suggestion!
Embedded credentials.Bundle
for the struct in both transport_test.go
and xds_client_custom_dialer_test.go
.
This follows from Go Tip 7 and eliminated unnecessary boilerplate methods and improved readability.
return nil, nil | ||
} | ||
|
||
func (s) TestNewWithDialerFromCredentialsBundle(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.
I don't think this really tests whether the transport is actually passing the new dial option to grpc.Dial
. It is not possible to check the exact dial options being passed because dial options are implemented as functions, and it is not possible to compare them. But at least, we should check if the call to grpc.Dial
or grpc.NewClient
from the transport passes the expected number of dial options.
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.
Updated this test such that initially it overrides internal.GRPCNewClient
with a custom pass-through grpc.NewClient
(then resets back to the original grpc.NewClient
as to not have order-dependent tests) that gets the number of dial options. Later the test verifies the number of dial options passed to the custom grpc.NewClient
is 3. Also, added a comment explaining why it is 3.
…ent for transport test
Design:
Dialer
method that specifies how to dial the xDS server via the credentials bundle without introducing new public APIs.Tested:
./scripts/vet.sh
go test -cpu 1,4 -timeout 7m ./...
go test -race -cpu 1,4 -timeout 7m ./...
RELEASE NOTES: