Skip to content

Commit

Permalink
NC | NSFS | Versioning | Fix Bug | Return 405 for get/head specific d…
Browse files Browse the repository at this point in the history 
…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]>
  • Loading branch information
@shirady
shirady committed Sep 16, 2024
1 parent cb6beec commit 8492113
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 30 deletions.
1 change: 1 addition & 0 deletions src/endpoint/s3/s3_errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
46 changes: 20 additions & 26 deletions src/endpoint/s3/s3_rest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
}
}
}
Expand All @@ -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,
Expand All @@ -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 = `<html> \
<head><title>${s3err.http_code} ${s3err.code}</title></head> \
Expand Down
16 changes: 12 additions & 4 deletions src/sdk/namespace_fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions src/test/unit_tests/test_bucketspace_versioning.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
});
});
});

Expand Down

0 comments on commit 8492113

Please sign in to comment.