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

More flexibility in configuration of client proxy(#1446) #1447

Closed
wants to merge 1 commit into from
Closed

More flexibility in configuration of client proxy(#1446) #1447

wants to merge 1 commit into from

Conversation

denchenko
Copy link

@denchenko denchenko commented Aug 17, 2017

  • ProxyURL client option
  • Posibility to use credentials from proxy string

fixes #1446

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@denchenko
Copy link
Author

denchenko commented Aug 17, 2017 via email

@googlebot
Copy link

CLAs look good, thanks!

* ProxyURL client option
* Posibility to use credentials from proxy string
@denchenko
Copy link
Author

Fixed tests and added case for using custom proxy.

@menghanl
Copy link
Contributor

Thanks for the contribution and sorry for the late reply.

One issue with your proposed fix is that, the new dial option WithProxyURL is too specific for this problem. And it would be better if we can get a more general solution.

A proxy dialer actually consists of three parts:

  • mapper from the target address to the proxy server address (for example, get proxy env variable)
  • dialer
  • handshaker

Currently, we have these three components, but they are not explicitly called out and not exported.

The proposal we make here is to make the mapper and handshaker public, so users can compose their custom proxy dialer as they want, using these components.

This includes:

  • Add a new proxy package
  • Move code in proxy.go to the new proxy package
  • Export ProxyFromEnv and HTTPConnectHandshake
    • ProxyFromEnv would return one more http.Header value for credentials
    • HTTPConnectHandshake would take one more http.Header parameter
  • Add a new function which takes a mapper, a dialer and a handshaker and returns a dialer for gRPC

With the above changes, for your particular problem, you would just need to provide your mapper (which always returns your specified URL and http.Header field containing your creds), and build a dialer with the existing handshaker.

Please let us know if you want to make the implementation as we suggested.

If you don't have time to implement all of those and want a solution for your issue, a walkaround would be to implement your own proxy dialer, which may need copying some of the function from proxy.go, and dial using WithDialer.

@dfawley dfawley added the Type: Feature New features or improvements in behavior label Aug 24, 2017
@denchenko
Copy link
Author

@dfawley no problem. I will implement it asap.

@menghanl menghanl assigned denchenko and unassigned menghanl Aug 31, 2017
@menghanl
Copy link
Contributor

Ping. Are you still planning on implementing this?

@denchenko
Copy link
Author

@menghanl sorry for delay, I was on vacation. I am planning to implement this on Monday.

@menghanl
Copy link
Contributor

menghanl commented Oct 2, 2017

@shevchenkodenis No worries! Thanks for implementing this!

@menghanl
Copy link
Contributor

Ping. Just want to check what the status of this is.

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
The Linux Foundation CLA GitHub bot

@menghanl
Copy link
Contributor

Closing the PR now.
Please leave a comment if you want to revive this.

@menghanl menghanl closed this Nov 30, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting proxy parameters is not flexible enough
5 participants