rgw: handle old clients with transfer-encoding: chunked. #50235

mkogan1 · 2023-02-23T15:49:35Z

s3 clients should provide an x-amz-decoded-content-length field when they use transport-encoding: chunked. Some clients do not. With swift we already allow chunked uploads that do not specify the
content length in advance. This commit adds similar support
for s3. Known client affected by this: boto2.

Fixes: https://tracker.ceph.com/issues/45789
Resolves: rhbz#2152801

Signed-off-by: Marcus Watts mwatts@redhat.com

= = = = = 8< = = = = =

Additional details from the BZ:
Description of problem:

Cu is getting error 411, missing content length error for PutObject operations for clients accessing via aws-sdk
Cu confirmed following implementation details:

library is '@aws-sdk/client-s3' with the version 3.54.0
they use it in javascript (node)
specific request type is PutObject with a stream, the content I can't give but it happens for any file from their testing, but please try various of object sizes.
The exact error is: "MissigContentLength: UnknownError"

Cu script:

import fs from 'fs';
import { Readable } from 'stream';

import { S3, PutObjectCommandInput, PutObjectCommandOutput } from '@aws-sdk/client-s3';
import { SdkStream } from '@aws-sdk/types';

const file = fs.createReadStream('/file/path')


const s3client = new S3({
	region: 's3Region',
	tls: true,
	forcePathStyle: true
	credentials: {
		accessKeyId: 'access key',
		secretAccessKey: 'secret key'
	},
	endpoint: 'endpoint'
});


const params: PutObjectCommandInput = {
	Bucket: bucket,
	Key: key,
	Body: file
};

const upload: PutObjectCommandOutput = await s3client.putObject(params, {});

return upload;

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

s3 clients *should* provide an x-amz-decoded-content-length field when they use transport-encoding: chunked. Some clients do not. With swift we already allow chunked uploads that do not specify the content length in advance. This commit adds similar support for s3. Known client affected by this: boto2. Fixes: https://tracker.ceph.com/issues/45789 Resolves: rhbz#2152801 Signed-off-by: Marcus Watts <mwatts@redhat.com>

cbodley · 2023-02-23T16:03:29Z

if there's a reproducer for this, can we please turn it into a s3test case? if boto2 is required, we still have some boto2 tests under s3tests/

Before the RGW fix PR was responding with 411 instead of 200 RGW fix PR: ceph/ceph#50235 Signed-off-by: Mark Kogan <mkogan@redhat.com>

mkogan1 · 2023-03-07T14:13:27Z

if there's a reproducer for this, can we please turn it into a s3test case? if boto2 is required, we still have some boto2 tests under s3tests/

@cbodley added s3test PR: ceph/s3-tests#501

mkogan1 · 2023-03-07T14:14:31Z

jenkins test make check

cbodley · 2023-03-07T14:31:33Z

we'll need to make sure this gets run against a suite-branch that modifies qa/rgw/s3tests-branch.yaml to point at your mkogan1:wip-chunked-trans-enc s3test branch

mkogan1 · 2023-03-07T16:56:09Z

jenkins test api

mkogan1 · 2023-03-16T17:21:10Z

@ivancich sorry, currently it will fail wip-eric-testing-1
https://github.com/ceph/java_s3tests/blob/81319fea9d8be9b8083723dd74ee59ff94f0351e/src/test/java/ObjectTest.java#L472
because there are test changes, working on testing per instructions in #50235 (comment) above

mattbenjamin

marcus says sounds plausible, he will review

mkogan1 · 2023-03-20T11:13:31Z

added java_s3test PR: ceph/java_s3tests#13

following ceph PR ceph/ceph#50235 it was found that the test 'object create w/negative content length, fails' test falsely fails on object PUT with chunked transfer encoding and does not attempts to create an object with a negative content length for details referring to discussion on: ceph#13 Signed-off-by: Mark Kogan <mkogan@redhat.com>

