From 849211381c9e388357644cea5647bd7954cbaa58 Mon Sep 17 00:00:00 2001 From: shirady <57721533+shirady@users.noreply.github.com> Date: Tue, 10 Sep 2024 07:54:22 +0300 Subject: [PATCH] NC | NSFS | Versioning | Fix Bug | Return 405 for get/head specific delete-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 <57721533+shirady@users.noreply.github.com> --- src/endpoint/s3/s3_errors.js | 1 + src/endpoint/s3/s3_rest.js | 46 ++++++++----------- src/sdk/namespace_fs.js | 16 +++++-- .../unit_tests/test_bucketspace_versioning.js | 27 +++++++++++ 4 files changed, 60 insertions(+), 30 deletions(-) diff --git a/src/endpoint/s3/s3_errors.js b/src/endpoint/s3/s3_errors.js index 82446ab2fe..6a31359a98 100644 --- a/src/endpoint/s3/s3_errors.js +++ b/src/endpoint/s3/s3_errors.js @@ -609,6 +609,7 @@ S3Error.RPC_ERRORS_TO_S3 = Object.freeze({ NO_SUCH_TAG: S3Error.NoSuchTagSet, INVALID_ENCODING_TYPE: S3Error.InvalidEncodingType, INVALID_TARGET_BUCKET: S3Error.InvalidTargetBucketForLogging, + METHOD_NOT_ALLOWED: S3Error.MethodNotAllowed, }); exports.S3Error = S3Error; diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index e07d3fb50d..6189546aca 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/src/endpoint/s3/s3_rest.js @@ -397,7 +397,7 @@ function parse_op_name(req) { return `${method}_object`; } -function handle_error(req, res, err) { +function _prepare_error(req, res, err) { let s3err = ((err instanceof S3Error) && err) || new S3Error(S3Error.RPC_ERRORS_TO_S3[err.rpc_code] || S3Error.InternalError); @@ -407,19 +407,26 @@ function handle_error(req, res, err) { s3err.detail = err.rpc_data.detail; } - if (s3err.rpc_data) { - if (s3err.rpc_data.etag) { + if (err.rpc_data) { + if (err.rpc_data.etag) { if (res.headersSent) { dbg.log0('Sent reply in body, bit too late for Etag header'); } else { - res.setHeader('ETag', s3err.rpc_data.etag); + res.setHeader('ETag', err.rpc_data.etag); } } - if (s3err.rpc_data.last_modified) { + if (err.rpc_data.last_modified) { if (res.headersSent) { dbg.log0('Sent reply in body, bit too late for Last-Modified header'); } else { - res.setHeader('Last-Modified', time_utils.format_http_header_date(new Date(s3err.rpc_data.last_modified))); + res.setHeader('Last-Modified', time_utils.format_http_header_date(new Date(err.rpc_data.last_modified))); + } + } + if (err.rpc_data.delete_marker) { + if (res.headersSent) { + dbg.log0('Sent reply in body, bit too late for x-amz-delete-marker header'); + } else { + res.setHeader('x-amz-delete-marker', String(err.rpc_data.delete_marker)); } } } @@ -432,6 +439,12 @@ function handle_error(req, res, err) { usage_report.s3_errors_info.total_errors += 1; usage_report.s3_errors_info[s3err.code] = (usage_report.s3_errors_info[s3err.code] || 0) + 1; + return s3err; +} + +function handle_error(req, res, err) { + const s3err = _prepare_error(req, res, err); + const reply = s3err.reply(req.originalUrl, req.request_id); dbg.error('S3 ERROR', reply, req.method, req.originalUrl, @@ -450,26 +463,7 @@ function handle_error(req, res, err) { } async function _handle_html_response(req, res, err) { - let s3err = - ((err instanceof S3Error) && err) || - new S3Error(S3Error.RPC_ERRORS_TO_S3[err.rpc_code] || S3Error.InternalError); - - if (s3err.rpc_data) { - if (s3err.rpc_data.etag) { - res.setHeader('ETag', s3err.rpc_data.etag); - } - if (s3err.rpc_data.last_modified) { - res.setHeader('Last-Modified', time_utils.format_http_header_date(new Date(s3err.rpc_data.last_modified))); - } - } - - // md_conditions used for PUT/POST/DELETE should return PreconditionFailed instead of NotModified - if (s3err.code === 'NotModified' && req.method !== 'HEAD' && req.method !== 'GET') { - s3err = new S3Error(S3Error.PreconditionFailed); - } - - usage_report.s3_errors_info.total_errors += 1; - usage_report.s3_errors_info[s3err.code] = (usage_report.s3_errors_info[s3err.code] || 0) + 1; + const s3err = _prepare_error(req, res, err); const reply = ` \ ${s3err.http_code} ${s3err.code} \ diff --git a/src/sdk/namespace_fs.js b/src/sdk/namespace_fs.js index f876307cce..6dbabd7d20 100644 --- a/src/sdk/namespace_fs.js +++ b/src/sdk/namespace_fs.js @@ -943,7 +943,7 @@ class NamespaceFS { stat = { ...dir_content_path_stat, xattr }; } } - this._throw_if_delete_marker(stat); + this._throw_if_delete_marker(stat, params); return this._get_object_info(params.bucket, params.key, stat, isDir); } catch (err) { if (this._should_update_issues_report(params, file_path, err)) { @@ -989,7 +989,7 @@ class NamespaceFS { ); const stat = await file.stat(fs_context); - this._throw_if_delete_marker(stat); + this._throw_if_delete_marker(stat, params); // await this._fail_if_archived_or_sparse_file(fs_context, file_path, stat); const start = Number(params.start) || 0; @@ -2714,11 +2714,19 @@ class NamespaceFS { return versioned_path; } - _throw_if_delete_marker(stat) { + _throw_if_delete_marker(stat, params) { if (this.versioning === versioning_status_enum.VER_ENABLED || this.versioning === versioning_status_enum.VER_SUSPENDED) { const xattr_delete_marker = stat.xattr[XATTR_DELETE_MARKER]; if (xattr_delete_marker) { - throw error_utils.new_error_code('ENOENT', 'Entry is a delete marker'); + const basic_err = error_utils.new_error_code('ENOENT', 'Entry is a delete marker'); + if (params.version_id) { + // 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. + throw new RpcError('METHOD_NOT_ALLOWED', + 'Method not allowed, delete object id of entry delete marker', + { last_modified: new Date(stat.mtime), delete_marker: true }); + } + throw basic_err; } } } diff --git a/src/test/unit_tests/test_bucketspace_versioning.js b/src/test/unit_tests/test_bucketspace_versioning.js index 345f10cd05..a57eeae2b0 100644 --- a/src/test/unit_tests/test_bucketspace_versioning.js +++ b/src/test/unit_tests/test_bucketspace_versioning.js @@ -2664,6 +2664,33 @@ mocha.describe('bucketspace namespace_fs - versioning', function() { assert.fail(`Failed with an error: ${err.Code}`); } }); + + mocha.it('head object, with version enabled, version id specified delete marker - should throw error with code 405', async function() { + try { + await s3_client.headObject({Bucket: bucket_name, Key: en_version_key, VersionId: versionID_1}); + assert.fail('Should fail'); + } catch (err) { + assert.strictEqual(err.$metadata.httpStatusCode, 405); + // In headObject the AWS SDK doesn't return the err.Code + // In AWS CLI it looks: + // An error occurred (405) when calling the HeadObject operation: Method Not Allowed + // in the docs: https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html + // if the HEAD request generates an error, it returns a generic code, such as ... + // 405 Method Not Allowed, ... It's not possible to retrieve the exact exception of these error codes. + } + }); + + mocha.it('get object, with version enabled, version id specified delete marker - should throw error with code 405', async function() { + try { + await s3_client.getObject({Bucket: bucket_name, Key: en_version_key, VersionId: versionID_1}); + assert.fail('Should fail'); + } catch (err) { + assert.strictEqual(err.$metadata.httpStatusCode, 405); + assert.strictEqual(err.Code, 'MethodNotAllowed'); + // In AWS CLI it looks: + // An error occurred (MethodNotAllowed) when calling the GetObject operation: The specified method is not allowed against this resource. + } + }); }); });