rgw: check object locks in multi-object delete #37933
Conversation
|
Tests for this: ceph/s3-tests#365 |
src/rgw/rgw_op.cc
Outdated
| } | ||
| } | ||
| } | ||
| int object_lock_response = verify_object_lock(this, obj->get_attrs(), false, false); |
There was a problem hiding this comment.
if this function is really going to take two booleans as arguments, maybe /* */ comment them where it's called
There was a problem hiding this comment.
This was changed in a later commit.
src/rgw/rgw_op.cc
Outdated
| @@ -6392,9 +6397,12 @@ int RGWDeleteMultiObj::verify_permission() | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| bool empty = rgw::sal::RGWObject::empty(s->object.get()) || s->object->get_instance().empty(); | |||
There was a problem hiding this comment.
@dang is this typical usage?
There was a problem hiding this comment.
I don't think this is right. It's technically valid, since empty() checks that the name is empty but not the instance, but it should be valid to have an un-instanced object, and this check will consider that to be an empty object.
If this check is as intended, can we change the name of the variable from "empty" to something more descriptive, since an object with a name but no instance is definitely not empty.
There was a problem hiding this comment.
The thing I'm trying to solve here is that while testing, I found that for multi-object delete, s->object.get() was returning null. The other side of the || is just what was there already, so I assume it has a purpose but I'm not very familiar with this code. #36583 has a very similar change, but calls it is_versioned. Would that be more appropriate? (I generally don't like putting in changes that are already in a separate PR, but I needed this part of the fix to test that the core change was working).
There was a problem hiding this comment.
Okay, now I'm understanding. Let's not call the variable "empty". "is_versioned" would work, but you'll have to negate it. Maybe "not_versioned" would be simplest?
There was a problem hiding this comment.
(ah, getting it now); yes, naming this var not_versioned or similar would assist readability
There was a problem hiding this comment.
Thanks. Changed to not_versioned.
There was a problem hiding this comment.
looks good, includes fix for https://tracker.ceph.com/issues/46567 / pr #36583 as well
|
Needs a rebase |
Multi-object delete (via the S3 API) will now check each object's retention date in the same way as single object delet does. Fixes: http://tracker.ceph.com/issues/47586 Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
Allow governance retention to be overridden by a suitably privileged user. Fixes: http://tracker.ceph.com/issues/47586 Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
…object delete. fixes: https://tracker.ceph.com/issues/47586 Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
markhoughton-microfocus
force-pushed
the
issue_47586_fix
branch
from
2d4fc91
to
5d22b7d
Compare
Signed-off-by: Mark Houghton <mhoughton@microfocus.com>
|
|
jenkins test make check |
|
Any idea why it's failing? I haven't got it to run locally yet and the Jenkins logs don't reveal much. |
|
@markhoughton-microfocus I don't believe a failure in seastar messenger could be traced to this PR |
|
@markhoughton-microfocus Some of the "make check" tests suffer from transient failures. I triggered a re-run, and now I see the tests all succeeded. |
|
@smithfarm thank you. In that case, from my side this is done and ready to be re-reviewed following the rebase (a very small change to pass on the new yield parameter). |
|
What are the next steps to get this merged? |
|
@markhoughton-microfocus There needs to be a successful teuthology run. I think that's the only obstacle to merging this. Sorry for the delay. |
|
Teuthology runs: |
Checklist
Add checks on object lock status before allowing an object to be deleted by the S3 multi-object delete API.
Also fixes some issues with verifying permissions when a bucket policy is in place. This part includes a fix which overlaps with #36583 (PR in review at the time of writing).
Fixes https://tracker.ceph.com/issues/47586