Skip to content
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.

Sign up for GitHub

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

feat: 166: Proxy support #480

Merged
merged 1 commit into from
Oct 3, 2024
Merged

feat: 166: Proxy support #480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions grpcurl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,21 +645,6 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
writeResult: writeResult,
}

dialer := func(ctx context.Context, address string) (net.Conn, error) {
// NB: We *could* handle the TLS handshake ourselves, in the custom
// dialer (instead of customizing both the dialer and the credentials).
// But that requires using insecure.NewCredentials() dial transport
// option (so that the gRPC library doesn't *also* try to do a
// handshake). And that would mean that the library would send the
// wrong ":scheme" metaheader to servers: it would send "http" instead
// of "https" because it is unaware that TLS is actually in use.
conn, err := (&net.Dialer{}).DialContext(ctx, network, address)
if err != nil {
writeResult(err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhump you probably have the most context on the custom dialer... maybe there was a desire to intercept dial errors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick feedback @dragonsinth! I had a similar thought, but I believe any dialer errors would be handled in a similar fashion (via writeResult()) here.

Unfortunatley the default grpc-go dialer is in an internal package, so if there is a desire to keep this error handling here I think the grpc-go dialer code would need to be ported over to grpcurl. I actually started down that path before opting for this approach. I would be happy to submit that PR as well if it would help evaluate how to address this feature request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if jhump has some "context" on this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dragonsinth and @jhump, I went ahead and posted PR #481 which hopefully helps determine a path forward for proxy support gprcurl. Thanks!

}
return conn, err
}

// Even with grpc.FailOnNonTempDialError, this call will usually timeout in
// the face of TLS handshake errors. So we can't rely on grpc.WithBlock() to
// know when we're done. So we run it in a goroutine and then use result
Expand All @@ -670,7 +655,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
opts = append([]grpc.DialOption{grpc.FailOnNonTempDialError(true)}, opts...)
// But we don't want caller to be able to override these two, so we put
// them *after* the explicitly provided options.
opts = append(opts, grpc.WithBlock(), grpc.WithContextDialer(dialer), grpc.WithTransportCredentials(creds))
opts = append(opts, grpc.WithBlock(), grpc.WithTransportCredentials(creds))

conn, err := grpc.DialContext(ctx, address, opts...)
var res interface{}
Expand Down
Loading