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 lifecycle validation according to S3. #12750

Merged
merged 3 commits into from Jan 18, 2017

Conversation

Projects
None yet
2 participants
@zhangsw
Copy link
Contributor

zhangsw commented Jan 3, 2017

According to S3, every S3 lifecycle rule has some restrictions and shouldn't be conflicted with other lifecycle rule.

Fixes: http://tracker.ceph.com/issues/18394

Signed-off-by: Zhang Shaowen zhangshaowen@cmss.chinamobile.com

rgw: add lifecycle validation according to S3.
Fixes: http://tracker.ceph.com/issues/18394

Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Jan 10, 2017

@cbodley @yehudasa Please help to review this pr, thx~

@dang
Copy link
Contributor

dang left a comment

A few comments. Also, if we're going to enforce these rules, we need tests to verify the enforcement.


//Rules are conflicted: if one rule's prefix starts with other rule's prefix, and these two rules
//define same action(now only support expiration days).
bool RGWLifecycleConfiguration::validate()

This comment has been minimized.

Copy link
@dang

dang Jan 10, 2017

Contributor

This isn't a full comparison of every entry to every other entry; it only compares adjacent entries. I assume this isn't what you want. If you want a full comparison of every entry with every other entry, you will need nested loops.

This comment has been minimized.

Copy link
@zhangsw

zhangsw Jan 11, 2017

Author Contributor

Yes, I just compare adjacent entries. The prefix_map's type is 'map<string, int>', so the elements in it are sorted by the rule's prefix. Assumes that "a"<A<"abc", prefix "abc" begins with prefix "a", then A must start with "a". So we can just compare adjacent entries. At present, if there are two rules like above, we can say lifecycle config is illegal because of only expire day is supported. When we have other action, we need to compare three rules like above(or four and so on), but not a full comparison. What do you think about this?

This comment has been minimized.

Copy link
@dang

dang Jan 11, 2017

Contributor

Okay, I see what you're doing here. I think it should work. I don't think you need the length check, necessarily, since a string that is a strict initial substring of another will always sort first in C++, but this should still work.

This comment has been minimized.

Copy link
@zhangsw

zhangsw Jan 12, 2017

Author Contributor

Yeah, I remove the length check and one string comparison.

@@ -43,6 +55,49 @@ void RGWLifecycleConfiguration::_add_rule(LCRule *rule)
prefix_map[rule->get_prefix()] = rule->get_expiration().get_days();
}

bool RGWLifecycleConfiguration::check_and_add_rule(LCRule *rule)

This comment has been minimized.

Copy link
@dang

dang Jan 10, 2017

Contributor

Since add_rule() is unused now, could you just put this code in add_rule()? That way, no one will accidentally call the wrong one.

This comment has been minimized.

Copy link
@zhangsw

zhangsw Jan 11, 2017

Author Contributor

The add_rule() is used in parsing the xml (RGWLifecycleConfiguration_S3::xml_end). If we change it, we will check the rule twice.

This comment has been minimized.

Copy link
@dang

dang Jan 11, 2017

Contributor

Okay, missed that. This is fine.

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Jan 11, 2017

@dang I can't find a file contains unit test about lifecycle. Any advice about where should I place the tests?

@dang

This comment has been minimized.

Copy link
Contributor

dang commented Jan 11, 2017

Lifecycle testing is in s3-tests here: https://github.com/ceph/s3-tests

rgw: remove useless length comparison in lifecycle validate.
Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>

@zhangsw zhangsw force-pushed the zhangsw:feature-rgw-lc-validatei branch from 83e2a2f to a142de3 Jan 12, 2017

@dang

This comment has been minimized.

Copy link
Contributor

dang commented Jan 12, 2017

This looks good now. Are you able to work on tests?

@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Jan 13, 2017

@dang yeah, I will work on the tests.

rgw: modify error code in lifecycle to be compatible with S3.
Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
@zhangsw

This comment has been minimized.

Copy link
Contributor Author

zhangsw commented Jan 17, 2017

@dang I add the test cases in ceph/s3-tests#138
and I modify some code to be compatible with S3's error code.

@dang

dang approved these changes Jan 18, 2017

@dang dang merged commit 963dd12 into ceph:master Jan 18, 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

@zhangsw zhangsw deleted the zhangsw:feature-rgw-lc-validatei branch Jan 19, 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.