Ceph » rgw

Properly formatted steps to reproduce (since I cannot edit the initial post):

❯ aws-cli/1.18.106 Python/3.8.10 Linux/5.13.0-44-generic botocore/1.17.29
❯ alias aws="aws --profile $profile --endpoint-url $endpoint" 
❯ aws s3api create-bucket --object-lock-enabled-for-bucket --bucket $bucket
❯ aws s3api put-object-lock-configuration --bucket $bucket --object-lock-configuration '{ "ObjectLockEnabled": "Enabled", "Rule": { "DefaultRetention": { "Mode": "COMPLIANCE", "Days": 1 }}}' # create bucket
❯ aws s3api get-object-lock-configuration --bucket $bucket # check bucket
{
    "ObjectLockConfiguration": {
        "ObjectLockEnabled": "Enabled",
        "Rule": {
            "DefaultRetention": {
                "Mode": "COMPLIANCE",
                "Days": 1
            }
        }
    }
}
❯ aws s3 cp /etc/services s3://$bucket # put example file into bucket
upload: ../../etc/services to s3://test-locking-bucket/services
❯ aws s3 ls s3://$bucket
2022-05-25 16:50:00      14464 services
❯ aws s3api list-object-versions --bucket $bucket
{
    "Versions": [
        {
            "ETag": "\"00060e37207f950bf0ebfd25810c19b9\"",
            "Size": 14464,
            "StorageClass": "STANDARD",
            "Key": "services",
            "VersionId": "TCNmYeOcJxcnaYH3b0f1h8vIj5.Y1MX",
            "IsLatest": true,
            "LastModified": "2022-05-25T14:50:00.538Z",
            "Owner": {
                "DisplayName": "chkugler",
                "ID": "chkugler" 
            }
        }
    ]
}
❯ aws s3 rm s3://$bucket/services
delete: s3://locking-test-bucket/services
❯ aws s3api list-object-versions --bucket $bucket
{
    "Versions": [
        {
            "ETag": "\"00060e37207f950bf0ebfd25810c19b9\"",
            "Size": 14464,
            "StorageClass": "STANDARD",
            "Key": "services",
            "VersionId": "TCNmYeOcJxcnaYH3b0f1h8vIj5.Y1MX",
            "IsLatest": false,
            "LastModified": "2022-05-25T14:50:00.538Z",
            "Owner": {
                "DisplayName": "chkugler",
                "ID": "chkugler" 
            }
        }
    ],
    "DeleteMarkers": [
        {
            "Owner": {
                "DisplayName": "chkugler",
                "ID": "chkugler" 
            },
            "Key": "services",
            "VersionId": "UGwXee--iZPQH3krIDR1xGi.d81p4YX",
            "IsLatest": true,
            "LastModified": "2022-05-25T14:53:37.988Z" 
        }
    ]
}
❯ aws s3api delete-object --bucket $bucket --key services --version-id='TCNmYeOcJxcnaYH3b0f1h8vIj5.Y1MX' # this must not work, but it does
{
    "VersionId": "TCNmYeOcJxcnaYH3b0f1h8vIj5.Y1MX" 
}
❯ aws s3api delete-object --bucket $bucket --key services --version-id='UGwXee--iZPQH3krIDR1xGi.d81p4YX' # this would restore the object
{
    "DeleteMarker": true,
    "VersionId": "UGwXee--iZPQH3krIDR1xGi.d81p4YX" 
}
❯ aws s3api list-object-versions --bucket $bucket # returns empty instead of having the object
❯ aws s3 ls s3://$bucket # returns empty instead of having the object

Just FYI:
the following patch (for RGWDeleteObj::execute method from src/rgw/rgw_op.cc):
- bool check_obj_lock = s->object->have_instance() && s->bucket->get_info().obj_lock_enabled();
+ bool check_obj_lock = s->bucket->get_info().obj_lock_enabled();

does the trick (to some degree) and prevents delete marker creation on a locked object.

But it still doesn't protect object version removal if delete marker is already present on an object (e.g. created before object lock policy application). Neither it prevents from delete marker removal.

@Casey, @Matt Li - do you think this makes sense?

Igor Fedotov wrote:

Just FYI:
the following patch (for RGWDeleteObj::execute method from src/rgw/rgw_op.cc):
- bool check_obj_lock = s->object->have_instance() && s->bucket->get_info().obj_lock_enabled();
+ bool check_obj_lock = s->bucket->get_info().obj_lock_enabled();

does the trick (to some degree) and prevents delete marker creation on a locked object.

i don't think that's correct. aws says in https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-managing.html#object-lock-managing-lifecycle:

Although you can't delete a protected object version, you can still create a delete marker for that object. Placing a delete marker on an object doesn't delete the object or its object versions.

Thanks, Casey. Yes, as noted in email, that is my understanding as well.

Matt

"What I think the spec requires is that a protected object version may not be deleted. This means real-deleted, not masked by a delete marker.

What would be incorrect would be a) if the delete action with version-id real-deleted the object at version-id; in fact, also b) if any other action (e.g., delete without version-id) caused the most recent version to be real-deleted."

Well, some more findings on the issue:
1) Apparently this issue is present in Pacific minor releases 16.2.6 through 16.2.9
Not sure about earlier ones but it definitely does not exist in Quincy and main branch.

2) After comparing the delete version operation's behavior between Quincy and Pacific I realized that RGWRados::get_obj_state_impl() function gets different object reference (rgw_obj& obj). In Pacific obj.key.get_oid() call result lacks proper object version specification (while it's present originally at RGWDeleteObj::execute()) and this finally causes ENOENT return from get_obj_state_impl().

More investigation reveals the following implementation of
int RGWRadosObject::get_obj_state(const DoutPrefixProvider *dpp, RGWObjectCtx *rctx, RGWBucket& bucket, RGWObjState **state, optional_yield y, {
rgw_obj obj(bucket.get_key(), key.name);

return store->getRados()->get_obj_state(dpp, rctx, bucket.get_info(), obj, state, follow_olh, y);
}

which is apparently a culprit as it makes an incomplete object reference which lacks version ref.
And indeed Quincy release implementation is different: {
return store->getRados()->get_obj_state(dpp, rctx, bucket.get_info(), get_obj(), state, follow_olh, y);
}

Updating to the latter implementation fixes the bug for me.
But I'm not completely sure this is 100% corrent and there are no side effects.

So @Matt Li, @Casey what do you think? Is it good enough solution for Pacific?

Hi Igor,

This looks promising to me, could you send a PR?

thanks sticking with this

Matt

Matt Benjamin wrote:

Hi Igor,

This looks promising to me, could you send a PR?

thanks sticking with this

Matt

Here it is: https://github.com/ceph/ceph/pull/47041

https://github.com/ceph/ceph/pull/47041 merged