following ceph PR ceph/ceph#50235 it was found that the test 'object create w/negative content length, fails' test falsely fails on object PUT with chunked transfer encoding and does not attempts to create an object with a negative content length for details referring to discussion on: #13 Signed-off-by: Mark Kogan <mkogan@redhat.com> (cherry picked from commit d625bfa)

ivancich · 2023-03-20T20:02:27Z

In my QA branch all my rgw/verify jobs fail due to this:

2023-03-20T03:40:41.347 INFO:teuthology.orchestra.run.smithi033.stdout:suite > Object tests > ObjectTest.testObjectCreateBadContentlengthNegative FAILED
2023-03-20T03:40:41.348 INFO:teuthology.orchestra.run.smithi033.stdout:    java.lang.AssertionError at ObjectTest.java:472

Of the 4 PRs on the branch, this seems the most likely to involve this. I'm verifying.

cbodley · 2023-03-20T20:21:31Z

thanks @ivancich. this was expected, yeah:

@ivancich sorry, currently it will fail wip-eric-testing-1 https://github.com/ceph/java_s3tests/blob/81319fea9d8be9b8083723dd74ee59ff94f0351e/src/test/java/ObjectTest.java#L472 because there are test changes, working on testing per instructions in #50235 (comment) above

ivancich · 2023-03-21T12:54:07Z

thanks @ivancich. this was expected, yeah:

@ivancich sorry, currently it will fail wip-eric-testing-1 https://github.com/ceph/java_s3tests/blob/81319fea9d8be9b8083723dd74ee59ff94f0351e/src/test/java/ObjectTest.java#L472 because there are test changes, working on testing per instructions in #50235 (comment) above

Sorry, I missed that comment directed at me above. How would you like me to handle QA then -- pull this PR out of the QA run or count this as a pass?

mkogan1 · 2023-03-21T12:59:15Z

thanks @ivancich. this was expected, yeah:

@ivancich sorry, currently it will fail wip-eric-testing-1 https://github.com/ceph/java_s3tests/blob/81319fea9d8be9b8083723dd74ee59ff94f0351e/src/test/java/ObjectTest.java#L472 because there are test changes, working on testing per instructions in #50235 (comment) above

Sorry, I missed that comment directed at me above. How would you like me to handle QA then -- pull this PR out of the QA run or count this as a pass?

following this ceph/java_s3tests#14 merging it should be OK to test again and it should pass now

mkogan1 · 2023-03-21T15:52:39Z

@cbodley
this PR passed teuthology rgw:verify suite & --limit 10 and a suite-repo&branch
with the explicit additional s3test for this scenario from: ceph/s3-tests#501

https://pulpito.ceph.com/mkogan-2023-03-21_13:51:47-rgw:verify-wip-chuncked-put-i04-distro-default-smithi/ -->
http://qa-proxy.ceph.com/teuthology/mkogan-2023-03-21_13:51:47-rgw:verify-wip-chuncked-put-i04-distro-default-smithi/7215297/teuthology.log : test_object_write_with_chunked_transfer_encoding PASSED [ 36%]

2023-03-21T14:25:08.794 INFO:teuthology.orchestra.run.smithi149.stdout:s3tests_boto3/functional/test_s3.py::test_object_write_with_chunked_transfer_encoding PASSED [ 36%]

is rgw:verify suite & --limit 10 enough or should I submit full rgw:verify or rgw?

@ivancich
sorry if you already submitted your teuthology runs, the s3tests with not failed because of this PR after ceph/java_s3tests#14 but without ceph/s3-tests#501 (which has not merged yet) they also do not verify this PR correctness

mkogan1 · 2023-03-21T15:54:50Z

jenkins test windows

mkogan1 · 2023-03-21T15:55:15Z

jenkins test make check arm64

cbodley · 2023-03-21T15:56:20Z

is rgw:verify suite & --limit 10 enough or should I submit full rgw:verify or rgw?

please run the whole rgw suite. we run s3tests in other subsuites like rgw/multifs, rgw/sts, rgw/crypt, etc

