added more testing and refactored the code

snowflakedb · Dec 10, 2024 · cb839a6 · cb839a6
1 parent 4233eb8
commit cb839a6
Show file tree

Hide file tree

Showing 10 changed files with 271 additions and 36 deletions.
diff --git a/index.d.ts b/index.d.ts
@@ -512,6 +512,11 @@ declare module 'snowflake-sdk' {
          *  The option to pass passcode from DUO.
          */
         passcode?: string;
+
+        /**
+         *  The option to override the environment proxy value (HTTPS_PROXY) if the connection proxy is configured in GCS
+         */
+        overrideEnvProxy?: string;
     }
 
     export interface Connection {

diff --git a/lib/connection/connection_config.js b/lib/connection/connection_config.js
@@ -515,12 +515,12 @@ function ConnectionConfig(options, validateCredentials, qaMode, clientInfo) {
     passcode = options.passcode;
   }
 
-  let overwriteEnvProxy = true;
-  if (Util.exists(options.overwriteEnvProxy)) {
-    Errors.checkArgumentValid(Util.isBoolean(options.overwriteEnvProxy), 
-      ErrorCodes.ERR_CONN_CREATE_INVALID_OVERWRITE_ENV_PROXY);
+  let overrideEnvProxy = true;
+  if (Util.exists(options.overrideEnvProxy)) {
+    Errors.checkArgumentValid(Util.isBoolean(options.overrideEnvProxy), 
+      ErrorCodes.ERR_CONN_CREATE_INVALID_OVERRIDE_ENV_PROXY);
 
-    overwriteEnvProxy = options.overwriteEnvProxy;
+    overrideEnvProxy = options.overrideEnvProxy;
   } 
 
   if (validateDefaultParameters) {
@@ -845,8 +845,8 @@ function ConnectionConfig(options, validateCredentials, qaMode, clientInfo) {
     return passcode;
   };
 
-  this.getOverwriteEnvProxy = function () {
-    return overwriteEnvProxy;
+  this.getOverrideEnvProxy = function () {
+    return overrideEnvProxy;
   };
 
   /**

diff --git a/lib/constants/error_messages.js b/lib/constants/error_messages.js
@@ -84,7 +84,7 @@ exports[404053] = 'A host must be specified.';
 exports[404054] = 'Invalid host. The specified value must be a string.';
 exports[404055] = 'Invalid passcodeInPassword. The specified value must be a boolean';
 exports[404056] = 'Invalid passcode. The specified value must be a string';
-exports[404057] = 'Invalid overwriteEnvProxy. The specified value must be a boolean';
+exports[404057] = 'Invalid overrideEnvProxy. The specified value must be a boolean';
 
 // 405001
 exports[405001] = 'Invalid callback. The specified value must be a function.';

diff --git a/lib/errors.js b/lib/errors.js
@@ -88,7 +88,7 @@ codes.ERR_CONN_CREATE_MISSING_HOST = 404053;
 codes.ERR_CONN_CREATE_INVALID_HOST = 404054;
 codes.ERR_CONN_CREATE_INVALID_PASSCODE_IN_PASSWORD = 404055;
 codes.ERR_CONN_CREATE_INVALID_PASSCODE = 404056;
-codes.ERR_CONN_CREATE_INVALID_OVERWRITE_ENV_PROXY = 404057;
+codes.ERR_CONN_CREATE_INVALID_OVERRIDE_ENV_PROXY = 404057;
 
 // 405001
 codes.ERR_CONN_CONNECT_INVALID_CALLBACK = 405001;

diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js
@@ -26,6 +26,7 @@ const resultStatus = require('./file_util').resultStatus;
 const HTTPS_PROXY = 'HTTPS_PROXY';
 
 const { Storage } = require('@google-cloud/storage');
+const { isBypassProxy } = require('../http/node');
 
 const EXPIRED_TOKEN = 'ExpiredToken';
 
@@ -53,19 +54,11 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
   const fs = typeof fileStream !== 'undefined' ? fileStream : require('fs');
   const connectionProxy = connectionConfig.getProxy();
 
-  let isOverrideEnvProxyEnabled = false;
+  //Todo Think about Bypass when NOproxy is configured.
   const originalHttpsProxy = process.env[HTTPS_PROXY];
+  let isEnvProxyOverridden = false;
   let proxyString = null;
-  if (Util.exists(connectionProxy) && connectionConfig.getOverrideEnvProxy()) {
-    const compareAndLogEnvAndAgentProxies = ProxyUtil.getCompareAndLogEnvAndAgentProxies(connectionProxy);
-    Logger.getInstance().debug('proxy settings used in GCS Util: %s', compareAndLogEnvAndAgentProxies.messages);
-    if ((compareAndLogEnvAndAgentProxies.warnings).length !== 0) {
-      Logger.getInstance().warn('GCS proxy wanring: %s', compareAndLogEnvAndAgentProxies.warnings);
-      Logger.getInstance().warn(`The ${HTTPS_PROXY} value will be overwritten during the Storage HTTP calls`);
-      proxyString = ProxyUtil.convertProxyToString(connectionProxy);
-      isOverrideEnvProxyEnabled = true;
-    }
-  }
+
   /**
   * Retrieve the GCS token from the stage info metadata.
   *
@@ -101,6 +94,24 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
       client = null;
     }
 
+    const isByPassed = isBypassProxy(connectionProxy, endPoint || `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com`);
+    if (Util.isEmptyObject(connectionProxy) || isByPassed) {
+      //do nothing
+    } else if (!Util.exists(originalHttpsProxy)) {
+      proxyString = ProxyUtil.stringifyProxy(connectionProxy);
+      isEnvProxyOverridden = true;
+    } else if (connectionConfig.getOverrideEnvProxy()) {
+      const compareAndLogEnvAndAgentProxies = ProxyUtil.getCompareAndLogEnvAndAgentProxies(connectionProxy);
+      Logger.getInstance().debug('proxy settings used in GCS Util: %s', compareAndLogEnvAndAgentProxies.messages);
+      if (Util.validateEmptyString(compareAndLogEnvAndAgentProxies.warnings) ) {
+        Logger.getInstance().warn('GCS proxy wanring: %s', compareAndLogEnvAndAgentProxies.warnings);
+        Logger.getInstance().warn(`The environment variable ${HTTPS_PROXY} value will be overwritten during the Storage HTTP calls`);
+        proxyString = ProxyUtil.stringifyProxy(connectionProxy);
+        isEnvProxyOverridden = true;
+      }
+    }
+
+
     if (typeof httpClient === 'undefined') {
       const proxyAgent = getProxyAgent(connectionProxy, new URL(connectionConfig.accessUrl), endPoint || `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` );
       axios = require('axios').create({
@@ -176,7 +187,7 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
             const gcsLocation = this.extractBucketNameAndPath(meta['stageInfo']['location']);
 
             let metadata;
-            if (isOverrideEnvProxyEnabled) {
+            if (isEnvProxyOverridden) {
               process.env[HTTPS_PROXY] = proxyString;
               metadata = await meta['client'].gcsClient
                 .bucket(gcsLocation.bucketName)
@@ -324,7 +335,7 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
     try {
       if (shouldPerformGCPBucket(accessToken)) {
         const gcsLocation = this.extractBucketNameAndPath(meta['stageInfo']['location']);
-        if (isOverrideEnvProxyEnabled){
+        if (isEnvProxyOverridden){
           process.env[HTTPS_PROXY] = proxyString;
           await meta['client'].gcsClient
             .bucket(gcsLocation.bucketName)
@@ -415,7 +426,7 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
       if (shouldPerformGCPBucket(accessToken)) {
         const gcsLocation = this.extractBucketNameAndPath(meta['stageInfo']['location']);
         let metadata;
-        if (isOverrideEnvProxyEnabled){
+        if (isEnvProxyOverridden){
           process.env[HTTPS_PROXY] = proxyString;
           await meta['client'].gcsClient
             .bucket(gcsLocation.bucketName)
@@ -524,6 +535,15 @@ function GCSUtil(connectionConfig, httpClient, fileStream) {
     return link;
   };
 
+  //getProxyString and getIsEnvProxyOverridden are for testing purpose.
+  this.getProxyString = function () {
+    return proxyString;
+  };
+
+  this.getIsEnvProxyOverridden = function () {
+    return isEnvProxyOverridden;
+  };
+
   /**
 * Left strip the specified character from a string.
 *

diff --git a/lib/proxy_util.js b/lib/proxy_util.js
@@ -55,18 +55,21 @@ exports.getCompareAndLogEnvAndAgentProxies = function (agentOptions) {
       ' protocol=' + agentOptions.protocol + ' proxy=' + proxyHostAndPort
       : ' proxy=' + proxyHostAndPort;
     const proxyUsername = agentOptions.user ? ' user=' + agentOptions.user : '';
+    const proxyString = `${Util.exists(proxyUsername) ? `${agentOptions.user}:${agentOptions.password}@` : ''}${proxyHostAndPort}`.toLowerCase();
     logMessages.messages = logMessages.messages + ` // Proxy configured in Agent:${proxyProtocolHostAndPort}${proxyUsername}`;
 
     // check if both the PROXY envvars and Connection proxy config is set
     // generate warnings if they are, and are also different
     if (envProxy.httpProxy &&
-            this.removeScheme(envProxy.httpProxy).toLowerCase() !== this.removeScheme(proxyHostAndPort).toLowerCase()) {
+            this.removeScheme(envProxy.httpProxy).toLowerCase() !== 
+            this.removeScheme(proxyString)) {
       logMessages.warnings = logMessages.warnings + ` Using both the HTTP_PROXY (${envProxy.httpProxy})`
             + ` and the proxyHost:proxyPort (${proxyHostAndPort}) settings to connect, but with different values.`
             + ' If you experience connectivity issues, try unsetting one of them.';
     }
     if (envProxy.httpsProxy &&
-            this.removeScheme(envProxy.httpsProxy).toLowerCase() !== this.removeScheme(proxyHostAndPort).toLowerCase()) {
+            this.removeScheme(envProxy.httpsProxy).toLowerCase() !== 
+            this.removeScheme(proxyString)) {
       logMessages.warnings = logMessages.warnings + ` Using both the HTTPS_PROXY (${envProxy.httpsProxy})`
             + ` and the proxyHost:proxyPort (${proxyHostAndPort}) settings to connect, but with different values.`
             + ' If you experience connectivity issues, try unsetting one of them.';
@@ -261,4 +264,10 @@ exports.describeProxy = function (proxy) {
   return `proxyHost: ${proxy.host}, proxyPort: ${proxy.port}, proxyUser: ${proxy.user}, `
     + `proxyPassword is ${LoggingUtil.describePresence(proxy.password)}, `
     + `proxyProtocol: ${proxy.protocol}, noProxy: ${proxy.noProxy}`;
+};
+
+exports.stringifyProxy = function (proxy) {
+  return `${(proxy.protocol).startsWith('https') ? 'https' : 'http'}://` + 
+  `${Util.exists(proxy.user) ? `${proxy.user}:${proxy.password}@` : ''}` +
+  `${proxy.host}:${proxy.port}`;
 };
diff --git a/lib/util.js b/lib/util.js
@@ -755,6 +755,13 @@ exports.isNotEmptyAsString = function (variable) {
   return exports.exists(variable);
 };
 
+exports.isEmptyObject = (object) => {
+  if (typeof object !== 'object' || object instanceof Array) {
+    return false;
+  }
+  return Object.keys(object).length === 0;
+};
+
 exports.isWindows = function () {
   return os.platform() === 'win32';
 };
diff --git a/test/unit/connection/connection_config_test.js b/test/unit/connection/connection_config_test.js
@@ -776,15 +776,15 @@ describe('ConnectionConfig: basic', function () {
         errorCode: ErrorCodes.ERR_CONN_CREATE_INVALID_PASSCODE
       },
       {
-        name: 'invalid overwriteEnvProxy',
+        name: 'invalid overrideEnvProxy',
 
         options: {
           account: 'account',
           username: 'username',
           password: 'password',
-          overwriteEnvProxy: 123456
+          overrideEnvProxy: 123456
         },
-        errorCode: ErrorCodes.ERR_CONN_CREATE_INVALID_OVERWRITE_ENV_PROXY
+        errorCode: ErrorCodes.ERR_CONN_CREATE_INVALID_OVERRIDE_ENV_PROXY
       },
     ];
 
@@ -1678,13 +1678,13 @@ describe('ConnectionConfig: basic', function () {
         getter: 'getPasscode',
       },
       {
-        name: 'overwriteEnvProxy',
+        name: 'overrideEnvProxy',
         input: {
           ...mandatoryOption,
-          overwriteEnvProxy: false,
+          overrideEnvProxy: false,
         },
         result: false,
-        getter: 'getOverwriteEnvProxy',
+        getter: 'getOverrideEnvProxy',
       },
     ];