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

codecov · 2025-09-18T13:24:31Z

Codecov Report

❌ Patch coverage is 46.57210% with 226 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.54%. Comparing base (8885266) to head (ccbf1cb).
⚠️ Report is 3 commits behind head on development/8.3.

Files with missing lines	Patch %	Lines
lib/storage/data/external/GCP/GcpService.js	43.53%	131 Missing ⚠️
lib/storage/data/external/AwsClient.js	30.20%	67 Missing ⚠️
lib/storage/data/external/GcpClient.js	4.00%	24 Missing ⚠️
lib/network/kmsAWS/Client.ts	93.61%	2 Missing and 1 partial ⚠️
lib/storage/data/LocationConstraintParser.js	91.66%	1 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           development/8.3    #2482      +/-   ##
===================================================
- Coverage            71.35%   70.54%   -0.82%     
===================================================
  Files                  221      219       -2     
  Lines                17817    17919     +102     
  Branches              3712     3724      +12     
===================================================
- Hits                 12714    12641      -73     
- Misses                5099     5273     +174     
- Partials                 4        5       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

SylvainSenechal · 2025-09-19T16:01:24Z

lib/storage/data/external/AwsClient.js

        if (keyContext.isDeleteMarker) {
-            return this._client.deleteObject(params, putCb);
+            return this._client.send(new DeleteObjectCommand(params)).then(res => putCb(null, res))
+            .catch(err => putCb(err));


Suggested change

.catch(err => putCb(err));

.catch(putCb);

Can double check other occurrences in this file & elsewhere

SylvainSenechal · 2025-09-19T16:13:52Z

lib/storage/data/external/AwsClient.js

+            };
+            return callback(null, response);
+        }).catch(err => {
+            if (err.code === 'NoSuchKey' || err.code === 'NotFound') {


I don't wanna leave the same comment on all occurence of this err.code vs err.name situation, but make sure this is ok. Because in the Zenko migration, I just checked and when I used getObjectCommand, i checked the err with err.name : https://github.com/scality/Zenko/blob/improvement/ZENKO-5054/tests/zenko_tests/node_tests/cloudserver/keyFormatVersion/tests/versionedBucket.js#L232

That's a limitation of the lack of TS support. Maybe we can have some quick wins here. ANyway there is two kind of errors today: errors coming from drivers/sdk that are usually native JS errors objects or enriched like in aws sdk. And there are the Arsenal errors that are not very standard, where the code would typically have the error string...
Maybe we can type partially at first this file, so we ensure we are working with the right types of errors, and don't miss anything?

@benzekrimaha double checking after your changes :
I see you used xxxException, but I can't import them, instead I can import xxx without the 'Exception' as you can see for example in the screenshot : NoSuchKey exists, but NoSuchKeyException doesn't exists.
Not too sure if it's me who has a messed up IDE or something, but worth double checking

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

williamlardier

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

williamlardier · 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

williamlardier · 2025-09-23T09:42:29Z

lib/storage/data/external/AwsClient.js

+            };
+            return callback(null, response);
+        }).catch(err => {
+            if (err.code === 'NoSuchKey' || err.code === 'NotFound') {


That's a limitation of the lack of TS support. Maybe we can have some quick wins here. ANyway there is two kind of errors today: errors coming from drivers/sdk that are usually native JS errors objects or enriched like in aws sdk. And there are the Arsenal errors that are not very standard, where the code would typically have the error string...
Maybe we can type partially at first this file, so we ensure we are working with the right types of errors, and don't miss anything?

lib/storage/data/external/AwsClient.js

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

williamlardier · 2025-09-23T09:55:30Z

lib/network/kmsAWS/Client.ts

-                // Prefer ARN, but fall back to KeyId if ARN is missing
-                // eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain
-                keyId = keyMetadata?.Arn ?? keyMetadata?.KeyId!;
+                keyId = keyMetadata?.Arn ?? (keyMetadata?.KeyId || '');


I think this is important top keep the comment about double arn prefix just below.

This one:

// May produce double arn prefix: scality arn + aws arn // arn:scality:kms:external:aws_kms:custom:key/arn:aws:kms:region:accountId:key/cbd69d33-ba8e-4b56-8cfe // If this is a problem, a config flag should be used to hide the scality arn when returning the KMS KeyId // or aws arn when creating the KMS Key

(sill applicable)

williamlardier · 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?

williamlardier · 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

williamlardier · 2025-09-23T10:04:35Z

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

+    /**
+     * List objects in a bucket.
+     * @param {object} params - S3 listObjects params
+     * @param {function} callback
+     */
+    listObjects(params, callback) {


Can we maybe deprecate the callback approach, as we currently support both?
Also what is returned is actually not a function but a promise, so doing an await listObjects(...) here will work, even if we don't provide any callback.

Or, if we want to only support callback, better not to return the promise at all, to avoid mixed ways of calling this function?

I think this is still relevant; a way to incrementally switch to async/await is to start enforcing this pattern. Caller can then "callbackify" temporarily if using promises is not possible, what do you think?

lib/network/utils.ts

williamlardier

mostly code style, so approving, could be nice to test if we can use S3ServiceException, it would allow better error handling logic I think

williamlardier · 2025-10-16T10:28:46Z

lib/auth/v4/streamingV4/constructChunkStringToSign.ts

-        const temp = justDataChunk instanceof Buffer
-            ? hash.update(justDataChunk)
-            : hash.update(justDataChunk, 'binary');
+        const temp = hash.update(justDataChunk);


Suggested change

const temp = hash.update(justDataChunk);

const temp = justDataChunk instanceof Buffer

? hash.update(justDataChunk)

: hash.update(justDataChunk, 'binary');

If we are sure this is a buffer, we should update the function from

justDataChunk?: Buffer | string,

to

justDataChunk?: Buffer,

williamlardier · 2025-10-16T11:35:10Z

lib/network/kmsAWS/Client.ts

+                logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
+                return cb(null);


If we return no error we want at least to have a warning log I think?

Suggested change

logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });

return cb(null);

logger.warn('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });

return cb(null);

williamlardier · 2025-10-16T11:35: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',

williamlardier · 2025-10-16T11:42:51Z

lib/network/utils.ts

 const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[];

+// Local AWSError type for compatibility with v3 error handling
+export type AWSError = Error & {


I'm pretty sure you can remove this and use the S3ServiceException type, this is exposed by the @aws-sdk/client-s3 module and all exceptions related to S3 are extending this base type, it has all the fields you have here, with the advantage of being typed & documented

williamlardier · 2025-10-16T11:44:57Z

tests/unit/storage/data/external/ExternalClients.spec.js

                const uploadId = 'externalBackendTestUploadId';
                testClient.completeMPU(jsonList, null, key,
                    uploadId, bucketName, log, (err, res) => {
+                        if (err) return done(err);


Suggested change

if (err) return done(err);

if (err) {

return done(err);

}

williamlardier · 2025-10-16T11:46:06Z

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

+            .then(data => callback && callback(null, data))
+            .catch(err => callback?.(err));


Suggested change

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

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

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

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

williamlardier · 2025-10-16T11:49:17Z

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

+    /**
+     * List objects in a bucket.
+     * @param {object} params - S3 listObjects params
+     * @param {function} callback
+     */
+    listObjects(params, callback) {


I think this is still relevant; a way to incrementally switch to async/await is to start enforcing this pattern. Caller can then "callbackify" temporarily if using promises is not possible, what do you think?

williamlardier · 2025-10-16T11:49:57Z

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

+        const logger = { trace: () => {} };
+        const stringToSign = constructStringToSignV2(fakeRequest, data, logger, 'GCP');


up on this, just so we don't forget about the ticket

williamlardier · 2025-10-16T11:51:17Z

lib/storage/data/external/AwsClient.js

+            }).then(() => {
            return callback();


Suggested change

}).then(() => {

return callback();

}).then(() => callback());

williamlardier · 2025-10-16T11:51:35Z

lib/storage/data/external/GcpClient.js

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


Suggested change

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

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

With the migration we can no longer use the serviceDefine property available with aws-sdk v2. This commit aims to extend the s3Client class to override the methods that are not compatible with GCP. Issue: ARSN-514

Issue: ARSN-514

Bumps [@eslint/plugin-kit](https://github.com/eslint/rewrite/tree/HEAD/packages/plugin-kit) from 0.3.5 to 0.4.0. - [Release notes](https://github.com/eslint/rewrite/releases) - [Changelog](https://github.com/eslint/rewrite/blob/main/packages/plugin-kit/CHANGELOG.md) - [Commits](https://github.com/eslint/rewrite/commits/core-v0.4.0/packages/plugin-kit) --- updated-dependencies: - dependency-name: "@eslint/plugin-kit" dependency-version: 0.4.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

DarkIsDude · 2025-10-30T09:08:00Z

lib/network/kmsAWS/Client.ts


    constructor(options: ClientOptions) {
        this._supportsDefaultKeyPerAccount = true;
-        const { providerName, tls, ak, sk, region, endpoint, noAwsArn } = options.kmsAWS;


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

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/network/kmsAWS/Client.ts Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/network/kmsAWS/Client.ts Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/network/utils.ts Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/network/kmsAWS/Client.ts Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/storage/data/external/AwsClient.js Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/storage/data/external/AwsClient.js Show resolved Hide resolved

SylvainSenechal reviewed Sep 19, 2025

View reviewed changes

lib/storage/data/external/AwsClient.js Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 22, 2025

View reviewed changes

tests/unit/storage/data/locConstraintParser.spec.js Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 22, 2025

View reviewed changes

tests/unit/storage/data/DummyService.js Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 22, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

SylvainSenechal reviewed Sep 22, 2025

View reviewed changes

lib/storage/data/external/GCP/GcpService.js Show resolved Hide resolved

williamlardier reviewed Sep 23, 2025

View reviewed changes

francoisferrand requested a review from SylvainSenechal October 1, 2025 10:44

benzekrimaha requested a review from DarkIsDude October 15, 2025 16:47

williamlardier approved these changes Oct 16, 2025

View reviewed changes

benzekrimaha and others added 7 commits October 29, 2025 09:36

gcp client code migration from aws-sdk v2 to v3

4a04d9f

With the migration we can no longer use the serviceDefine property available with aws-sdk v2. This commit aims to extend the s3Client class to override the methods that are not compatible with GCP. Issue: ARSN-514

code migration from aws-sdk v2 to v3

e977b6b

Issue: ARSN-514

tests update post migration from aws-sdk v2 to v3

f2d1862

Issue: ARSN-514

fixup post reviews

dfa82ee

fixup post reviews

b0d7e19

post reviews fixups

30a9a72

benzekrimaha force-pushed the improvement/ARSN-514 branch from d61de14 to 30a9a72 Compare October 29, 2025 08:42

fixup thanks to cloudserver failure

b1ab841

DarkIsDude approved these changes Oct 30, 2025

View reviewed changes

benzekrimaha added 18 commits October 30, 2025 11:11

fixup double middleware usage

2725f3e

fixup double middleware usage

5082db4

fixupp

9050d1d

fixupp

537edef

fixupp

f42ae42

fixupp

afa98e4

fixupp

bbc5736

fixupp

1860908

wip

c1e4353

wip

f22d161

wip

80a5bc1

wip

15a219d

wip

3628f54

region default value

ccbf1cb

wip

cb1406c

wip

13566df

wip

bc68e48

wip

9b89515

		logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
		return cb(null);

		.then(data => callback && callback(null, data))
		.catch(err => callback?.(err));

		const logger = { trace: () => {} };
		const stringToSign = constructStringToSignV2(fakeRequest, data, logger, 'GCP');

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

AWS-SDK Migration #2482

Are you sure you want to change the base?

AWS-SDK Migration #2482

Uh oh!

Conversation

benzekrimaha commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Aug 12, 2025

Hello benzekrimaha,

Uh oh!

bert-e commented Sep 18, 2025

Waiting for approval

Uh oh!

codecov bot commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SylvainSenechal commented Sep 19, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

williamlardier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

williamlardier Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

williamlardier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha commented Aug 12, 2025 •

edited

Loading

codecov bot commented Sep 18, 2025 •

edited

Loading

SylvainSenechal commented Sep 19, 2025 •

edited by atlassian bot

Loading

williamlardier Sep 23, 2025 •

edited

Loading