Skip to content

NC | NSFS | Versioning | Fix Bug | Return 405 for Get/Head Specific Delete-Marker #8338

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

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Sep 10, 2024

Explain the changes

  1. In namespace_fs add the case of specific version to the error that is thrown with additional information that we will use to set headers in the http response. To support it I added the params argument to the function _throw_if_delete_marker.
  2. In s3_error add the mapping between the rpc that we used in namespace_fs and the S3 error that we want it to be mapped.
  3. In s3_rest change the s3err.rpc_data to err.rpc_data since the object s3error does not have the rpc_data as a property inside it. Add the case to set http header for the delete-marker, reuse this change in a refactored function _prepare_error.

Issues:

  1. 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.

Currently, we returned:

  • Head specific delete-marker: "An error occurred (404) when calling the HeadObject operation: Not Found" without additional headers in the response.
  • Get specific delete-marker: "An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist."

List of gaps:

  1. The s3err.rpc_data also appears in the function _handle_html_response, need to decide if we want to change it there as well.
  2. For better debugging of headers in the response I suggest adding the printings of all the headers (probably a util function that we need to create).
  3. We might need to see if there are more cases in which we need to set the header of x-amz-delete-marker when versioning is enabled.
  4. By changing the header I thought that it might pass the Ceph S3 tests (in containerized) s3tests_boto3/functional/test_s3.py::test_get_object_ifnonematch_good and s3tests_boto3/functional/test_s3.py::test_get_object_ifmodifiedsince_failed - but they did not pass - need to investigate more.
  5. We might have this issue in a containerized environment, I opened issue Containerized | Versioning | Return 405 for Get/Head Specific Delete-Marker #8369 .

Testing Instructions:

Unit Test

Please run: sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha ./src/test/unit_tests/test_bucketspace_versioning.js

Manual Tests

  1. Create an account with the CLI: sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /tmp/nsfs_root1 --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid>
    Note: before creating the account need to give permission to the new_buckets_path: chmod 777 /tmp/nsfs_root1, chmod 777 /tmp/nsfs_root2.
  2. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5
    Notes:
  • I Change the config.NSFS_CHECK_BUCKET_BOUNDARIES = false; //SDSD because I’m using the /tmp/ and not /private/tmp/.
  1. Create the alias for S3 service:alias nc-user-1-s3=‘AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443’.
  2. Check the connection to the endpoint and try to list the buckets (should be empty): nc-user-1-s3 s3 ls; echo $?
  3. Add bucket to the account using AWS CLI: nc-user-1-s3 s3 mb s3://bucket-v (bucket-v is the bucket name in this example)
  4. Enable versioning: nc-user-1-s3 s3api put-bucket-versioning --bucket bucket-v --versioning-configuration Status=Enabled
  5. Put an object: nc-user-1-s3 s3api put-object --bucket bucket-v --key hello.txt
  6. Delete the object: nc-user-1-s3 s3api delete-object --bucket bucket-v --key hello.txt (save the VersionId from the output).
  7. Head specific delete-marker: nc-user-1-s3 s3api get-object --bucket bucket-v --key hello.txt --version-id mtime-d41oghwhfyf4-ino-2f74i2 --debug (should be An error occurred (405) when calling the HeadObject operation: Method Not Allowed use the debug flag to validate the headers of Last-Modified and x-amz-delete-marker).
  8. Get specific delete-marker: touch my_get.txt; nc-user-1-s3 s3api get-object --bucket bucket-v --key hello.txt --version-id mtime-d41oghwhfyf4-ino-2f74i2 my_get.txt --debug (should be An error occurred (MethodNotAllowed) when calling the GetObject operation: The specified method is not allowed against this resource. use the debug flag to validate the headers of Last-Modified and x-amz-delete-marker).
  • Doc added/updated
  • Tests added

@shirady shirady force-pushed the nsfs-nc-versioning-head-get-version-id-delete-marker branch from bd259fc to 03b57bb Compare September 10, 2024 06:24
@shirady shirady self-assigned this Sep 10, 2024
@shirady shirady force-pushed the nsfs-nc-versioning-head-get-version-id-delete-marker branch 2 times, most recently from 379b8a3 to 81439e0 Compare September 12, 2024 05:15
@shirady shirady changed the title NC | NSFS | Versioning | Fix Bug | Return 405 for Get/head Specific Delete-Marker NC | NSFS | Versioning | Fix Bug | Return 405 for Get/Head Specific Delete-Marker Sep 12, 2024
Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

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

LGTM

…elete-marker

1. In namespace_fs add the case of specific version to the error that is thrown with additional information that we will use to set headers in the http response. To support it I added the params argument to the function _throw_if_delete_marker.
2. In s3_error add the mapping between the rpc that we used in namespace_fs and the S3 error that we want it to be mapped.
3. In s3_rest change the s3err.rpc_data to err.rpc_data since the object s3error does not have the rpc_data as a property inside it. Add the case to set http header for the delete-marker, reuse this change in a refactored function _prepare_error.

Signed-off-by: shirady <[email protected]>
@shirady shirady force-pushed the nsfs-nc-versioning-head-get-version-id-delete-marker branch from 6cea101 to 8492113 Compare September 16, 2024 15:28
@shirady shirady merged commit 87e28c9 into noobaa:master Sep 17, 2024
10 checks passed
@shirady shirady deleted the nsfs-nc-versioning-head-get-version-id-delete-marker branch September 17, 2024 05:06
shirady added a commit to shirady/noobaa-core that referenced this pull request Sep 25, 2024
1. In test_bucketspace_versioning.js in the object tagging tests
  - Add Header check related to HeadObject with tagging, as was manually tested in PR NC | NSFS | Fix Bug | Head Object on a Tagged Object Does Not Return x-amz-tagging-count Header noobaa#8357.
  - Change the bucket from suspended_bucket_name to bucket_name (the bucket that we use with versioning Enabled).
2. In test_bucketspace_versioning.js add headers check, as was manually tested in PR NC | NSFS | Versioning | Fix Bug | Return 405 for Get/Head Specific Delete-Marker noobaa#8338 in the following tests:
  - head object, with version enabled, version id specified delete marker - should throw error with code 405
  - get object, with version enabled, version id specified delete marker - should throw error with code 405

Signed-off-by: shirady <[email protected]>
romayalon pushed a commit to romayalon/noobaa-core that referenced this pull request Sep 26, 2024
1. In test_bucketspace_versioning.js in the object tagging tests
  - Add Header check related to HeadObject with tagging, as was manually tested in PR NC | NSFS | Fix Bug | Head Object on a Tagged Object Does Not Return x-amz-tagging-count Header noobaa#8357.
  - Change the bucket from suspended_bucket_name to bucket_name (the bucket that we use with versioning Enabled).
2. In test_bucketspace_versioning.js add headers check, as was manually tested in PR NC | NSFS | Versioning | Fix Bug | Return 405 for Get/Head Specific Delete-Marker noobaa#8338 in the following tests:
  - head object, with version enabled, version id specified delete marker - should throw error with code 405
  - get object, with version enabled, version id specified delete marker - should throw error with code 405

Signed-off-by: shirady <[email protected]>
(cherry picked from commit 229a01e)
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.

2 participants