feat(bigquery): add proxy support for API calls - #2941
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for an optional HTTP(S) proxy in the BigQuery source, allowing API calls to be routed through a proxy server. The changes include updates to the configuration struct, documentation, and the initialization logic for BigQuery clients and services. However, the current implementation is incomplete as the proxy configuration is not applied to Dataplex client initialization or service account impersonation calls, which may cause failures in restricted environments. Additionally, the documentation should be generalized to reflect that the proxy affects all outbound API calls from the source, not just BigQuery-specific ones.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for an optional HTTP(S) proxy in the BigQuery source, allowing API calls to be routed through a proxy server. The changes include updates to the configuration schema, documentation, and the internal client initialization logic for both BigQuery and Dataplex. However, several critical issues were identified in the review: the current implementation creates a new http.Transport for every client request which can lead to resource exhaustion, the proxy configuration is currently ignored for gRPC-based operations (the default for these clients), and the casting of the default transport is brittle and could cause runtime failures if the transport is wrapped by other libraries.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for HTTP(S) proxies in the BigQuery source. It adds a proxy configuration field, updates documentation, and refactors the internal connection logic to use proxy-aware HTTP clients for both BigQuery and Dataplex services. Feedback suggests using http.ProxyFromEnvironment for more robust proxy detection and injecting the proxy-aware client into the context via oauth2.HTTPClient to ensure authentication requests also honor the proxy settings.
529d19f to
324d148
Compare
a2d6191 to
865f60f
Compare
|
hi @Genesis929 do we have any update on this ? |
Genesis929
left a comment
There was a problem hiding this comment.
LGTM, but we may better have related integration tests.
|
Hi @Deeven-Seru would you mind adding integration tests for this? Thank you |
|
done @Yuan325 I have added the integration test,a single test but I believe it covers the scope. |
8ea22b9 to
101d23b
Compare
101d23b to
f4e1bd9
Compare
f4e1bd9 to
cd1212f
Compare
|
Hi @Deeven-Seru, Thank you so much for the time and effort you put into this pull request, and for following up with the integration tests! We've decided to move forward with an alternative approach proposed in PR #3437 to solve this routing issue. That PR introduces an Please let us know if #3437 would miss any scenario we are trying to address in this PR, or else we would like to close this PR. We really appreciate your willingness to contribute and help improve the project! |
Summary
proxyfield to BigQuery source configexplicit proxy or HTTP(S)_PROXY)
This wasn’t supported yet — BigQuery client creation didn’t wire proxy transports, so HTTPS_PROXY/HTTP_PROXY were ignored.
this PR that adds a
proxyfield on the BigQuery source config and routes both the client and REST service through a proxy-aware HTTP client (explicit proxy or HTTP(S)_PROXY). Docs updated.Example:
proxy: "https://proxy.company:8443"
fixes #2766