AWS-SDK Migration #2482
AWS-SDK Migration #2482
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
from
ad7e01e to
2f3ed00
Compare
benzekrimaha
changed the base branch from
development/8.2
to
development/8.3
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
2 times, most recently
from
f8e5940 to
942aa25
Compare
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
from
942aa25 to
f9f96ce
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## development/8.3 #2482 +/- ##
===================================================
- Coverage 71.07% 70.22% -0.85%
===================================================
Files 220 218 -2
Lines 17736 17789 +53
Branches 3683 3669 -14
===================================================
- Hits 12605 12492 -113
- Misses 5127 5293 +166
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Wait, you say you've got two Jira ticket 514/524, but they both point to this same PR 🤔 |
lib/network/kmsAWS/Client.ts
Outdated
| endpoint, | ||
| httpOptions, | ||
| ...credentials, | ||
| requestHandler: undefined, // v3 handles agents differently |
There was a problem hiding this comment.
Aren't we dropping tls support here ? If you say it is still handled but differently, maybe reference where/how it is done. Or maybe it's just not needed ?
In Vault I was able to use it like this https://github.com/scality/vault2/pull/177/files#diff-a78b27e0448a01885b6f5220e7a97d50c0729ff792356c653a09bf67361446e9R154
Other example in s3utils : https://github.com/scality/s3utils/blob/development/1.16/VerifyReplication/storage/s3.js#L63
lib/network/utils.ts
Outdated
| const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[]; | ||
|
|
||
| // Local AWSError type for compatibility with v3 error handling | ||
| export type AWSError = Error & { |
There was a problem hiding this comment.
I haven't tested/run the code but just wanna double check, you sure this file is ok this way ? I mean specifically, I'm looking at line 50, should err.code be replaced with err.name ?
There was a problem hiding this comment.
I see the isAWSError below. I think this part should be migrated as well, because they indeed use name now, plus, we should use instanceOf as documented here: https://aws.amazon.com/blogs/developer/service-error-handling-modular-aws-sdk-js/.
If we need this type then it means we either need to migrate to TS now to ensure we are not missing anything (it can be hard to see all affected places), or we should cover all error cases in the unit tests, to ensure the affected logic will still handle properly errors, even if we still use .code (because maybe we wrap the errors and use an arsenal one?)
| 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)); |
There was a problem hiding this comment.
| .catch(err => putCb(err)); | |
| .catch(putCb); |
Can double check other occurrences in this file & elsewhere
| }; | ||
| return callback(null, response); | ||
| }).catch(err => { | ||
| if (err.code === 'NoSuchKey' || err.code === 'NotFound') { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
| }); | ||
| }); | ||
|
|
||
| it('should create a new master encryption key', done => { |
There was a problem hiding this comment.
Looks like a duplicate of the first test "should support default encryption key per account" 🤔
| "ajv": "6.12.3", | ||
| "async": "~2.6.4", | ||
| "aws-sdk": "^2.1691.0", | ||
| "@aws-sdk/client-s3": "^3.540.0", |
There was a problem hiding this comment.
I checked the 3.540 release, it's already from Mar 22, 2024. Do you think we can pick something more recent ? I guess it should be compatible with current migrated code
| .customizeDescription('You must specify at least one part'); | ||
| return callback(error); | ||
| } | ||
| for (let ind = 1; ind < partList.length; ++ind) { |
There was a problem hiding this comment.
Not sure if this is likely to happen in practice, but does that need stricter checking than simple ordering ? For example here, [1,2,10] would be valid here but maybe we want to enforce [1,2,3] (or [0,1,2]) ?
Maybe checking that that partList[i] = i + 1 (or i)
There was a problem hiding this comment.
That is not required, we can provide any number (a subset) of all uploaded parts during a complete, as long as they are in correct order and we are not providing a part number that was missing from the initial upload phase, this is accepted. So with |1,2,3,4,5,6,7,9,11] uploaded, we can complete with [1,3,5,11] if we want.
| listObjects(params, callback) { | ||
| return this.send(new ListObjectsCommand(params)) | ||
| .then(data => callback && callback(null, data)) | ||
| .catch(err => callback && callback(err)); | ||
| } |
There was a problem hiding this comment.
It could make sense to switch this file to ts, but the callback should be required?
If no you can do that
| 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?
| }; | ||
| return callback(null, response); | ||
| }).catch(err => { | ||
| if (err.code === 'NoSuchKey' || err.code === 'NotFound') { |
There was a problem hiding this comment.
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?
| .customizeDescription('You must specify at least one part'); | ||
| return callback(error); | ||
| } | ||
| for (let ind = 1; ind < partList.length; ++ind) { |
There was a problem hiding this comment.
That is not required, we can provide any number (a subset) of all uploaded parts during a complete, as long as they are in correct order and we are not providing a part number that was missing from the initial upload phase, this is accepted. So with |1,2,3,4,5,6,7,9,11] uploaded, we can complete with [1,3,5,11] if we want.
| // 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 || ''); |
There was a problem hiding this comment.
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
| maxAttempts: 1, | ||
| requestHandler: undefined, // v3 handles agents differently | ||
| // v3 does not use signatureVersion or sslEnabled directly | ||
| // v3 does not use customUserAgent directly |
There was a problem hiding this comment.
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?
lib/network/kmsAWS/Client.ts
Outdated
| const params = { | ||
| KeyId: masterKeyId, | ||
| KeySpec: 'AES_256', | ||
| KeySpec: "AES_256" as const, |
There was a problem hiding this comment.
| 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
| /** | ||
| * List objects in a bucket. | ||
| * @param {object} params - S3 listObjects params | ||
| * @param {function} callback | ||
| */ | ||
| listObjects(params, callback) { |
There was a problem hiding this comment.
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?
lib/network/utils.ts
Outdated
| const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[]; | ||
|
|
||
| // Local AWSError type for compatibility with v3 error handling | ||
| export type AWSError = Error & { |
There was a problem hiding this comment.
I see the isAWSError below. I think this part should be migrated as well, because they indeed use name now, plus, we should use instanceOf as documented here: https://aws.amazon.com/blogs/developer/service-error-handling-modular-aws-sdk-js/.
If we need this type then it means we either need to migrate to TS now to ensure we are not missing anything (it can be hard to see all affected places), or we should cover all error cases in the unit tests, to ensure the affected logic will still handle properly errors, even if we still use .code (because maybe we wrap the errors and use an arsenal one?)
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/improvement/ARSN-514 && \
git merge origin/development/8.3Resolve merge conflicts and commit git push origin HEAD:improvement/ARSN-514 |
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
from
fa33e20 to
915c31f
Compare
| this._client.send(new GetObjectCommand(params)).then(data => { | ||
| // Always return an object with .createReadStream for test compatibility | ||
| const stream = data.Body; | ||
| const abort = () => { | ||
| if (isAborted) return; // Prevent multiple abort calls | ||
| isAborted = true; | ||
|
|
There was a problem hiding this comment.
I'm not sure to see where this variable is defined? We first initialize it in the next line, shouldn't we create it out of the scope of abort?
| this._client.send(new GetObjectCommand(params)).then(data => { | |
| // Always return an object with .createReadStream for test compatibility | |
| const stream = data.Body; | |
| const abort = () => { | |
| if (isAborted) return; // Prevent multiple abort calls | |
| isAborted = true; | |
| this._client.send(new GetObjectCommand(params)).then(data => { | |
| // Always return an object with .createReadStream for test compatibility | |
| const stream = data.Body; | |
| let isAborted = false; | |
| const abort = () => { | |
| if (isAborted) { | |
| return; // Prevent multiple abort calls | |
| } | |
| isAborted = true; | |
| if (typeof stream.destroy === 'function') { | ||
| stream.destroy(); | ||
| } else if (typeof stream.abort === 'function') { | ||
| stream.abort(); | ||
| } else if (typeof stream.close === 'function') { | ||
| stream.close(); | ||
| } else if (typeof stream.end === 'function') { | ||
| stream.end(); | ||
| } |
There was a problem hiding this comment.
| if (typeof stream.destroy === 'function') { | |
| stream.destroy(); | |
| } else if (typeof stream.abort === 'function') { | |
| stream.abort(); | |
| } else if (typeof stream.close === 'function') { | |
| stream.close(); | |
| } else if (typeof stream.end === 'function') { | |
| stream.end(); | |
| } | |
| (stream?.destroy || stream?.abort || stream?.close || stream?.end)?.(); |
?
| if (typeof stream.removeAllListeners === 'function') { | ||
| stream.removeAllListeners(); | ||
| } |
There was a problem hiding this comment.
| if (typeof stream.removeAllListeners === 'function') { | |
| stream.removeAllListeners(); | |
| } | |
| stream.removeAllListeners?.(); |
| if (stream && !stream.abort) { | ||
| stream.abort = abort; | ||
| } |
There was a problem hiding this comment.
| if (stream && !stream.abort) { | |
| stream.abort = abort; | |
| } | |
| if (!stream?.abort) { | |
| stream.abort = abort; | |
| } |
| const response = { | ||
| createReadStream: () => stream, | ||
| Body: stream, | ||
| abort, | ||
| destroy: abort, | ||
| }; | ||
| return callback(null, response); |
There was a problem hiding this comment.
| const response = { | |
| createReadStream: () => stream, | |
| Body: stream, | |
| abort, | |
| destroy: abort, | |
| }; | |
| return callback(null, response); | |
| return callback(null, { | |
| createReadStream: () => stream, | |
| Body: stream, | |
| abort, | |
| destroy, | |
| }); |
(with the new abort variable name)
| endpoint: `${protocol}://${endpoint}`, | ||
| region, | ||
| requestHandler: new NodeHttpHandler({ | ||
| socketTimeout: 0, |
There was a problem hiding this comment.
From this doc it seems we should use requestTimeout
| // If error is NoSuchKey, bucket exists but key doesn't (which is expected) | ||
| if (err instanceof NoSuchKey || err instanceof NotFound) { | ||
| callback(null, {}); | ||
| } | ||
| // Other errors likely mean bucket doesn't exist or access denied | ||
| callback(err); |
There was a problem hiding this comment.
otherwise we call both callbacks
| // If error is NoSuchKey, bucket exists but key doesn't (which is expected) | |
| if (err instanceof NoSuchKey || err instanceof NotFound) { | |
| callback(null, {}); | |
| } | |
| // Other errors likely mean bucket doesn't exist or access denied | |
| callback(err); | |
| // If error is NoSuchKey, bucket exists but key doesn't (which is expected) | |
| if (err instanceof NoSuchKey || err instanceof NotFound) { | |
| return callback(null, {}); | |
| } | |
| // Other errors likely mean bucket doesn't exist or access denied | |
| return callback(err); |
| const logger = { trace: () => {} }; | ||
| const stringToSign = constructStringToSignV2(fakeRequest, data, logger, 'GCP'); |
There was a problem hiding this comment.
I think we should probably use a real class logger here, and properly pass the request IDs, as here it would remove any logs in case of real issues?
(can be in a followup ticket?)
| awsResp[location] = { error: err, external: true }; | ||
| return callback(null, awsResp); | ||
| } | ||
| }).then(() => { |
There was a problem hiding this comment.
Maybe not the order of promise we want, as after calling the callback in the .catch, we will execute the .then?
| `${this.type}: ${err.message}`), | ||
| ); | ||
| } | ||
| }).then(completeMpuRes => { |
There was a problem hiding this comment.
Better to keep the .then before the .catch here I think, same reason as above comment
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
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
from
915c31f to
bdf2a5c
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
benzekrimaha
force-pushed
the
improvement/ARSN-514
branch
from
8b58d38 to
b4822bf
Compare
| endpoint, | ||
| }); | ||
| this._client.endpoint = new AWS.Endpoint(endpoint); | ||
| this._client = new S3Client({ ...this._s3Params, endpoint }); |
There was a problem hiding this comment.
I think it needs region ?
| this._client = new S3Client({ ...this._s3Params, endpoint }); | |
| this._client = new S3Client({ ...this._s3Params, region, endpoint }); |
| // set regional endpoint | ||
| region = err.region; | ||
| } else if (res) { | ||
| if (res) { |
There was a problem hiding this comment.
Pretty sure if (res) is not needed, but LocationConstraint can be undefined according to the typescript structure, should be fine though, if the region is undefined new S3Client handles it
| (stream?.destroy || stream?.abort || stream?.close || stream?.end || stream?.removeAllListeners)?.(); | ||
| }; | ||
| if (!stream?.abort) { | ||
| stream.abort = abort; |
There was a problem hiding this comment.
Wait, abort is not defined, should it be destroy ?
There was a problem hiding this comment.
Yeah after reviewing William's comment (#2482 (comment)), I think you changed the function name was changed but this line wasn't changed
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