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

Add channel arg to enable/disable http proxy #15699

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Conversation

yashykt
Copy link
Member

@yashykt yashykt commented Jun 9, 2018

Add channel arg to enable/disable http proxy.
Feature request from #11751

Currently, the recommended approach to disable proxy is to set the no_proxy environment variable. This environment variable is not suitable to be used with IP ranges.

After this change, we would still be supporting the 'no_proxy' environment variable to disable proxy usage for those names across gRPC. The additional capability that the new channel arg would provide is that we can now disable proxy use for a particular channel. This also means that usecases that need to disable proxy use for an entire range of IPs and hence cannot use the 'no_proxy' env variable to disable proxy usage, now have an option to use this channel arg to disable proxy use.
Please note that this channel argument doesn't take the names/domains/addresses to be blacklisted. It simply disables proxy usage for this channel.

Given that we are adding an argument to disable http proxy, it might also make sense in the future to provide an argument that explicitly provides the http proxy to use. Currently, we just use the 'http_proxy' environment variable.

@grpc-testing
Copy link

****************************************************************

libgrpc.so

     VM SIZE                                                         FILE SIZE
 ++++++++++++++ GROWING                                           ++++++++++++++
  +0.0%    +128 [None]                                            +1.32Ki  +0.0%
  +5.6%     +96 src/core/ext/filters/client_channel/http_proxy.cc     +96  +5.6%
      +3.6%     +59 proxy_mapper_map_name                                 +59  +3.6%
      [NEW]     +33 http_proxy_enabled                                    +33  [NEW]
      +9.5%      +4 [Unmapped]                                             +4  +9.5%

  +0.0%    +224 TOTAL                                             +1.41Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link

[trickle] No significant performance differences

@grpc-testing
Copy link

[microbenchmarks] No significant performance differences

@yashykt
Copy link
Member Author

yashykt commented Jun 11, 2018

@AspirinSJL can you do the primary reviewing please?

@yashykt yashykt added the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2018
@AspirinSJL
Copy link
Member

@yashykt Sure. Looking.

@AspirinSJL
Copy link
Member

LGTM.

But could you explain a bit more about how this channel arg is better than the old environment variable way? Like, how the arg is suitable to be used with IP ranges?

@yashykt
Copy link
Member Author

yashykt commented Jun 12, 2018

Updated the PR comment.

This arg does not have a value of any name/domain/address. It just disables proxy use for this channel. (Since, a channel would have a single name, it won't make much sense to accept a list of names to blacklist for proxy usage.)

Copy link
Member

@AspirinSJL AspirinSJL left a comment

Choose a reason for hiding this comment

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

Thanks!

@yashykt
Copy link
Member Author

yashykt commented Jun 12, 2018

Known Issues : #15610
Thanks for reviewing!

@yashykt yashykt merged commit 56e59ef into grpc:master Jun 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018
@yashykt yashykt deleted the noproxyarg branch October 26, 2018 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants