Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrating to AWS SDK V3 | Add Internal Class to Decide Which AWS SDK Version to Use #7395

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Jul 11, 2023

Explain the changes

  1. Adding a new supportive layer for creating an AWS S3 Client using AWS SDK V2 and AWS SDK V3 this layer will return the S3 Client base on our decision, for example: if the signature version is 'v2' then AWS SDK V2, else AWS SDK V3.
     

Background:

when using AWS SDK V2 we encounter the warning about AWS SDK V2 entering maintenance mode:

We are formalizing our plans to enter AWS SDK for JavaScript (v2) into maintenance mode in 2023.
Please migrate your code to use AWS SDK for JavaScript (v3).
For more information, check the migration guide at https://a.co/7PzMCcy

The API is not backward compatible with the old version, so we need to change our code.
 
One of the changes is related to signatureVersion, by using the AWS SDK V3 we can only use signatureVersion v4.

v2: The signature version to sign requests with (overriding the API configuration).
v3: Deprecated. Signature V2 supported in v2 SDK has been deprecated by AWS. v3 only supports signature v4.

Since the server might only support signatureVersion v2, we still need to support it in our s3-client generated from the AWS SDK.

Issues: Fixed #xxx / Gap #xxx

  1. None

Testing Instructions:

set AWS_SDK_VERSION_3_DISABLED to false.

AWS Server

  1. Create a new js file in the repo with all needed parameters to create a new instance of s3 client (using aws sdk v3 params), for example, access key, secret access key, endpoint ('https://s3.amazonaws.com'), signature version ('v2'), and region.
  2. Run operations that were implemented in the class, for example: listObjectsV2, and print the result.
  3. Repeat steps 1-2 with signature version v4.

To run jest tests:
npx jest test_noobaa_s3_client.test.js

  • Doc added/updated
  • Tests added - add a couple of simple tests with jest framework (checks the decision on which SDK version to use, checks automated replacement of params).

@shirady shirady marked this pull request as draft July 11, 2023 11:37
@shirady shirady self-assigned this Jul 11, 2023
@shirady shirady force-pushed the migrate-aws-sdk-s3-client branch 3 times, most recently from 244b72d to 244a7a7 Compare July 13, 2023 11:17
@shirady shirady marked this pull request as ready for review July 13, 2023 11:18
@shirady shirady requested a review from liranmauda July 16, 2023 05:38
Copy link
Contributor

@dannyzaken dannyzaken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of unnecessary abstraction in this PR.
According to noobaa_s3_client_sdkv3rg.js, it looks like the interface is almost unchanged (in V2 there is .promise and in v3 Body is returned as stream.

can we instead, for V3, return the actual client (new S3(...)), and for only for V2 have a layer to do the necessary fixes? we still have to handle the conversion from stream to buffer in V3. we already have a util for that in buffer_utils (see here)

@shirady shirady marked this pull request as draft July 17, 2023 11:15
@shirady shirady marked this pull request as ready for review July 17, 2023 11:20
@shirady shirady requested a review from dannyzaken July 17, 2023 11:20
@shirady shirady requested a review from dannyzaken July 20, 2023 11:18
@shirady shirady requested a review from dannyzaken July 23, 2023 12:48
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady merged commit e34c10b into noobaa:master Jul 26, 2023
7 checks passed
@shirady shirady deleted the migrate-aws-sdk-s3-client branch July 27, 2023 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants