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

fix(@aws-amplify/storage): Automatically adjust systemClockoffset in Storage #9115

Merged
merged 21 commits into from
Nov 4, 2021

Conversation

jamesaucode
Copy link
Contributor

@jamesaucode jamesaucode commented Oct 27, 2021

Description of changes

  • Instead of throwing any error caught inside AxiosHttpHandler, we should return an HttpResponse with the related info caught from the error, else all the s3middleware including error serializing and retry middleware will fail if any error is caught.
  • added an autoAdjustClockskewMiddleware that will automatically adjust the client option's systemClockoffset if a RequestTimeTooSkewed error is encountered.
  • createNewS3Client is re-used in multiple files, extracted it's logic to S3ClientUtils.ts
  • Modified AWSManagedUpload so that it doesn't create S3Client for every single part. Instead it uses 1 main s3client inside the class.
  • Allow AxiosHttpHandler to take call-specific EventsEmitter, this allows multipart upload to pass event emitter that belongs to an individual part.

Edit: Newer aws-sdk introduced some changes that will be breaking for our React Native customers. Therefore we are adding this middleware as a temporary workaround to unblock customers with clock skew issues.

Issue #, if available

fixes #6494

Description of how you validated changes

Unit tests
Manually tested by adjusting the system clock and doing Storage api calls

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jamesaucode jamesaucode changed the title Storage/clockskew fix(@aws-amplify/storage): Automatically adjust systemClockoffset in Storage Oct 27, 2021
@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2021

This pull request introduces 2 alerts when merging 580ee21 into 5d5083c - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@jamesaucode jamesaucode added Storage Related to Storage components/category needs-review Used as a label in order to review open PRs labels Oct 27, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2021

Codecov Report

Merging #9115 (5d8af6c) into main (4ef8aa9) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9115      +/-   ##
==========================================
+ Coverage   78.01%   78.07%   +0.05%     
==========================================
  Files         250      250              
  Lines       18116    18122       +6     
  Branches     3889     3891       +2     
==========================================
+ Hits        14133    14148      +15     
+ Misses       3853     3844       -9     
  Partials      130      130              
Impacted Files Coverage Δ
packages/storage/src/common/S3ClientUtils.ts 96.15% <100.00%> (+4.04%) ⬆️
packages/storage/src/providers/AWSS3Provider.ts 91.05% <100.00%> (+2.06%) ⬆️
...torage/src/providers/AWSS3ProviderManagedUpload.ts 99.23% <100.00%> (+0.57%) ⬆️
...ckages/storage/src/providers/axios-http-handler.ts 98.85% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ef8aa9...5d8af6c. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2021

This pull request introduces 1 alert when merging 03faab9 into 2b800f5 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@jamesaucode
Copy link
Contributor Author

Closing this as we as a team decided to upgrade the @aws-sdk version (that includes a patch that will fix this clockskew issue) instead of rolling out our own middleware. I will link the PR here when it's out.

@jamesaucode jamesaucode reopened this Nov 4, 2021
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

Comment on lines +99 to +104
if (isTimeSkewedError(err)) {
const serverDate = new Date(err.ServerTime);
config.systemClockOffset = serverDate.getTime() - Date.now();
}
throw err;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamesaucode
Copy link
Contributor Author

Upgrading aws-sdk might actually take us longer and requires more discussion with the team. We are re-opening this PR as a temporary workaround to unblock any users with clock skew issues with Storage.

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Just one qustions
Overall LGTM
Thanks @jamesaucode 🌮

Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

LGTM thanks @jamesaucode 🌮 🏅

@jamesaucode jamesaucode merged commit 873941c into aws-amplify:main Nov 4, 2021
@jamesaucode jamesaucode deleted the storage/clockskew branch November 5, 2021 00:47
@wlee221 wlee221 mentioned this pull request Dec 6, 2021
4 tasks
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-review Used as a label in order to review open PRs Storage Related to Storage components/category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock Drift/Skew code does not handle all skew error scenarios
5 participants