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

rgw: add support for FormPost of Swift API #11179

Merged
merged 26 commits into from May 4, 2017

Conversation

Projects
None yet
4 participants
@rzarzynski
Copy link
Contributor

rzarzynski commented Sep 21, 2016

Both tempest and s3-tests haven't found any regression here.

CC: @yehudasa, @cbodley

@dmick dmick added the needs-doc label Sep 22, 2016

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from 61d6dbd to 92a8469 Sep 26, 2016

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Sep 26, 2016

Rebased due to a merge conflict.

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from 92a8469 to 64d9e06 Nov 4, 2016

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Nov 4, 2016

@yehudasa, @mattbenjamin, @cbodley: rebased due to merge conflicts after getting in the compression stuff and front-end rework.

I will retest with Tempest and s3-tests.

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Nov 5, 2016

Retested. Tempest and s3-tests haven't found any new issue on this branch in comparison to master. However, the fresh master seems to have a regression related to Swift's object versioning (created bug #17803).

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

mattbenjamin commented Nov 28, 2016

@rzarzynski could you rebase? I'm seeing compression-related conflicts vs master. tx!

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from 64d9e06 to dbb0337 Nov 28, 2016

@mattbenjamin mattbenjamin self-assigned this Nov 29, 2016

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Dec 2, 2016

It looks the PR got new merge conflicts yesterday. Rebasing and testing.

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from dbb0337 to c9421a7 Dec 2, 2016

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Dec 2, 2016

@mattbenjamin: both s3-tests and Tempest haven't found any regression in comparison to the current master (3a9bcaa, Fri Dec 2 14:48:15 2016 +0800).

s3-tests found an unrelated issue in master (so no regression here) which causes a failure in following tests:

s3tests.functional.test_s3.test_versioning_multi_object_delete
s3tests.functional.test_s3.test_versioning_multi_object_delete_with_marker

I'm going to fill a ticket for that.

Tempest confirmed the FormPost is operational:

$ ./run_tempest.sh -V tempest.api.object_storage.test_object_formpost
...
{1} tempest.api.object_storage.test_object_formpost.ObjectFormPostTest.test_post_object_using_form [0.048283s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_expired [2.028040s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_invalid_signature [0.011914s] ... ok

======
Totals
======
Ran: 3 tests in 5.0000 sec.
 - Passed: 3
 - Skipped: 0
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 2.0882 sec.

==============
Worker Balance
==============
 - Worker 0 (2 tests) => 0:00:02.042187
 - Worker 1 (1 tests) => 0:00:00.048283

I appended a7fead4 which slightly modifies error handling in the situation when a form is expired or has an invalid signature. It looks that in the meanwhile Tempest became more restrictive. Previously both 401 Unauthorized and 403 Forbidden were acceptable. After upgrading to a newer version (51feb121085cf0a4d70af4b2b086f03991d5bceb, Wed Nov 16 10:13:27 2016 +0000) only the first one is seen as correct.

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Dec 2, 2016

@mattbenjamin: according to the Casey's comment the fix for the issue in master has been merged today.

@oritwas

This comment has been minimized.

Copy link
Contributor

oritwas commented Apr 3, 2017

@rzarzynski please rebase

@oritwas oritwas self-assigned this Apr 3, 2017

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from a7fead4 to fed83c2 Apr 6, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Apr 6, 2017

@oritwas, @mattbenjamin: rebased to master. Tempest nor s3-tests haven't found any regression here. In a dedicated run Tempest additionally confirmed the FormPost is operational:

$ ./run_tempest.sh -V tempest.api.object_storage.test_object_formpost
WARNING: This script is deprecated and will be removed in the near future. Please migrate to tempest run or another method of launching a test runner
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover} --list 
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover}  --load-list /tmp/tmpVXDbSs
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover}  --load-list /tmp/tmpDY58PT
{1} tempest.api.object_storage.test_object_formpost.ObjectFormPostTest.test_post_object_using_form [0.073423s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_expired [2.022728s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_invalid_signature [0.012430s] ... ok

======
Totals
======
Ran: 3 tests in 7.0000 sec.
 - Passed: 3
 - Skipped: 0
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 2.1086 sec.

==============
Worker Balance
==============
 - Worker 0 (2 tests) => 0:00:02.035737
 - Worker 1 (1 tests) => 0:00:00.073423

The updated branch contains the change requested by @oritwas in PR #11066.

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Apr 10, 2017

@oritwas: the branch was a subject of following Teuthology runs:

Finally we have 6 unrelated failures. All of them are because of the well-known Valgrind issue:
strange leak of std::string memory from md_config_t seen in radosgw.

@oritwas

This comment has been minimized.

Copy link
Contributor

oritwas commented Apr 23, 2017

@rzarzynski , needs another rebase

RGWAccessControlPolicy policy;
map<string, bufferlist> attrs;
boost::optional<ceph::real_time> delete_at;

/* Must be called after get_data() or the result is undefined. */
virtual std::string get_current_filename() const = 0;
virtual std::string get_current_contype() const = 0;

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

I prefer calling the method get_current_content_type

@@ -1023,9 +1025,9 @@ class RGWPostObj : public RGWOp {
void execute() override;

virtual int get_params() = 0;
virtual int get_data(bufferlist& bl) = 0;
virtual int get_data(ceph::bufferlist& bl, bool* again) = 0;

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

why not using a reference instead of a pointer for again?

* parses params in the format: 'first; param1=foo; param2=bar'
*/
void RGWPostObj_ObjStore::parse_boundary_params(
const std::string& params_str,

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

nit: formatting

*/
static int index_of(bufferlist& bl, int max_len, const string& str,
bool check_crlf,
bool *reached_boundary, int *skip)

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

I prefer reference for reached_boundary and skip otherwise you need to check they are not null

int RGWPostObj_ObjStore::read_with_boundary(ceph::bufferlist& bl,
uint64_t max,
const bool check_crlf,
bool* const reached_boundary,

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

reference ...

@@ -216,15 +216,78 @@ class RGWPutObj_ObjStore : public RGWPutObj
int get_padding_last_aws4_chunk_encoded(bufferlist &bl, uint64_t chunk_size);
};


This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

nit: empty line

ceph::bufferlist data;
};

using parts_coll_t = \

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

don't like this name parts_collection_t maybe?

std::string& first,
std::map<std::string, std::string>& params);

static bool part_str(/*const*/ parts_coll_t& parts,

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

const comment

const std::string& name,
std::string *val);

static std::string get_part_str(/*const*/ parts_coll_t& parts,

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

and here

const std::string& name,
const std::string& def_val = std::string());

static bool part_bl(/*const*/ parts_coll_t& parts,

This comment has been minimized.

Copy link
@oritwas

oritwas Apr 23, 2017

Contributor

and here

rzarzynski added some commits Sep 13, 2016

rgw: ignore fields placed after "file" in S3's browser uploads.
Fixes: http://tracker.ceph.com/issues/17273
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: RGWPostObj is able now to handle multiple files in single form.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: ONLY move the parts of RGWPostObj_ObjStore_S3 to RGWPostObj_ObjS…
…tore.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: accommodate the multipart-boundary parsing in RGWPostObj_ObjStore.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: make the RGWPostObj_ObjStore::post_form_part public due to rgw_c…
…rypt.cc.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add RGWPostObj_ObjStore::get_params() to encapsulate boundary ex…
…traction.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

rzarzynski added some commits Sep 18, 2016

rgw: clean-up the unnecessary RGWPostObj::boundary member.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: make parse_boundary_params() static method of RGWPostObj_ObjStore.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add an early, initial implementation of the Swift's FormPost mid…
…dleware.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add support for object prefixes in Swift's FormPost.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add support for form expiration in Swift's FormPost.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: strip the parts state from RGWPostObj_ObjStore.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: remove unused RGWPostObj_ObjStore::post_form_part::content_type.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add RGWPostObj_ObjStore::get_part_str method.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: implement form's signature verification in Swift's FormPost.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: refactor the expiration checking in FormPost of Swift API.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: add basic support for redirect in Swift's FormPost.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: restrict the scope of RGWPostObj::data_pending.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: enforce presence of at least one file to upload in RGWFormPost.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: RGWFormPost does support Swift's max_file_size parameter.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: Swift's FormPost does support per-file content types.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: improve debug printing in browser upload of S3 and Swift.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: fix error handling in RGWPostObj_ObjStore::read_with_boundary.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: FormPost returns 401 Unauthorized instead of 403 Forbidden.
This patch emerged because newer versions of Tempest became
more restrictive in the matter of FormPost's error handling.
Previously, Tempest was accepting both 403 Forbidden as well
as 401 Unauthorized as a response for signature mismatch or
expired form. Actually only the second one is acceptable for
tempest.api.object_storage.test_object_formpost_negative.*

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: Browser Upload's primitives take bool output params as references.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-swift-formpost branch from fed83c2 to 66a17f7 Apr 24, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Apr 24, 2017

@oritwas: thanks for the review! I've addressed your comments, rebased the branch and verified it locally. Tempest nor s3-tests (both in AWSv2 and AWSv4 mode) haven't found any regression here. Additionally the Tempest confirmed the FormPost is operational (a separated run):

$ ./run_tempest.sh -V tempest.api.object_storage.test_object_formpost
WARNING: This script is deprecated and will be removed in the near future. Please migrate to tempest run or another method of launching a test runner
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover} --list 
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover}  --load-list /tmp/tmpHoSEVb
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover}  --load-list /tmp/tmpE2P2gE
{1} tempest.api.object_storage.test_object_formpost.ObjectFormPostTest.test_post_object_using_form [0.045707s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_expired [2.024572s] ... ok
{0} tempest.api.object_storage.test_object_formpost_negative.ObjectFormPostNegativeTest.test_post_object_using_form_invalid_signature [0.010969s] ... ok

======
Totals
======
Ran: 3 tests in 5.0000 sec.
 - Passed: 3
 - Skipped: 0
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 2.0812 sec.

==============
Worker Balance
==============
 - Worker 0 (2 tests) => 0:00:02.035966
 - Worker 1 (1 tests) => 0:00:00.045707
void RGWPostObj_ObjStore::parse_boundary_params(const std::string& params_str,
std::string& first,
std::map<std::string,
std::string>& params)

This comment has been minimized.

Copy link
@oritwas

oritwas May 4, 2017

Contributor

nit: too much spacing

@oritwas

oritwas approved these changes May 4, 2017

Update rgw_rest.cc
fix indentation.
Signed-off-by: Orit Wasserman <owasserm@redhat.com>

@oritwas oritwas merged commit 144dc78 into ceph:master May 4, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@rzarzynski rzarzynski deleted the rzarzynski:wip-rgw-swift-formpost branch May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.