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: S3 add an option to enable/disable "Expect: 100-continue" headers #2783

Conversation

ThomasVille
Copy link

@ThomasVille ThomasVille commented Jul 24, 2019

Since #437, when using Riak-CS v2.1.1 as backend, uploading files larger than 1MB using streams on NodeJS leads to corrupted data stored in Riak-CS.

This PR adds an option to prevent the Expect: 100-continue header from being added to the content of the file when uploading a large file.

Fixes basho/riak_cs#1327.

Checklist
  • npm run test passes
  • .d.ts file is updated
  • changelog is added, npm run add-change

@ThomasVille
Copy link
Author

The tests fail with the following error which doesn't seem related to the changes introduced in this PR:

> node ./scripts/region-checker/index.js
Hard-coded regions detected. This should only be done if absolutely certain!
File: /config.js	Line 183:	*     accessKeyId: 'AKID', secretAccessKey: 'SECRET', region: 'us-west-2'
File: /services/s3.js	Line 525:	region = 'us-east-1';
File: /services/s3.js	Line 527:	region = 'eu-west-1';
File: /services/s3.js	Line 646:	request.httpRequest.region === 'us-east-1') {
File: /services/s3.js	Line 657:	service.updateReqBucketRegion(request, 'us-east-1');
File: /services/s3.js	Line 658:	if (bucketRegionCache[bucket] !== 'us-east-1') {
File: /services/s3.js	Line 659:	bucketRegionCache[bucket] = 'us-east-1';
File: /services/s3.js	Line 664:	service.updateReqBucketRegion(getRegionReq, 'us-east-1');
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] region-check: `node ./scripts/region-checker/index.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] region-check script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2019-07-24T09_01_46_528Z-debug.log

@ThomasVille
Copy link
Author

@trivikr Do you have any idea on why the tests fail even though I have not changed anything related to regions in my PR?

@nullren
Copy link
Contributor

nullren commented Oct 10, 2019

@ThomasVille there's a whitelist you need to update in scripts/region-checker/whitelist.js. for example, see e74bd88

@ThomasVille ThomasVille force-pushed the feature-s3-add-option-to-enable-expect-100-headers-2 branch from 50e73aa to e4cfe03 Compare October 11, 2019 17:38
@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #2783 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2783      +/-   ##
==========================================
+ Coverage   96.96%   96.96%   +<.01%     
==========================================
  Files         300      300              
  Lines        8991     8994       +3     
  Branches     1676     1677       +1     
==========================================
+ Hits         8718     8721       +3     
  Misses        273      273
Impacted Files Coverage Δ
lib/config.js 87.64% <ø> (ø) ⬆️
lib/services/s3.js 98.19% <100%> (+0.01%) ⬆️

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 34cc60e...c7de385. Read the comment docs.

@ThomasVille ThomasVille force-pushed the feature-s3-add-option-to-enable-expect-100-headers-2 branch from e23460c to 38a96e1 Compare October 11, 2019 17:51
@ThomasVille ThomasVille force-pushed the feature-s3-add-option-to-enable-expect-100-headers-2 branch from 38a96e1 to c7de385 Compare October 11, 2019 17:59
@ThomasVille
Copy link
Author

@nullren Thank you for the hint, I had no idea there was such a whitelist of lines to ignore.

@ajredniwja
Copy link
Contributor

Is this still an active PR? Can the conflicts be merged?

@ajredniwja ajredniwja added closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Jun 16, 2020
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 21, 2020
@github-actions github-actions bot closed this Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multipart upload fails (AccessDenied) when using Nodejs aws-sdk client with v2 signature [JIRA: RCS-380]
4 participants