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

Proxy settings overrides curl option globally with http transport #665

Open
mrpavlikov opened this issue Oct 18, 2021 · 8 comments
Open
Labels
bug Something isn't working difficulty: medium Intermediate difficulty; some experience with codebase may be helpful P2 Work that should be prioritized. Bugs classified here have known reasonable workarounds

Comments

@mrpavlikov
Copy link

mrpavlikov commented Oct 18, 2021

Your client library and Google Ads API versions:

  • Client library version: v10.1.0
  • Google Ads API version: V8

Your environment:

  • PHP Version => 7.3.29-1~deb10u1
  • System => Linux 18c018ba8d92 5.11.0-37-generic #41-Ubuntu SMP Mon Sep 20 16:39:20 UTC 2021 x86_64
  • The PHP Extension grpc is not installed
  • The PHP Extension protobuf is not installed
  • REST transport

Description of the bug:

Using proxy settings in .ini file or having it set manually while building client with (new GoogleAdsClientBuilder())->withProxy('protocol://user:pass@host:port') sets environment variable http_proxy and after that everything that uses curl automatically starts using this proxy too, which is not desired behavior and kinda unexpected side effect.

Steps to reproduce:

$ini = 'path/to/file.ini';
$oAuth2Credential = (new OAuth2TokenBuilder())->fromFile($ini)->build();
$googleAdsClient = (new GoogleAdsClientBuilder())
    ->fromFile($ini)
    ->withOAuth2Credential($oAuth2Credential)
    ->withProxy('protocol://user:pass@host:port')
    ->build();
$client = $googleAdsClient->getGoogleAdsServiceClient();

// now this will use proxy too
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, "google.com");
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
$output = curl_exec($ch);
curl_close($ch);

Expected behavior:

Curl calls must stay unaffected.

@mrpavlikov mrpavlikov added bug Something isn't working triage Need triage labels Oct 18, 2021
@PierrickVoulet PierrickVoulet removed the triage Need triage label Oct 22, 2021
@PierrickVoulet
Copy link
Collaborator

Hi @mrpavlikov - Thank you for reporting.

Workaround: You should be able to override existing environment variables that set the proxy to use by setting the curl option proxy to an empty string. See the curl man page and curl_setopt documentation.

Potential fix: The library could set the proxy from the gRPC channel options instead of setting environment variables. See grpc.http_proxy and Python's implementation.

@PierrickVoulet PierrickVoulet added difficulty: medium Intermediate difficulty; some experience with codebase may be helpful P2 Work that should be prioritized. Bugs classified here have known reasonable workarounds labels Oct 22, 2021
@mrpavlikov
Copy link
Author

So far I'm doing putenv('http_proxy'); after library call to erase global proxy setting, but thanks!

@PierrickVoulet PierrickVoulet removed their assignment Nov 5, 2021
@mrpavlikov
Copy link
Author

Is it "won't fix" or something? Like affecting global curl setting is fine?

@fiboknacky fiboknacky reopened this Nov 9, 2021
@fiboknacky
Copy link
Member

Sorry, it was a mistake. Let's keep this open for now.

@hombo86
Copy link

hombo86 commented Feb 6, 2023

This problem still exists. The current method of setting environment variables in the source code has no effect, and the value of the proxy parameter is not transparently transmitted to the underlying guzzle and curl.
Client library version: v12
Composer version:v17.1

@ictdevel
Copy link

ictdevel commented Apr 26, 2023

For grpc transport it's need to use grpc.http_proxy='proxy_address'

$options['transportConfig']['grpc']['stubOpts']['grpc.http_proxy'] = $this->getProxy();

But since the GoogleAdsClient class is final, it is not possible to override the getGoogleAdsClientOptions method in the proper way

And withProxy doesn't work for rest transport at all

@hombo86
Copy link

hombo86 commented May 29, 2023

For grpc transport it's need to use grpc.http_proxy='proxy_address'

$options['transportConfig']['grpc']['stubOpts']['grpc.http_proxy'] = $this->getProxy();

But since the GoogleAdsClient class is final, it is not possible to override the getGoogleAdsClientOptions method in the proper way

And withProxy doesn't work for rest transport at all

yes

@ictdevel
Copy link

ictdevel commented Dec 15, 2023

Since version 14, GoogleAdsClient is no longer a final class and therefore it is easy to extend from it and override the method

    public function getGoogleAdsClientOptions(): array
    {
        $options = parent::getGoogleAdsClientOptions();
        $options['transportConfig']['grpc']['stubOpts']['grpc.http_proxy'] = $this->getProxy();

        return $options;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working difficulty: medium Intermediate difficulty; some experience with codebase may be helpful P2 Work that should be prioritized. Bugs classified here have known reasonable workarounds
Projects
None yet
Development

No branches or pull requests

5 participants