Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions config.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,14 @@ config.STS_CORS_EXPOSE_HEADERS = 'ETag';

config.DENY_UPLOAD_TO_STORAGE_CLASS_STANDARD = false;

/**
* NSFS_GLACIER_FORCE_STORAGE_CLASS when set to true
* will force `GLACIER` storage class if no storage class
* is provided and if `STANDARD` storage class is provided
* @type {boolean}
*/
config.NSFS_GLACIER_FORCE_STORAGE_CLASS = false;

// S3_RESTORE_MAX_DAYS controls that for how many maximum number
// of days an object can be restored using `restore-object` call.
config.S3_RESTORE_REQUEST_MAX_DAYS = 30;
Expand Down
3 changes: 3 additions & 0 deletions src/endpoint/s3/s3_rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ async function handle_request(req, res) {
}
http_utils.check_headers(req, headers_options);

// Will override the storage class if configured
s3_utils.override_storage_class(req);

Comment on lines +110 to +112
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Do not mutate headers before SigV4 auth; move override after authenticate_request()

Changing x-amz-storage-class pre-auth can invalidate AWS SigV4 when that header is signed. Apply override after authenticate_request(req) and before authorize_request(req); also limit to relevant ops.

Apply this diff:

-    // Will override the storage class if configured
-    s3_utils.override_storage_class(req);
+    // (moved below) override storage class only after signature auth

Place this block immediately after authenticate_request(req);:

