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.

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: Add proxy dialer support. #481

Closed

Conversation

nmische
Copy link
Contributor

@nmische nmische commented Sep 4, 2024

This PR ports the internal grpc-go dialer with proxy support into the grpcurl package such that grpcurl can support idiomatic proxy configuration via the HTTPS_PROXY environment variable. The port does have a minor modification in that it uses the standard lib's net.Dialer rather than the NetDialerWithTCPKeepalive from the grpc-go's internal package.

This is an alternative to PR #480.

Addresses #166.

@nmische nmische mentioned this pull request Sep 4, 2024
@nmische
Copy link
Contributor Author

nmische commented Sep 4, 2024

NOTE: I have not ported any of the tests over, but I can do that if this approach is preferable to #480.

@nmische nmische mentioned this pull request Sep 6, 2024
}

// NOTE: Use net.Dialer to avoid dependency on grpc-go's internal package
// conn, err := internal.NetDialerWithTCPKeepalive().DialContext(ctx, "tcp", newAddr)
Copy link
Member

Choose a reason for hiding this comment

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

do we need to then manually set keepalive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The current grpcurl dialer doesn't set keepalive and the grpc-go dailer only sets keepalive for select platforms, others get a vanilla dialer.

@@ -37,6 +37,8 @@ import (
"google.golang.org/protobuf/types/known/structpb"
)

var GrpcurlUA string

Copy link
Member

Choose a reason for hiding this comment

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

this seems.... unfortunate...

@dragonsinth
Copy link
Member

#480

@dragonsinth dragonsinth closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants