SNOW-1820372: (from 947) when proxy is set in Connection, driver does send traffic through the proxy to S3, but not to Azure blob / GCS bucket (only Snowflake). Works with proxy envvar (GCP) #982
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #982 +/- ##
==========================================
+ Coverage 88.91% 88.99% +0.08%
==========================================
Files 72 72
Lines 6944 7006 +62
==========================================
+ Hits 6174 6235 +61
- Misses 770 771 +1 ☔ View full report in Codecov by Sentry. |
sfc-gh-ext-simba-jy
changed the title
sfc-gh-ext-simba-jy
requested review from
sfc-gh-dprzybysz and
sfc-gh-pmotacki
lib/util.js
Outdated
| @@ -740,6 +740,10 @@ exports.getEnvVar = function (variable) { | |||
| return process.env[variable.toLowerCase()] || process.env[variable.toUpperCase()]; | |||
| }; | |||
|
|
|||
| exports.restoreEnvVar = function (envVar, value) { | |||
There was a problem hiding this comment.
Do we still need this method and its test?
There was a problem hiding this comment.
No, we do not need it anymore. I removed it. Thank you!
lib/file_transfer_agent/gcs_util.js
Outdated
| isProxyEnabled = true; | ||
| const proxyAgent = getProxyAgent(proxy, new URL(connectionConfig.accessUrl), endPoint || `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` ); | ||
| axios = require('axios').create({ | ||
| proxy: false, |
There was a problem hiding this comment.
Is it correct to set here proxy = false?
There was a problem hiding this comment.
Yes, we need to set proxy: false if we want to use https-proxy-agent in Axios. If not, the driver will face the 'too many direction' error. The same implementation is here: https://github.com/snowflakedb/snowflake-connector-nodejs/blob/master/lib/http/base.js#L289
lib/file_transfer_agent/gcs_util.js
Outdated
| client = { gcsToken: gcsToken, gcsClient: storage }; | ||
| } else { | ||
| client = null; | ||
| } | ||
|
|
||
| if (typeof httpClient === 'undefined') { | ||
| axios = require('axios'); |
There was a problem hiding this comment.
The line could be a part of if ... else when we confirm whether the proxy is set or not.
if (proxy || Util.getEnvVar('http_proxy')) {
...
} else {
axios = require('axios');
}
lib/file_transfer_agent/gcs_util.js
Outdated
| client = { gcsToken: gcsToken, gcsClient: storage }; | ||
| } else { | ||
| client = null; | ||
| } | ||
|
|
||
| if (typeof httpClient === 'undefined') { |
There was a problem hiding this comment.
The code of setting up axios could be extracted to the separated method.
Description
Please explain the changes you made here.
Checklist
npm run lint:check -- CHANGED_FILESand fix problems in changed code)npm run test:unitandnpm run test:integration)