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: Proxy support #480

Merged

Conversation

nmische
Copy link
Contributor

@nmische nmische commented Sep 1, 2024

This PR removes the custom dialer used by grpcurl such that the default grpc-go dialer is used. The default grpc-go dialer supports proxy configuration similar the net.http package (e.g. ProxyFromEnvironment).

I'm assuming this is a safe change as all the tests continue to pass and I can now use the HTTPS_PROXY environment variable to route grpcurl calls through a proxy. That said, it is not clear to me why a custom dialer was used in the first place, so it is possible I am missing something here. (Based on my reading of the code, the custom dialer doesn't seem to add any additional functionality above and beyond a default net.Dialer.)

I have been using grpcurl for a couple of years now, but have only recently needed to use it with a proxy, so I would appreciate any feedback on this PR. Thanks!

Addresses #166

@nmische nmische changed the title fix: 166: Remove custom dialer. feat: 166: Remove custom dialer. Sep 1, 2024
@nmische nmische changed the title feat: 166: Remove custom dialer. feat: 166: Proxy support Sep 1, 2024
// 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!

@dragonsinth dragonsinth merged commit cdb43b0 into fullstorydev:master Oct 3, 2024
7 checks passed
@stuartcarnie
Copy link

I reverted this patch and it resolves #496

@zhyuri zhyuri mentioned this pull request Dec 19, 2024
4 tasks
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.

3 participants