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

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

lib/storage/data/LocationConstraintParser.js

+                maxAttempts: 1,
+                requestHandler: undefined, // v3 handles agents differently
+                // v3 does not use signatureVersion or sslEnabled directly
+                // v3 does not use customUserAgent directly


Can we maybe point to where the logic now is? In the sdk, or elsewhere in the code, so we can know where to look later?

ghost · 2025-09-23T09:59:21Z

lib/network/kmsAWS/Client.ts

        const params = {
            KeyId: masterKeyId,
-            KeySpec: 'AES_256',
+            KeySpec: "AES_256" as const,


Suggested change

KeySpec: "AES_256" as const,

KeySpec: 'AES_256' as const,

Or you can also create the variable as a const once and reuse it here

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

lib/network/utils.ts

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

lib/storage/data/external/AwsClient.js

SylvainSenechal · 2025-12-29T16:13:25Z

About the stream :

Do we confirm we want to keep _normalizeSdkStream even though we probably don't need to handle the "fromWeb" part, and the body is likely always readable ?
the stream has 3 methods : abort/destroy/createReadStream. destroy is used for sure, but I feel like abort is useless ? And createReadStream, I thought it was use in the datawrapper.js file, but I don't see it anymore, is it used somewhere else ?
The part about rawBody !== stream is a bit confusing, and I think it wouldn't be necessary if we assumed that the body was always Readable
nit: Is the boolean "finished" usefull, I think destroying stream is idempotent

francoisferrand · 2025-12-30T14:27:16Z

lib/storage/data/external/AwsClient.js

                    logLevel = 'info';
                    retError = errors.LocationNotFound;
-                } else {
+                }


nit: incorrect line wrap

francoisferrand · 2025-12-30T14:28:49Z

lib/storage/data/external/AwsClient.js

+            .then(data => {
+                const rawBody = data.Body;
+                const stream = this._normalizeSdkStream(rawBody);
+                if (!stream || typeof stream.on !== 'function') {


no reason to check that stream.on is defined : this is the whole purpose of _normalizeSdkStream, and it should return null if it does not know how to create a stream from the input...

francoisferrand · 2025-12-30T14:33:30Z

lib/storage/data/external/AwsClient.js

+                stream.abort = abortRequest;
+                stream.destroy = err => abortRequest(err);
+                stream.createReadStream = () => stream;
+


somehow here we are recreating the whole stream : instead of modifying the "normalized" object returned by AWS SDK, should we not just create the appropriate "stream" (as we need for our use-case) which will return AWS' stream in its createReadStream method?

Suggested change

stream.abort = abortRequest;

stream.destroy = err => abortRequest(err);

stream.createReadStream = () => stream;

const stream = {

abort: abortRequest,

destroy: abortRequest,

createReadStream: () => data.Body,

};

francoisferrand · 2025-12-30T14:35:11Z

lib/storage/data/external/AwsClient.js

+                        rawBody.destroy?.(err);
+                    }
+                    try {
+                        originalDestroy(err);


should destroy be called on abort()? should it not instead be called only on destroy() ?
(not sure what the semantics of JS streams actually are)

francoisferrand · 2025-12-30T14:37:48Z

lib/storage/data/external/GcpClient.js

        });
    }
+
+    listObjects(params, callback) {


why do we need these extra methods?
the required methods should be already implemented in AwsClient...

francoisferrand · 2025-12-30T14:41:46Z

tests/unit/s3routes/routesUtils/responseStreamData.spec.js

+                }, 1000),
+                isclosed: true,
+            }, undefined, logger.newRequestLogger());
+        } catch (err) {


error should be handled through some callback : if we catch an exception here, we may have the bug in production code as well....
--> find where the exception is generated
--> handle it properly, sending the response through appropriate callback (or tweak tests/mocks to properly receive it, and fail the test with done(err) accordingly)
--> remove the try/catch and useless once

francoisferrand · 2025-12-30T14:52:56Z

tests/unit/storage/data/DummyService.js

+        this.putObjectAsync = promisify(this.putObject.bind(this));
+        this.copyObjectAsync = promisify(this.copyObject.bind(this));
+        this.headObjectAsync = promisify(this.headObject.bind(this));
+        this.putObjectTaggingAsync = promisify(this.putObjectTagging.bind(this));
+        this.deleteObjectTaggingAsync = promisify(this.deleteObjectTagging.bind(this));
+        this.completeMultipartUploadAsync = promisify(this.completeMultipartUpload.bind(this));
+        this.deleteObjectAsync = promisify(this.deleteObject.bind(this));
+        this.createMultipartUploadAsync = promisify(this.createMultipartUpload.bind(this));
+        this.uploadPartAsync = promisify(this.uploadPart.bind(this));
+        this.listPartsAsync = promisify(this.listParts.bind(this));
+        this.abortMultipartUploadAsync = promisify(this.abortMultipartUpload.bind(this));
+


AFAIU, these methods "emulate" the AWS sdk : since we now use them through send() (in AWSClient, etc...), no point in promisify (esp. since every method was moved, so the whole file is changed) and best to async' every function

francoisferrand · 2025-12-30T14:55:55Z

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

+                if (metaVersionId) {
+                    result.MetaVersionId = metaVersionId;
+                }
+                return callback && callback(null, result);


callback must be set, this is the semantic of this function...
(same later)

francoisferrand · 2025-12-30T15:00:19Z

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

-    },
+    }
+
+    headBucket(params, callback) {


not really needed? this is used only in GcpClient.healthcheck

healthcheck(location, callback) { const checkBucketHealth = (bucket, cb) => { let bucketResp; this._client.headBucket({ Bucket: bucket }, err => {

need to check other of these "new" API here, and add them only if really needed

francoisferrand · 2025-12-30T15:03:23Z

lib/storage/data/external/GCP/GcpApis/putObject.js

+
+const { getPutTagsMetadata, attachHeaderCaptureMiddleware } = require('../GcpUtils');

 function putObject(params, callback) {


where are these functions used? directly in cloudserver?

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