mdw-at-linuxbox · 2023-03-21T18:07:04Z

Looks fine - only thing I can see is do you really still want to call this "rgw/civetweb"?

mdw-at-linuxbox

Looks fine (other than calling it a "civetweb" change)

mkogan1 · 2023-03-22T05:07:44Z

Looks fine - only thing I can see is do you really still want to call this "rgw/civetweb"?

thank you, changed the PR title, preferred to leave the commit title to keep track all the way back when searching the commit history of various old release branches by commit tile

mkogan1 · 2023-03-22T05:15:57Z

jenkins test make check arm64

mkogan1 · 2023-03-22T14:20:47Z

queued full rgw suite tueuthology run,
https://pulpito.ceph.com/mkogan-2023-03-21_16:00:08-rgw-wip-chuncked-put-i04-distro-default-smithi/

ivancich · 2023-03-23T15:32:59Z

QA run:

ivancich · 2023-03-23T15:35:27Z

I think we should wait until this passes the rgw upgrade suite.

cbodley · 2023-03-23T15:40:25Z

I think we should wait until this passes the rgw upgrade suite.

we're waiting on Mark's results, which are running against the new test case in ceph/s3-tests#501

ivancich · 2023-03-23T15:58:41Z

I think we should wait until this passes the rgw upgrade suite.

we're waiting on Mark's results, which are running against the new test case in ceph/s3-tests#501

OK, then I'll pull this off of my QA run and won't merge.

mkogan1 · 2023-03-27T07:40:26Z

