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

feat(node22): replace aws-sdk with @aws-sdk/s3, remove npm dependency #402

Merged
merged 7 commits into from
Feb 18, 2025

Conversation

nkavtur
Copy link
Contributor

@nkavtur nkavtur commented Feb 11, 2025

✏️ Changes

This PR includes the following changes:

  1. Replaced aws-sdk with @aws-sdk/s3-client. Previously, auth0-extension-s3-tools relied on aws-sdk for S3 metadata storage. Now that we're using @aws-sdk/s3-client directly, so auth0-extension-s3-tools is no longer needed. A custom S3 client wrapper has been added to replace it.

  2. Removed npm dependency since it is not used for anything

Notes about @aws-sdk/s3-client:

The key difference between aws-sdk and @aws-sdk/client-s3 is how they handle region resolution when instantiating an S3 client:

aws-sdk (v2) automatically resolved the region. It was using this logic:

  1. When an error occurred, it extracted the x-amz-bucket-region header from the response and assigned it to error.region (extractError).
  2. If the error was a redirect-related error, it updated request.httpRequest.region = error.region before retrying (retryableError).

@aws-sdk/client-s3 (v3) does not handle region resolution automatically—you must specify the region explicitly when creating the S3 client. To keep the old behavior, I implemented a wrapper around the new S3 client that replicates the region-handling logic from aws-sdk v2.

🔗 References

Jira Ticket

🎯 Testing

✅ This change has been tested in a Webtask

✅ This change has unit test coverage

✅ This change has integration test coverage

🚫 This change has been tested for performance

  1. Follow the testing steps described in this PR to build and deploy authz-extension to the dev space.
  2. Install extension and configure extension to use s3 for metadata storage.

Screenshot 2025-02-17 at 13 57 53

  1. Perform manual testing and verify extension is working fine using s3 storage

🚀 Deployment

✅ This can be deployed any time

🎡 Rollout

In order to verify that the deployment was successful we will test the extension in prod spaces before making it public available.

🔥 Rollback

We will rollback changes if it breaks anything.

📄 Procedure

Explain how the rollback for this change will look like, how we can recover fast.

🖥 Appliance

Note to reviewers: ensure that this change is compatible with the Appliance.

@sauntimo sauntimo requested review from sauntimo February 18, 2025 10:31
sauntimo
sauntimo previously approved these changes Feb 18, 2025
Copy link
Contributor

@sauntimo sauntimo left a comment

Choose a reason for hiding this comment

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

  • switching the heavy aws-sdk and dropping unnecessary deps for much lighter ones is an awesome improvement, great work @nkavtur!
  • My only comment is that you'll need to update the version for the extension in:
    • package.json (and package-lock.json)
    • webtask.json
    • html.js EXTENSION_VERSION - this hardcoded value is used in the title of the GUI which makes it easier to debug if customers send us screenshots
    • and add an entry in the change log

I've verified this PR in the following ways;

  • tested in dev space with new node22 extension version using demozero extensions page:
    image

  • datadog logs showing node22 is used
    image

  • successfully ran integration tests against space
    image

  • Also verified locally that all 170 unit tests are passing

@nkavtur nkavtur marked this pull request as ready for review February 18, 2025 12:08
@nkavtur nkavtur merged commit 03fca39 into master Feb 18, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants