Skip to content

feat: 166: Add proxy dialer support.#481

Closed
nmische wants to merge 1 commit intofullstorydev:masterfrom
nmische:nmische/166-proxy-dialer
Closed

feat: 166: Add proxy dialer support.#481
nmische wants to merge 1 commit intofullstorydev:masterfrom
nmische:nmische/166-proxy-dialer

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.

)

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