@cbodley @ivancich ACK
a full rgw suite run with the new test_object_write_with_chunked_transfer_encoding test (ceph/s3-tests#501)
has completed successfully below, none of the failures is related to the new test
https://pulpito.ceph.com/mkogan-2023-03-21_16:00:08-rgw-wip-chuncked-put-i04-distro-default-smithi/
for representative example: http://qa-proxy.ceph.com/teuthology/mkogan-2023-03-21_16:00:08-rgw-wip-chuncked-put-i04-distro-default-smithi/7215423/teuthology.log :
2023-03-24T11:02:22.344 INFO:teuthology.orchestra.run.smithi026.stdout:s3tests_boto3/functional/test_s3.py::test_object_write_with_chunked_transfer_encoding PASSED [ 36%]

mkogan1 · 2023-04-04T09:45:31Z

@cbodley @ivancich ACK a full rgw suite run with the new test_object_write_with_chunked_transfer_encoding test (ceph/s3-tests#501) has completed successfully below, none of the failures is related to the new test https://pulpito.ceph.com/mkogan-2023-03-21_16:00:08-rgw-wip-chuncked-put-i04-distro-default-smithi/ for representative example: http://qa-proxy.ceph.com/teuthology/mkogan-2023-03-21_16:00:08-rgw-wip-chuncked-put-i04-distro-default-smithi/7215423/teuthology.log : 2023-03-24T11:02:22.344 INFO:teuthology.orchestra.run.smithi026.stdout:s3tests_boto3/functional/test_s3.py::test_object_write_with_chunked_transfer_encoding PASSED [ 36%]

@cbodley may please ask to take another look when there's time, any additional tests?

Before the RGW fix PR was responding with 411 instead of 200 RGW fix PR: ceph/ceph#50235 Signed-off-by: Mark Kogan <mkogan@redhat.com> (cherry picked from commit 13a9bfc)

rgw: handle old clients with transfer-encoding: chunked. Reviewed-by: Casey Bodley <cbodley@redhat.com> Reviewed-by: Marcus Watts <mwatts@redhat.com>

mkogan1 added bug-fix rgw labels Feb 23, 2023

mkogan1 requested review from mattbenjamin, cbodley and mdw-at-linuxbox February 23, 2023 15:49

mkogan1 requested a review from a team as a code owner February 23, 2023 15:49

mkogan1 added a commit to mkogan1/s3-tests that referenced this pull request Mar 7, 2023

test object PUT with chunked transfer enconding

13a9bfc

Before the RGW fix PR was responding with 411 instead of 200 RGW fix PR: ceph/ceph#50235 Signed-off-by: Mark Kogan <mkogan@redhat.com>

mkogan1 mentioned this pull request Mar 7, 2023

Test object PUT with chunked transfer encoding ceph/s3-tests#501

Merged

cbodley approved these changes Mar 7, 2023

View reviewed changes

cbodley added the needs-qa label Mar 7, 2023

ivancich added the wip-eric-testing-1 for ivancich testing label Mar 15, 2023

mattbenjamin reviewed Mar 16, 2023

View reviewed changes

mkogan1 mentioned this pull request Mar 20, 2023

remove unnecessary test testObjectCreateBadContentlengthNegative ceph/java_s3tests#14

Merged

mdw-at-linuxbox approved these changes Mar 21, 2023

View reviewed changes

mkogan1 changed the title ~~rgw/civetweb: handle old clients with transfer-encoding: chunked.~~ rgw: handle old clients with transfer-encoding: chunked. Mar 22, 2023

cbodley added the wip-mkogan-testing label Mar 23, 2023

ivancich removed the wip-eric-testing-1 for ivancich testing label Mar 23, 2023

cbodley merged commit 1509b58 into ceph:main Apr 7, 2023
14 of 19 checks passed

rgw: handle old clients with transfer-encoding: chunked. #50235

rgw: handle old clients with transfer-encoding: chunked. #50235

mkogan1 commented Feb 23, 2023 •

edited

cbodley commented Feb 23, 2023

mkogan1 commented Mar 7, 2023

mkogan1 commented Mar 7, 2023

cbodley commented Mar 7, 2023

mkogan1 commented Mar 7, 2023

mkogan1 commented Mar 16, 2023

mattbenjamin left a comment

mkogan1 commented Mar 20, 2023

ivancich commented Mar 20, 2023

cbodley commented Mar 20, 2023

ivancich commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

cbodley commented Mar 21, 2023

mdw-at-linuxbox commented Mar 21, 2023

mdw-at-linuxbox left a comment

mkogan1 commented Mar 22, 2023

mkogan1 commented Mar 22, 2023

mkogan1 commented Mar 22, 2023

ivancich commented Mar 23, 2023

ivancich commented Mar 23, 2023

cbodley commented Mar 23, 2023

ivancich commented Mar 23, 2023

mkogan1 commented Mar 27, 2023

mkogan1 commented Apr 4, 2023

rgw: handle old clients with transfer-encoding: chunked. #50235

rgw: handle old clients with transfer-encoding: chunked. #50235

Conversation

mkogan1 commented Feb 23, 2023 • edited

Contribution Guidelines

Checklist

cbodley commented Feb 23, 2023

mkogan1 commented Mar 7, 2023

mkogan1 commented Mar 7, 2023

cbodley commented Mar 7, 2023

mkogan1 commented Mar 7, 2023

mkogan1 commented Mar 16, 2023

mattbenjamin left a comment

Choose a reason for hiding this comment

mkogan1 commented Mar 20, 2023

ivancich commented Mar 20, 2023

cbodley commented Mar 20, 2023

ivancich commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

mkogan1 commented Mar 21, 2023

cbodley commented Mar 21, 2023

mdw-at-linuxbox commented Mar 21, 2023

mdw-at-linuxbox left a comment

Choose a reason for hiding this comment

mkogan1 commented Mar 22, 2023

mkogan1 commented Mar 22, 2023

mkogan1 commented Mar 22, 2023

ivancich commented Mar 23, 2023

ivancich commented Mar 23, 2023

cbodley commented Mar 23, 2023

ivancich commented Mar 23, 2023

mkogan1 commented Mar 27, 2023

mkogan1 commented Apr 4, 2023

mkogan1 commented Feb 23, 2023 •

edited