AWS-SDK Migration #2482

benzekrimaha · 2025-08-12T07:57:53Z

Please note that a ticket has been creaed to add coverage not to make this PR more extended : https://scality.atlassian.net/browse/ARSN-524

bert-e · 2025-08-12T07:57:57Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-09-18T09:53:02Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

SylvainSenechal · 2025-09-19T13:48:40Z

Issue: ARSN-514

Please note that a ticket has been creaed to add coverage not to make this PR more extended : https://scality.atlassian.net/browse/ARSN-524

Wait, you say you've got two Jira ticket 514/524, but they both point to this same PR 🤔

lib/network/kmsAWS/Client.ts

lib/network/utils.ts

lib/network/kmsAWS/Client.ts

lib/storage/data/external/AwsClient.js

tests/unit/storage/data/locConstraintParser.spec.js

tests/unit/storage/data/DummyService.js

SylvainSenechal · 2025-09-22T10:14:51Z

tests/functional/kmsAWS/highlevel.spec.js

+        });
    });

    it('should create a new master encryption key', done => {


Looks like a duplicate of the first test "should support default encryption key per account" 🤔

Up, still here

package.json

lib/storage/data/external/GCP/GcpService.js

ghost

This completes Sylvain's review. I think I covered everything that makes sense given what should probably change, so I'll come back once it's adressed

ghost · 2025-09-23T09:37:40Z

lib/storage/data/external/GcpClient.js

+    listObjects(params, callback) {
+        return this.send(new ListObjectsCommand(params))
+            .then(data => callback && callback(null, data))
+            .catch(err => callback && callback(err));
+    }


It could make sense to switch this file to ts, but the callback should be required?

If no you can do that

Suggested change

listObjects(params, callback) {

return this.send(new ListObjectsCommand(params))

.then(data => callback && callback(null, data))

.catch(err => callback && callback(err));

}

listObjects(params, callback) {

return this.send(new ListObjectsCommand(params))

.then(data => callback?.(null, data))

.catch(err => callback?.(err));

}

But better to always call it?

lib/storage/data/external/AwsClient.js

lib/storage/data/external/GCP/GcpService.js

lib/network/kmsAWS/Client.ts

lib/storage/data/LocationConstraintParser.js

lib/network/kmsAWS/Client.ts

lib/storage/data/external/GCP/GcpService.js

lib/network/utils.ts

francoisferrand

Added a few comments about GCP, I think we are adding lots of dead code: should be cleaned up (but maybe in a followup)

GcpClient class in GcpService.js should be renamed to avoid confusion/duplicated type name → #2482 (comment)
It seems the functions in GcpApis are not used (they are 'added' in GcpService's class, but not called... some leftover before code was moved "inside" that class maybe, long before this PR ?) → #2482 (comment)
the sdk v2 functions added in GcpClient.js seem weird : if we need to keep the old syntax, should be in AwsClient (esp. since the code there is not Gcp-specific) → #2482 (comment) and #2482 (comment)
most sdk v2 functions added in GcpService.js seem not to be called → #2482 (comment) and #2482 (comment)
code for MPU looks fishy: in GcpClient.js:121 we call this._client.createMultipartUpload but this call was not migrated to new SDK nor implemented in GcpService.js.... and thus I think MPU are not tested with GCP
DummyService.js should get cleaned' up → #2482 (comment)

francoisferrand · 2026-01-07T16:15:39Z

lib/storage/data/external/GCP/GcpService.js

+ *   - MPU (multipart upload) is emulated using GCP's compose/copy APIs.
+ *   - All S3-compatible methods are available, with overrides for GCP quirks.
+ */
+class GcpClient extends S3Client {


should be rename GcpService or GCP to avoid having 2 times the same type, which can only create confusion

francoisferrand · 2026-01-07T16:33:27Z

lib/storage/data/external/GcpClient.js

+    listObjects(params, callback) {
+        return this.send(new ListObjectsCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }
+
+    putObject(params, callback) {
+        return this.send(new PutObjectCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }
+
+    getObject(params, callback) {
+        return this.send(new GetObjectCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }
+
+    deleteObject(params, callback) {
+        return this.send(new DeleteObjectCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }
+
+    copyObject(params, callback) {
+        return this.send(new CopyObjectCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }
+
+    headObject(params, callback) {
+        return this.send(new HeadObjectCommand(params))
+            .then(data => callback?.(null, data))
+            .catch(err => callback?.(err));
+    }


if these functions are used/needed (i.e. part of the abstraction), they should be added to AwsClient ;
if not, they should not be added (at all)

francoisferrand · 2026-01-07T16:35:34Z

lib/storage/data/external/GCP/GcpService.js

+     * @param {object} params - S3 upload params
+     * @param {function} callback
+     */
+    upload(params, callback) {


all (or most of) these new functions are not used : this class is used as _client in GcpClient (.js), where only the following API are called:
headBucket, creatMultipoartUpdload, completeMultipartUpload, uploadPart, uploadPartCopy and abortMultipartUpload (in addition to the send method inherited from S3Client)

actually it is matching: the functions renamed when they are exported, in index.js...

As we can no longer use the Service.defineService previously used in aws-sdk v2, we have to manually this commit aims to implement the GCP client using the new aws-sdk v3 architecture and migrate the existing code to use it. Please note that now both the setup and signing mechanisms had to be re-implemented in the GcpService file. Issue: ARSN-514

Issue: ARSN-514

In this commit several changes were done as aws-sdk v3 happens to be less permissive than v2 in terms of error handling and stream management. To ensure backward compatibility with existing functionalities adjustments were made, please refer to code comments for more details. Issue: ARSN-514

As the sdk now returns name instead of code for errors, we need to adapt the error handling accordingly. Issue: ARSN-514

Issue: ARSN-514

A security issue was fixed using resolutions in package.json as the vulnerability is in a transient dependency. Issue: ARSN-514

ISSUE: ARSN-546

francoisferrand · 2026-01-07T18:15:11Z

/bypass_author_approval

ISSUE: ARSN-546

benzekrimaha force-pushed the improvement/ARSN-514 branch from ad7e01e to 2f3ed00 Compare September 18, 2025 09:52

benzekrimaha changed the base branch from development/8.2 to development/8.3 September 18, 2025 09:52

benzekrimaha force-pushed the improvement/ARSN-514 branch 2 times, most recently from f8e5940 to 942aa25 Compare September 18, 2025 13:02

benzekrimaha marked this pull request as ready for review September 18, 2025 13:02

benzekrimaha changed the title ~~Improvement/arsn 514~~ AWS-SDK Migration Sep 18, 2025

scality deleted a comment from bert-e Sep 18, 2025

benzekrimaha force-pushed the improvement/ARSN-514 branch from 942aa25 to f9f96ce Compare September 18, 2025 13:17

scality deleted a comment from codecov bot Sep 18, 2025