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

Return 405 for Get/Head Specific Delete-Marker #8733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vh05
Copy link
Contributor

@vh05 vh05 commented Jan 29, 2025

If the specified version in the request is a delete marker the returned is 405 Method Not Allowed.

  1. Create a versioned bucket and multiple versions of the object by copying the same object multiple times
  # nb-s3 mb s3://test-16
  # nb-s3 cp Makefile s3://test-16
  1. delete the object
    nb-s3api delete-object --bucket test-16 --key Makefile

  2. Now head/get object with version id of the delete marker (list-object-versions will give the delete version marker id)

# nb-s3api head-object --bucket test-16 --key Makefile --version-id nbver-9
An error occurred (405) when calling the HeadObject operation: Method Not Allowed

Output:

Fixes: #8369

@vh05 vh05 requested a review from shirady January 29, 2025 06:47
@shirady
Copy link
Contributor

shirady commented Jan 29, 2025

@vh05,
Could you attach the manual testing instructions you performed and the screenshot of the result? (to help us to see the result)
Do you plan to add to this PR automatic tests?
If you need I added test cases in NSFS in PR #8338 and #8402

@@ -1568,7 +1568,13 @@ function get_obj_id(req, rpc_code) {
* @param {string} rpc_code
*/
function check_object_mode(req, obj, rpc_code) {
if (!obj || obj.deleted || obj.delete_marker) {
if (obj && obj.delete_marker) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we be sure that in this case the object is a delete_marker and also the version ID was specified in the request? From reading this condition I only know that it is a delete marker.

Copy link
Contributor Author

@vh05 vh05 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bucket with version enabled and the object is deleted, we can see the delete marker object as true. In other case the delete marker is false or obj it self is null.

Cases:

  1. If we delete the object (with version enabled), head the object with/out version_id, we get Method not allowed
  2. If we try to fetch any other version that is deleted, we get Not Found as Obj is NULL
  3. If version not enabled, we return Not Found, since obj is NULL

Correct me if wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the request?
The difference here that it was on delete marker with version id.

According to the AWS docs GetObject and HeadObject:(https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html#API_GetObject_ResponseElements):
If the specified version in the request is a delete marker, the response returns a 405 Method Not Allowed error and the Last-Modified: timestamp response header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the behavior what I saw on aws s3
head object

# aws s3api head-object --bucket vh-22 --key Makefile --version-id aqGt6sJfDntex6GPXfpl2RZwa5fUQYgV

An error occurred (405) when calling the HeadObject operation: Method Not Allowed

get-object

# aws s3api get-object --bucket vh-22 --key Makefile --version-id aqGt6sJfDntex6GPXfpl2RZwa5fUQYgV a

An error occurred (MethodNotAllowed) when calling the GetObject operation: The specified method is not allowed against this resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now I got what you are saying,

If we dont specify version id on delete marker, we should return ENOENT and version id is given we should return Method Not allowed. Am I right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be compliant with the S3 protocol.
So in the previous check that you did on AWS you can also do it without "--version-id" flag and look at the errors.
Then you need to test the same cases on noobaa (on the a data bucket) and ensure the behavior is the same.
ENOENT is the term in filesystem, we need to return the right RPC error (that would be translated to the S3 error we need to return).
I hope it is clear.

@vh05
Copy link
Contributor Author

vh05 commented Jan 30, 2025

@vh05, Could you attach the manual testing instructions you performed and the screenshot of the result? (to help us to see the result) Do you plan to add to this PR automatic tests? If you need I added test cases in NSFS in PR #8338 and #8402

@vh05, Could you attach the manual testing instructions you performed and the screenshot of the result? (to help us to see the result) Do you plan to add to this PR automatic tests? If you need I added test cases in NSFS in PR #8338 and #8402

Yes, will add test

If the specified version in the request is a delete marker
the returned is 405 Method Not Allowed.

Fixes: noobaa#8369

Signed-off-by: Vinayakswami Hariharmath <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Containerized | Versioning | Return 405 for Get/Head Specific Delete-Marker
2 participants