+    // Safe to mutate after signature auth and before policy checks.
+    // Limit to ops that accept x-amz-storage-class (PUT object, MPU create).
+    if (req.op_name === 'put_object' || req.op_name === 'post_object_uploads') {
+        s3_utils.override_storage_class(req);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Will override the storage class if configured
s3_utils.override_storage_class(req);
// (moved below) override storage class only after signature auth
Suggested change
// Will override the storage class if configured
s3_utils.override_storage_class(req);
// Safe to mutate after signature auth and before policy checks.
// Limit to ops that accept x-amz-storage-class (PUT object, MPU create).
if (req.op_name === 'put_object' || req.op_name === 'post_object_uploads') {
s3_utils.override_storage_class(req);
}
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_rest.js around lines 110-112, the code mutates x-amz-*
headers before SigV4 authentication by calling
s3_utils.override_storage_class(req); move this call so it runs immediately
after authenticate_request(req) and before authorize_request(req) to avoid
invalidating signed headers, and restrict the override to only the S3 operations
that accept a storage-class header (e.g., PUT object/ multipart upload
completes) rather than running for all requests.

const redirect = await populate_request_additional_info_or_redirect(req);
if (redirect) {
res.setHeader('Location', redirect);
Expand Down
14 changes: 14 additions & 0 deletions src/endpoint/s3/s3_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const STORAGE_CLASS_STANDARD = 'STANDARD';
const STORAGE_CLASS_GLACIER = 'GLACIER'; // "S3 Glacier Flexible Retrieval"
/** @type {nb.StorageClass} */
const STORAGE_CLASS_GLACIER_IR = 'GLACIER_IR'; // "S3 Glacier Instant Retrieval"
/** @type {nb.StorageClass} */
const STORAGE_CLASS_GLACIER_DA = 'GLACIER_DA'; // "DBS3 specific Storage Class"

const DEFAULT_S3_USER = Object.freeze({
ID: '123',
Expand Down Expand Up @@ -382,6 +384,7 @@ function parse_storage_class(storage_class) {
if (!storage_class) return STORAGE_CLASS_STANDARD;
if (storage_class === STORAGE_CLASS_STANDARD) return STORAGE_CLASS_STANDARD;
if (storage_class === STORAGE_CLASS_GLACIER) return STORAGE_CLASS_GLACIER;
if (storage_class === STORAGE_CLASS_GLACIER_DA) return STORAGE_CLASS_GLACIER_DA;
if (storage_class === STORAGE_CLASS_GLACIER_IR) return STORAGE_CLASS_GLACIER_IR;
throw new Error(`No such s3 storage class ${storage_class}`);
}
Expand Down Expand Up @@ -819,9 +822,19 @@ function parse_body_public_access_block(req) {
return parsed;
}

function override_storage_class(req) {
if (
config.NSFS_GLACIER_FORCE_STORAGE_CLASS &&
parse_storage_class_header(req) === STORAGE_CLASS_STANDARD
) {
req.headers['x-amz-storage-class'] = STORAGE_CLASS_GLACIER;
}
}
Comment on lines +825 to +832
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add JSDoc documentation for side effects and behavior.

The override_storage_class function mutates the request object, which is a side effect that should be documented. Additionally, the function lacks documentation explaining its purpose and when it should be called.

Apply this diff to add JSDoc documentation:

+/**
+ * override_storage_class rewrites the storage class header to GLACIER when
+ * NSFS_GLACIER_FORCE_STORAGE_CLASS is enabled and the incoming storage class
+ * is STANDARD (or not specified, which defaults to STANDARD).
+ * 
+ * This function mutates req.headers as a side effect.
+ * 
+ * @param {nb.S3Request} req - The S3 request object
+ */
 function override_storage_class(req) {
     if (
         config.NSFS_GLACIER_FORCE_STORAGE_CLASS &&
         parse_storage_class_header(req) === STORAGE_CLASS_STANDARD
     ) {
         req.headers['x-amz-storage-class'] = STORAGE_CLASS_GLACIER;
     }
 }
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_utils.js around lines 825 to 832, the function
override_storage_class lacks JSDoc explaining its purpose and side effects: add
a JSDoc block immediately above the function that describes what the function
does (overrides x-amz-storage-class to GLACIER when
NSFS_GLACIER_FORCE_STORAGE_CLASS is set and the parsed header equals STANDARD),
documents the parameter (req: incoming request object with headers), notes that
it mutates req.headers (side effect), states there is no return value, and
indicates when it should be called (e.g., before request is forwarded/sent).
Ensure the doc mentions edge cases (only runs when config flag true) and
optional behavior for clarity.


exports.STORAGE_CLASS_STANDARD = STORAGE_CLASS_STANDARD;
exports.STORAGE_CLASS_GLACIER = STORAGE_CLASS_GLACIER;
exports.STORAGE_CLASS_GLACIER_IR = STORAGE_CLASS_GLACIER_IR;
exports.STORAGE_CLASS_GLACIER_DA = STORAGE_CLASS_GLACIER_DA;
exports.DEFAULT_S3_USER = DEFAULT_S3_USER;
exports.DEFAULT_OBJECT_ACL = DEFAULT_OBJECT_ACL;
exports.decode_chunked_upload = decode_chunked_upload;
Expand Down Expand Up @@ -863,5 +876,6 @@ exports.key_marker_to_cont_tok = key_marker_to_cont_tok;
exports.parse_sse_c = parse_sse_c;
exports.verify_string_byte_length = verify_string_byte_length;
exports.parse_body_public_access_block = parse_body_public_access_block;
exports.override_storage_class = override_storage_class;
exports.OBJECT_ATTRIBUTES = OBJECT_ATTRIBUTES;
exports.OBJECT_ATTRIBUTES_UNSUPPORTED = OBJECT_ATTRIBUTES_UNSUPPORTED;
10 changes: 7 additions & 3 deletions src/sdk/namespace_fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -3612,11 +3612,15 @@ class NamespaceFS {
}

async _is_storage_class_supported(storage_class) {
const glacier_storage_classes = [
s3_utils.STORAGE_CLASS_GLACIER,
s3_utils.STORAGE_CLASS_GLACIER_DA,
s3_utils.STORAGE_CLASS_GLACIER_IR,
Copy link
Contributor

Choose a reason for hiding this comment

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

Newly added storage class in STORAGE_CLASS_GLACIER_DA, And STORAGE_CLASS_GLACIER_IR was missing in previous check? Is there any reason for adding it now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, these are newly added. GLACIER_DA is something DBS3 specific and they want to support this one as well on the other hand our implementation is quite close to GLACIER_IR (instant retrieval) because DBS2 usually triggers the retrieval within 10 minutes and the restores usually finish within 20 minutes of submitting the request unlike AWS S3 which can take couple of hours.

];

if (!storage_class || storage_class === s3_utils.STORAGE_CLASS_STANDARD) return true;

if (storage_class === s3_utils.STORAGE_CLASS_GLACIER) {
// TODO: Upon integration with underlying systems, we should
// check if archiving is actually supported or not
if (glacier_storage_classes.includes(storage_class)) {
return config.NSFS_GLACIER_ENABLED || false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/sdk/nb.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ type DigestType = 'sha1' | 'sha256' | 'sha384' | 'sha512';
type CompressType = 'snappy' | 'zlib';
type CipherType = 'aes-256-gcm';
type ParityType = 'isa-c1' | 'isa-rs' | 'cm256';
type StorageClass = 'STANDARD' | 'GLACIER' | 'GLACIER_IR';
type StorageClass = 'STANDARD' | 'GLACIER' | 'GLACIER_IR' | 'GLACIER_DA';
type ResourceType = 'HOSTS' | 'CLOUD' | 'INTERNAL';
type NodeType =
'BLOCK_STORE_S3' |
Expand Down