From 4e68026a570d9150ee2b821d443ec4169a1b8f10 Mon Sep 17 00:00:00 2001 From: John Yun Date: Thu, 28 Nov 2024 09:24:22 -0800 Subject: [PATCH 1/8] applied useReginoal URL condition --- lib/file_transfer_agent/gcs_util.js | 3 ++- lib/file_transfer_agent/s3_util.js | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js index 2a9cf251d..11b13c931 100644 --- a/lib/file_transfer_agent/gcs_util.js +++ b/lib/file_transfer_agent/gcs_util.js @@ -69,7 +69,8 @@ function GCSUtil(httpclient, filestream) { } }); - const storage = new Storage({ interceptors_: interceptors }); + const useRegionalUrl = stageInfo.region === 'me-central2' || stageInfo.useRegionalUrl; + const storage = useRegionalUrl ? new Storage({ interceptors_: interceptors, apiEndpoint: `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` }) : new Storage({ interceptors_: interceptors }); client = { gcsToken: gcsToken, gcsClient: storage }; } else { diff --git a/lib/file_transfer_agent/s3_util.js b/lib/file_transfer_agent/s3_util.js index 088d780b2..6d41d9f2d 100644 --- a/lib/file_transfer_agent/s3_util.js +++ b/lib/file_transfer_agent/s3_util.js @@ -55,10 +55,20 @@ function S3Util(connectionConfig, s3, filestream) { const stageCredentials = stageInfo['creds']; const securityToken = stageCredentials['AWS_TOKEN']; // if GS sends us an endpoint, it's likely for FIPS. Use it. + + // const useS3RegionalUrl = stageInfo.useRegionalUrl ||stageInfo.useS3RegionalUrl; + const useS3RegionalUrl = true; + let endPoint = null; if (stageInfo['endPoint']) { - endPoint = 'https://' + stageInfo['endPoint']; + endPoint = `https://${stageInfo['endPoint']}`; + } else { + if (stageInfo.region && useS3RegionalUrl) { + const domainSuffixForRegionalUrl = (stageInfo.region).toLowerCase().startsWith('cn-') ? 'amazonaws.com.cn' : 'amazonaws.com'; + endPoint = `https://s3.${stageInfo.region}.${domainSuffixForRegionalUrl}`; + } } + const config = { apiVersion: '2006-03-01', @@ -80,7 +90,7 @@ function S3Util(connectionConfig, s3, filestream) { } } if (proxy) { - const proxyAgent = getProxyAgent(proxy, new URL(connectionConfig.accessUrl), SNOWFLAKE_S3_DESTINATION); + const proxyAgent = getProxyAgent(proxy, new URL(connectionConfig.accessUrl), endPoint || SNOWFLAKE_S3_DESTINATION); config.requestHandler = new NodeHttpHandler({ httpAgent: proxyAgent, httpsAgent: proxyAgent From 6af0581be22a9c24cb415ff9cc2dc3371795293f Mon Sep 17 00:00:00 2001 From: John Yun Date: Thu, 28 Nov 2024 14:54:11 -0800 Subject: [PATCH 2/8] add testing cases --- lib/file_transfer_agent/s3_util.js | 6 +-- test/unit/file_transfer_agent/gcs_test.js | 33 +++++++++++++ test/unit/file_transfer_agent/s3_test.js | 57 ++++++++++++++++++++++- 3 files changed, 91 insertions(+), 5 deletions(-) diff --git a/lib/file_transfer_agent/s3_util.js b/lib/file_transfer_agent/s3_util.js index 6d41d9f2d..f92901095 100644 --- a/lib/file_transfer_agent/s3_util.js +++ b/lib/file_transfer_agent/s3_util.js @@ -54,11 +54,9 @@ function S3Util(connectionConfig, s3, filestream) { this.createClient = function (stageInfo, useAccelerateEndpoint) { const stageCredentials = stageInfo['creds']; const securityToken = stageCredentials['AWS_TOKEN']; - // if GS sends us an endpoint, it's likely for FIPS. Use it. + const useS3RegionalUrl = stageInfo.useRegionalUrl || stageInfo.useS3RegionalUrl; - // const useS3RegionalUrl = stageInfo.useRegionalUrl ||stageInfo.useS3RegionalUrl; - const useS3RegionalUrl = true; - + // if GS sends us an endpoint, it's likely for FIPS. Use it. let endPoint = null; if (stageInfo['endPoint']) { endPoint = `https://${stageInfo['endPoint']}`; diff --git a/test/unit/file_transfer_agent/gcs_test.js b/test/unit/file_transfer_agent/gcs_test.js index bd2de06e9..ad9a86d5e 100644 --- a/test/unit/file_transfer_agent/gcs_test.js +++ b/test/unit/file_transfer_agent/gcs_test.js @@ -64,6 +64,39 @@ describe('GCS client', function () { GCS = new SnowflakeGCSUtil(httpclient, filestream); }); + describe('AWS client endpoint testing', async function () { + const originalStageInfo = meta.stageInfo; + const testCases = [ + { + name: 'when useRegionalURL is only enabled', + stageInfo: { + ...originalStageInfo, + useRegionalURL: true, + }, + endPoint: null, + result: 'storage.mocklocation.rep.googleapis.com' + }, + { + name: 'when region is me-central2', + stageInfo: { + ...originalStageInfo, + useRegionalURL: false, + endPoint: null, + region: 'me-central2' + }, + result: 'storage.me-central2.rep.googleapis.com`' + }, + ]; + + testCases.forEach(({ name, stageInfo, result }) => { + it(name, () => { + const client = GCS.createClient(stageInfo); + assert.strictEqual(client.gcsClient.apiEndPoint, result); + } ); + + }); + }); + it('extract bucket name and path', async function () { const GCS = new SnowflakeGCSUtil(); diff --git a/test/unit/file_transfer_agent/s3_test.js b/test/unit/file_transfer_agent/s3_test.js index 244a84fbe..4a97ec0e5 100644 --- a/test/unit/file_transfer_agent/s3_test.js +++ b/test/unit/file_transfer_agent/s3_test.js @@ -44,7 +44,7 @@ describe('S3 client', function () { before(function () { mock('s3', { - S3: function () { + S3: function (config) { function S3() { this.getObject = function () { function getObject() { @@ -57,6 +57,8 @@ describe('S3 client', function () { return new getObject; }; + + this.config = config; this.putObject = function () { function putObject() { this.then = function (callback) { @@ -82,6 +84,59 @@ describe('S3 client', function () { AWS = new SnowflakeS3Util(noProxyConnectionConfig, s3, filesystem); }); + describe('AWS client endpoint testing', async function () { + const originalStageInfo = meta.stageInfo; + const testCases = [ + { + name: 'when useS3RegionalURL is only enabled', + stageInfo: { + ...originalStageInfo, + useS3RegionalUrl: true, + }, + endPoint: null, + result: null + }, + { + name: 'when useS3RegionalURL and is enabled and domain starts with cn', + stageInfo: { + ...originalStageInfo, + useS3RegionalUrl: true, + endPoint: null, + region: 'cn-mockLocation' + }, + result: 'https://s3.cn-mockLocation.amazonaws.com.cn' + }, + { + name: 'when endPoint is enabled', + stageInfo: { + ...originalStageInfo, + endPoint: 's3.endpoint', + useS3RegionalUrl: false + }, + result: 'https://s3.endpoint' + }, + { + name: 'when both endPoint and useS3PReiongalUrl is valid', + stageInfo: { + ...originalStageInfo, + endPoint: 's3.endpoint', + useS3RegionalUrl: true, + + }, + result: 'https://s3.endpoint' + }, + ]; + + testCases.forEach(({ name, stageInfo, result }) => { + it(name, () => { + const client = AWS.createClient(stageInfo); + assert.strictEqual(client.config.endpoint, result); + } ); + + }); + }); + + it('extract bucket name and path', async function () { let result = extractBucketNameAndPath('sfc-eng-regression/test_sub_dir/'); assert.strictEqual(result.bucketName, 'sfc-eng-regression'); From dac57d1929a0f62b6fefc3038b6a77a8ff4a28ae Mon Sep 17 00:00:00 2001 From: John Yun Date: Thu, 28 Nov 2024 15:16:34 -0800 Subject: [PATCH 3/8] fix --- test/unit/file_transfer_agent/gcs_test.js | 20 ++++++++------------ test/unit/file_transfer_agent/s3_test.js | 2 +- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/test/unit/file_transfer_agent/gcs_test.js b/test/unit/file_transfer_agent/gcs_test.js index ad9a86d5e..6c1503eae 100644 --- a/test/unit/file_transfer_agent/gcs_test.js +++ b/test/unit/file_transfer_agent/gcs_test.js @@ -64,34 +64,30 @@ describe('GCS client', function () { GCS = new SnowflakeGCSUtil(httpclient, filestream); }); - describe('AWS client endpoint testing', async function () { - const originalStageInfo = meta.stageInfo; + describe('GCS client endpoint testing', async function () { const testCases = [ { name: 'when useRegionalURL is only enabled', stageInfo: { - ...originalStageInfo, - useRegionalURL: true, + useRegionalUrl: true, + region: 'mockLocation', }, - endPoint: null, - result: 'storage.mocklocation.rep.googleapis.com' + result: 'https://storage.mocklocation.rep.googleapis.com' }, { name: 'when region is me-central2', stageInfo: { - ...originalStageInfo, - useRegionalURL: false, - endPoint: null, + useRegionalUrl: false, region: 'me-central2' }, - result: 'storage.me-central2.rep.googleapis.com`' + result: 'https://storage.me-central2.rep.googleapis.com' }, ]; testCases.forEach(({ name, stageInfo, result }) => { it(name, () => { - const client = GCS.createClient(stageInfo); - assert.strictEqual(client.gcsClient.apiEndPoint, result); + const client = GCS.createClient({ ...stageInfo, ...meta.stageInfo, creds:{ GCS_ACCESS_TOKEN: "mockToken"} }); + assert.strictEqual(client.gcsClient.apiEndpoint, result); } ); }); diff --git a/test/unit/file_transfer_agent/s3_test.js b/test/unit/file_transfer_agent/s3_test.js index 4a97ec0e5..fc66e63d4 100644 --- a/test/unit/file_transfer_agent/s3_test.js +++ b/test/unit/file_transfer_agent/s3_test.js @@ -92,8 +92,8 @@ describe('S3 client', function () { stageInfo: { ...originalStageInfo, useS3RegionalUrl: true, - }, endPoint: null, + }, result: null }, { From b3e540155ea6e320cce35920e480d955088bece7 Mon Sep 17 00:00:00 2001 From: John Yun Date: Thu, 28 Nov 2024 15:18:30 -0800 Subject: [PATCH 4/8] lint fix --- test/unit/file_transfer_agent/gcs_test.js | 2 +- test/unit/file_transfer_agent/s3_test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/file_transfer_agent/gcs_test.js b/test/unit/file_transfer_agent/gcs_test.js index 6c1503eae..7b2fa43a0 100644 --- a/test/unit/file_transfer_agent/gcs_test.js +++ b/test/unit/file_transfer_agent/gcs_test.js @@ -86,7 +86,7 @@ describe('GCS client', function () { testCases.forEach(({ name, stageInfo, result }) => { it(name, () => { - const client = GCS.createClient({ ...stageInfo, ...meta.stageInfo, creds:{ GCS_ACCESS_TOKEN: "mockToken"} }); + const client = GCS.createClient({ ...stageInfo, ...meta.stageInfo, creds: { GCS_ACCESS_TOKEN: 'mockToken' } }); assert.strictEqual(client.gcsClient.apiEndpoint, result); } ); diff --git a/test/unit/file_transfer_agent/s3_test.js b/test/unit/file_transfer_agent/s3_test.js index fc66e63d4..64bf0bf07 100644 --- a/test/unit/file_transfer_agent/s3_test.js +++ b/test/unit/file_transfer_agent/s3_test.js @@ -92,7 +92,7 @@ describe('S3 client', function () { stageInfo: { ...originalStageInfo, useS3RegionalUrl: true, - endPoint: null, + endPoint: null, }, result: null }, From ffab583578f71e38d7ec174e3cd4382f34f50f8e Mon Sep 17 00:00:00 2001 From: John Yun Date: Tue, 3 Dec 2024 13:11:06 -0800 Subject: [PATCH 5/8] update the PR based on the comments --- lib/file_transfer_agent/gcs_util.js | 3 ++- lib/file_transfer_agent/s3_util.js | 7 +++---- test/unit/file_transfer_agent/gcs_test.js | 8 ++++++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js index 11b13c931..a719d1a1d 100644 --- a/lib/file_transfer_agent/gcs_util.js +++ b/lib/file_transfer_agent/gcs_util.js @@ -69,7 +69,8 @@ function GCSUtil(httpclient, filestream) { } }); - const useRegionalUrl = stageInfo.region === 'me-central2' || stageInfo.useRegionalUrl; + //TODO: hardcoded region will be replaced in the future + const useRegionalUrl = (stageInfo.region).toLowerCase() === 'me-central2' || stageInfo.useRegionalUrl; const storage = useRegionalUrl ? new Storage({ interceptors_: interceptors, apiEndpoint: `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` }) : new Storage({ interceptors_: interceptors }); client = { gcsToken: gcsToken, gcsClient: storage }; diff --git a/lib/file_transfer_agent/s3_util.js b/lib/file_transfer_agent/s3_util.js index f92901095..de3dfbc67 100644 --- a/lib/file_transfer_agent/s3_util.js +++ b/lib/file_transfer_agent/s3_util.js @@ -54,20 +54,19 @@ function S3Util(connectionConfig, s3, filestream) { this.createClient = function (stageInfo, useAccelerateEndpoint) { const stageCredentials = stageInfo['creds']; const securityToken = stageCredentials['AWS_TOKEN']; - const useS3RegionalUrl = stageInfo.useRegionalUrl || stageInfo.useS3RegionalUrl; + const isRegionalUrlEnabled = stageInfo.useRegionalUrl || stageInfo.useS3RegionalUrl; // if GS sends us an endpoint, it's likely for FIPS. Use it. let endPoint = null; if (stageInfo['endPoint']) { endPoint = `https://${stageInfo['endPoint']}`; } else { - if (stageInfo.region && useS3RegionalUrl) { + if (stageInfo.region && isRegionalUrlEnabled) { const domainSuffixForRegionalUrl = (stageInfo.region).toLowerCase().startsWith('cn-') ? 'amazonaws.com.cn' : 'amazonaws.com'; endPoint = `https://s3.${stageInfo.region}.${domainSuffixForRegionalUrl}`; } } - - + const config = { apiVersion: '2006-03-01', region: stageInfo['region'], diff --git a/test/unit/file_transfer_agent/gcs_test.js b/test/unit/file_transfer_agent/gcs_test.js index 7b2fa43a0..2219e81d8 100644 --- a/test/unit/file_transfer_agent/gcs_test.js +++ b/test/unit/file_transfer_agent/gcs_test.js @@ -82,6 +82,14 @@ describe('GCS client', function () { }, result: 'https://storage.me-central2.rep.googleapis.com' }, + { + name: 'when region is me-central2', + stageInfo: { + useRegionalUrl: false, + region: 'ME-cEntRal2' + }, + result: 'https://storage.me-central2.rep.googleapis.com' + }, ]; testCases.forEach(({ name, stageInfo, result }) => { From 1e9c6f09d8a5c46f45a225efc54f5ed18c43d2ff Mon Sep 17 00:00:00 2001 From: John Yun Date: Tue, 3 Dec 2024 13:14:49 -0800 Subject: [PATCH 6/8] changed the variable name in gcs --- lib/file_transfer_agent/gcs_util.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js index a719d1a1d..19117c0a8 100644 --- a/lib/file_transfer_agent/gcs_util.js +++ b/lib/file_transfer_agent/gcs_util.js @@ -70,8 +70,8 @@ function GCSUtil(httpclient, filestream) { }); //TODO: hardcoded region will be replaced in the future - const useRegionalUrl = (stageInfo.region).toLowerCase() === 'me-central2' || stageInfo.useRegionalUrl; - const storage = useRegionalUrl ? new Storage({ interceptors_: interceptors, apiEndpoint: `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` }) : new Storage({ interceptors_: interceptors }); + const isRegionalUrlEnabled = (stageInfo.region).toLowerCase() === 'me-central2' || stageInfo.useRegionalUrl; + const storage = isRegionalUrlEnabled ? new Storage({ interceptors_: interceptors, apiEndpoint: `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` }) : new Storage({ interceptors_: interceptors }); client = { gcsToken: gcsToken, gcsClient: storage }; } else { From 753d088a9e02dd6f26dc970d846a41bc332fa545 Mon Sep 17 00:00:00 2001 From: John Yun Date: Wed, 4 Dec 2024 10:48:16 -0800 Subject: [PATCH 7/8] add the case for the explict endpoint and testing case for this --- lib/file_transfer_agent/gcs_util.js | 11 ++++-- test/unit/file_transfer_agent/gcs_test.js | 45 +++++++++++++++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js index 19117c0a8..9db441ef3 100644 --- a/lib/file_transfer_agent/gcs_util.js +++ b/lib/file_transfer_agent/gcs_util.js @@ -69,10 +69,15 @@ function GCSUtil(httpclient, filestream) { } }); - //TODO: hardcoded region will be replaced in the future + //TODO: SNOW-1789759 hardcoded region will be replaced in the future const isRegionalUrlEnabled = (stageInfo.region).toLowerCase() === 'me-central2' || stageInfo.useRegionalUrl; - const storage = isRegionalUrlEnabled ? new Storage({ interceptors_: interceptors, apiEndpoint: `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com` }) : new Storage({ interceptors_: interceptors }); - + let endPoint = null; + if (stageInfo['endPoint']) { + endPoint = stageInfo[endPoint]; + } else if (isRegionalUrlEnabled) { + endPoint = `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com`; + } + const storage = endPoint ? new Storage({ interceptors_: interceptors, apiEndpoint: endPoint }) : new Storage({ interceptors_: interceptors }); client = { gcsToken: gcsToken, gcsClient: storage }; } else { client = null; diff --git a/test/unit/file_transfer_agent/gcs_test.js b/test/unit/file_transfer_agent/gcs_test.js index 2219e81d8..6ce0c84d9 100644 --- a/test/unit/file_transfer_agent/gcs_test.js +++ b/test/unit/file_transfer_agent/gcs_test.js @@ -67,29 +67,68 @@ describe('GCS client', function () { describe('GCS client endpoint testing', async function () { const testCases = [ { - name: 'when useRegionalURL is only enabled', + name: 'when the useRegionalURL is only enabled', stageInfo: { + endPoint: null, useRegionalUrl: true, region: 'mockLocation', }, result: 'https://storage.mocklocation.rep.googleapis.com' }, { - name: 'when region is me-central2', + name: 'when the region is me-central2', stageInfo: { + endPoint: null, useRegionalUrl: false, region: 'me-central2' }, result: 'https://storage.me-central2.rep.googleapis.com' }, { - name: 'when region is me-central2', + name: 'when the region is me-central2 (mixed case)', stageInfo: { + endPoint: null, useRegionalUrl: false, region: 'ME-cEntRal2' }, result: 'https://storage.me-central2.rep.googleapis.com' }, + { + name: 'when the region is me-central2 (uppercase)', + stageInfo: { + endPoint: null, + useRegionalUrl: false, + region: 'ME-CENTRAL2' + }, + result: 'https://storage.me-central2.rep.googleapis.com' + }, + { + name: 'when the endPoint is specified', + stageInfo: { + endPoint: 'https://storage.specialEndPoint.rep.googleapis.com', + useRegionalUrl: false, + region: 'ME-cEntRal1' + }, + result: 'https://storage.specialEndPoint.rep.googleapis.com' + }, + { + name: 'when both the endPoint and the useRegionalUrl are specified', + stageInfo: { + endPoint: 'https://storage.specialEndPoint.rep.googleapis.com', + useRegionalUrl: true, + region: 'ME-cEntRal1' + }, + result: 'https://storage.specialEndPoint.rep.googleapis.com' + }, + { + name: 'when both the endPoint is specified and the region is me-central2', + stageInfo: { + endPoint: 'https://storage.specialEndPoint.rep.googleapis.com', + useRegionalUrl: true, + region: 'ME-CENTRAL2' + }, + result: 'https://storage.specialEndPoint.rep.googleapis.com' + }, ]; testCases.forEach(({ name, stageInfo, result }) => { From c5838f7281b6df6f5a03ab57f14b2740e021fed6 Mon Sep 17 00:00:00 2001 From: John Yun Date: Wed, 4 Dec 2024 11:26:20 -0800 Subject: [PATCH 8/8] fix --- lib/file_transfer_agent/gcs_util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/file_transfer_agent/gcs_util.js b/lib/file_transfer_agent/gcs_util.js index 9db441ef3..fc46557ac 100644 --- a/lib/file_transfer_agent/gcs_util.js +++ b/lib/file_transfer_agent/gcs_util.js @@ -73,7 +73,7 @@ function GCSUtil(httpclient, filestream) { const isRegionalUrlEnabled = (stageInfo.region).toLowerCase() === 'me-central2' || stageInfo.useRegionalUrl; let endPoint = null; if (stageInfo['endPoint']) { - endPoint = stageInfo[endPoint]; + endPoint = stageInfo['endPoint']; } else if (isRegionalUrlEnabled) { endPoint = `storage.${stageInfo.region.toLowerCase()}.rep.googleapis.com`; }