NC | Adding support of user bucket path #9271
Conversation
WalkthroughAdds support for optional NSFS custom bucket paths: new config keys and header, request/schema/CLI extensions, S3 endpoint header handling, bucketspace path validation against an account-scoped allow-list, and integration tests for success, collision and authorization cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant S3Endpoint as S3 PutBucket
participant ObjectSDK
participant BucketspaceFS
participant AccountCfg as Account Config
Client->>S3Endpoint: PUT Bucket (Header: x-noobaa-custom-bucket-path: /custom/path)
S3Endpoint->>S3Endpoint: read header -> params.custom_bucket_path
S3Endpoint->>ObjectSDK: create_bucket(name, lock_enabled, custom_bucket_path)
ObjectSDK->>AccountCfg: fetch nsfs_account_config.custom_bucket_path_allowed_list
ObjectSDK->>BucketspaceFS: create_bucket(params)
alt custom path provided and allowed
BucketspaceFS->>BucketspaceFS: set bucket_storage_path = params.custom_bucket_path
BucketspaceFS-->>ObjectSDK: success
else custom path provided but not allowed
BucketspaceFS-->>ObjectSDK: throw UNAUTHORIZED (AccessDenied)
else no custom path
BucketspaceFS->>BucketspaceFS: set bucket_storage_path = default (new_buckets_path + name)
BucketspaceFS-->>ObjectSDK: success
end
ObjectSDK-->>S3Endpoint: success / error
S3Endpoint-->>Client: 200 OK / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/bucketspace_fs.js (1)
306-314: Critical: Validate and confine filesystem path from header to prevent unauthorized bucket creationThe
user_bucket_pathparameter (from HTTP headers) is accepted without validation, enabling a client to direct bucket creation to arbitrary filesystem paths. The existing validation function only validates the bucket name, not this path.Apply the suggested guard to ensure
user_bucket_pathis absolutely rejected if it's an absolute path (e.g., /etc/passwd) and confined to remain strictly within the intended base directory:- const { name } = params; - const bucket_config_path = this.config_fs.get_bucket_path_by_name(name); - const bucket_storage_path = params.user_bucket_path || - path.join(sdk.requesting_account.nsfs_account_config.new_buckets_path, name); + const { name } = params; + const bucket_config_path = this.config_fs.get_bucket_path_by_name(name); + let bucket_storage_path; + if (params.user_bucket_path) { + if (typeof params.user_bucket_path !== 'string' || params.user_bucket_path.trim() === '') { + throw new RpcError('INVALID_REQUEST', 'user_bucket_path must be a non-empty string'); + } + const resolved = path.resolve(params.user_bucket_path); + if (!path.isAbsolute(resolved)) { + throw new RpcError('INVALID_REQUEST', 'user_bucket_path must be an absolute path'); + } + const base = path.resolve(sdk.requesting_account.nsfs_account_config.new_buckets_path); + // Constrain to the account's bucket root + if (!(resolved === base || resolved.startsWith(base + path.sep))) { + throw new RpcError('FORBIDDEN', 'user_bucket_path must reside under the account new_buckets_path'); + } + bucket_storage_path = resolved; + } else { + bucket_storage_path = path.join( + sdk.requesting_account.nsfs_account_config.new_buckets_path, + name + ); + }
🧹 Nitpick comments (6)
config.js (1)
1019-1020: Header constant looks good; consider default CORS allow-listing itIf browsers will send this header, add it to S3_CORS_ALLOW_HEADERS to pass preflight.
Apply after the new constant:
config.NSFS_USER_BUCKET_PATH_HTTP_HEADER = 'x-nsfs-bucket-path'; + +// Allow the custom header for browser clients (preflight) +if (Array.isArray(config.S3_CORS_ALLOW_HEADERS) && + !config.S3_CORS_ALLOW_HEADERS.includes(config.NSFS_USER_BUCKET_PATH_HTTP_HEADER)) { + config.S3_CORS_ALLOW_HEADERS.push(config.NSFS_USER_BUCKET_PATH_HTTP_HEADER); +}src/api/bucket_api.js (1)
36-36: Tighten schema for user_bucket_pathEnforce non-empty, absolute path (NSFS-only) to fail early on bad input.
- user_bucket_path: { type: 'string' } + user_bucket_path: { + type: 'string', + minLength: 1, + description: 'Absolute filesystem path for underlying bucket directory (NSFS only).', + pattern: '^/' // require absolute POSIX path + }src/endpoint/s3/ops/s3_put_bucket.js (1)
12-13: Normalize header key to lowercase before lookupNode lowercases incoming header keys. If the header name is overridden via ENV, normalize toLowerCase to avoid mismatches. Based on learnings
- const user_bucket_path = req.headers[config.NSFS_USER_BUCKET_PATH_HTTP_HEADER]; - await req.object_sdk.create_bucket({ name: req.params.bucket, lock_enabled: lock_enabled, user_bucket_path: user_bucket_path }); + const header_name = String(config.NSFS_USER_BUCKET_PATH_HTTP_HEADER || '').toLowerCase(); + const user_bucket_path = typeof req.headers[header_name] === 'string' + ? req.headers[header_name].trim() + : undefined; + await req.object_sdk.create_bucket({ + name: req.params.bucket, + lock_enabled, + user_bucket_path + });src/test/integration_tests/nsfs/test_nsfs_integration.js (3)
383-401: Use the config constant for header name and reflect it in the test titleAvoid string drift and keep title accurate.
- mocha.it('create s3 bucket with x-nsfs-bucket-path', async function() { + mocha.it(`create s3 bucket with ${config.NSFS_USER_BUCKET_PATH_HTTP_HEADER}`, async function() { @@ - s3_correct_uid_default_nsr.middlewareStack.add( + s3_correct_uid_default_nsr.middlewareStack.add( (next, context) => args => { - args.request.headers["x-nsfs-bucket-path"] = x_nsfs_bucket_path; + args.request.headers[config.NSFS_USER_BUCKET_PATH_HTTP_HEADER] = x_nsfs_bucket_path; return next(args); }, { step: "finalizeRequest", name: "addCustomHeader", } );
402-425: Fix typo in test name and reuse the config header constantTitle mentions x-mcg-bucket-path; should be the configured header. Reuse the constant to prevent drift.
- mocha.it('create s3 bucket with x-mcg-bucket-path fail as directory exist', async function() { + mocha.it(`create s3 bucket with ${config.NSFS_USER_BUCKET_PATH_HTTP_HEADER} - fail if directory exists`, async function() { @@ - args.request.headers["x-nsfs-bucket-path"] = x_nsfs_bucket_path; + args.request.headers[config.NSFS_USER_BUCKET_PATH_HTTP_HEADER] = x_nsfs_bucket_path; return next(args);
383-401: Add negative tests for path validation (relative/out-of-root)Once validation is added, add cases that assert INVALID_REQUEST for relative paths and FORBIDDEN for paths outside new_buckets_path.
I can draft the additional test cases after you pick the exact errors to assert.
Also applies to: 402-425
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config.js(1 hunks)src/api/bucket_api.js(1 hunks)src/endpoint/s3/ops/s3_put_bucket.js(1 hunks)src/sdk/bucketspace_fs.js(1 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/nsfs/test_nsfs_integration.js
🧠 Learnings (2)
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/endpoint/s3/ops/s3_put_bucket.js
📚 Learning: 2025-07-30T10:20:09.402Z
Learnt from: liranmauda
Repo: noobaa/noobaa-core PR: 9174
File: config.js:1044-1046
Timestamp: 2025-07-30T10:20:09.402Z
Learning: The NooBaa config.js file has a built-in environment variable override system via `load_config_env_overrides()` function. Any config property can be overridden using environment variables prefixed with `CONFIG_JS_`. For example, `CONFIG_JS_NC_HEALTH_BUCKETS_COUNT_LIMIT=10000` would override `config.NC_HEALTH_BUCKETS_COUNT_LIMIT`. The system handles type conversion automatically and provides consistent override patterns across all config values.
Applied to files:
config.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
jackyalbo
force-pushed
the
jacky-nc-fixes
branch
from
1bf7a19 to
bc8b718
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/endpoint/s3/ops/s3_put_bucket.js (1)
12-13: Consider adding defensive validation at the entry point.Even if downstream validation exists, applying defense-in-depth principles by adding basic checks here would reduce the attack surface and provide clearer error messages:
const user_bucket_path = req.headers[config.NSFS_USER_BUCKET_PATH_HTTP_HEADER]; +if (user_bucket_path !== undefined) { + if (typeof user_bucket_path !== 'string' || user_bucket_path.trim() === '') { + throw new S3Error(S3Error.InvalidArgument); + } + if (user_bucket_path.includes('\0') || user_bucket_path.includes('..')) { + throw new S3Error(S3Error.InvalidArgument); + } +} await req.object_sdk.create_bucket({ name: req.params.bucket, lock_enabled: lock_enabled, user_bucket_path: user_bucket_path });This provides basic sanitization while allowing comprehensive validation downstream.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config.js(1 hunks)src/api/bucket_api.js(1 hunks)src/endpoint/s3/ops/s3_put_bucket.js(1 hunks)src/sdk/bucketspace_fs.js(1 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- config.js
- src/api/bucket_api.js
- src/sdk/bucketspace_fs.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/endpoint/s3/ops/s3_put_bucket.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
jackyalbo
force-pushed
the
jacky-nc-fixes
branch
from
bc8b718 to
b76db97
Compare
jackyalbo
force-pushed
the
jacky-nc-fixes
branch
4 times, most recently
from
5f51d95 to
9bbc2a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/manage_nsfs.js (1)
486-510: fs_backend empty-string deletion semantics are broken; fix confirmedThe issue is verified and accurate. Line 506 uses a truthiness check:
fs_backend: user_input.fs_backend ? String(user_input.fs_backend) : config.NSFS_NC_STORAGE_BACKEND,This prevents empty strings from reaching the normalization at line 534 (
data.nsfs_account_config.fs_backend = data.nsfs_account_config.fs_backend || undefined;), breaking the intended "deletion via empty string" behavior.The bucket version (line 133) correctly uses an explicit
undefinedcheck, allowing empty strings through:fs_backend: user_input.fs_backend === undefined ? config.NSFS_NC_STORAGE_BACKEND : String(user_input.fs_backend),The
custom_bucket_path_allowed_listat line 507 is handled correctly—it assigns directly and lets line 547's normalization handle falsy values.Apply the suggested fix at line 506:
- fs_backend: user_input.fs_backend ? String(user_input.fs_backend) : config.NSFS_NC_STORAGE_BACKEND, + fs_backend: user_input.fs_backend === undefined + ? config.NSFS_NC_STORAGE_BACKEND + : String(user_input.fs_backend),
🧹 Nitpick comments (3)
src/test/integration_tests/nsfs/test_nsfs_integration.js (3)
418-457: Happy-path test for custom bucket path + allowed-list is solidCreating a dedicated account with
custom_bucket_path_allowed_listincludingtmp_fs_rootand then asserting that:
- the bucket create succeeds when the header points under
new_buckets_path, and- the filesystem path at
x_nsfs_bucket_pathexistsgives good end-to-end coverage that the allow-list is honored and the header path is actually used. As an optional cleanup, you could extract the repeated middleware wiring into a small helper to reduce duplication across these tests.
As per coding guidelines, the new behavior is covered by tests under
src/test/**.
459-485: Directory-exists collision test for custom path behaves as expectedThis test reuses the account with an allow-list to ensure:
- creating a bucket with a custom path that already exists returns
BucketAlreadyExists, and- no directory is created at the default new_buckets_path (when the header is present).
That correctly validates collision handling and that the header overrides the default location rather than falling back to it. For robustness and readability, consider using a single consistent pattern for building
x_nsfs_bucket_pathandno_x_nsfs_bucket_path(e.g., viapath.joinor a shared helper) to avoid relying on implicit trailing slashes innew_buckets_path.
486-508: Disallowed-prefix test is correct; comment could be clarifiedThe test correctly checks that:
- using a custom path like
/root/${bucket_name}-custom-paththat is outside the account’scustom_bucket_path_allowed_listreturnsAccessDenied, and- the target path is not created on disk.
The inline comment on Line 489 ("already created in previous test") is misleading, since
/root/...is intentionally not created earlier; cleaning that up would avoid future confusion. You might also align the path construction style here with the earlier tests for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config.js(1 hunks)src/api/account_api.js(1 hunks)src/api/bucket_api.js(1 hunks)src/api/common_api.js(1 hunks)src/cmd/manage_nsfs.js(2 hunks)src/endpoint/s3/ops/s3_put_bucket.js(1 hunks)src/manage_nsfs/manage_nsfs_constants.js(3 hunks)src/manage_nsfs/manage_nsfs_help_utils.js(2 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/bucketspace_fs.js(1 hunks)src/server/system_services/schemas/nsfs_account_schema.js(2 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(3 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/endpoint/s3/ops/s3_put_bucket.js
- src/api/bucket_api.js
- config.js
- src/sdk/bucketspace_fs.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/coretest/nc_coretest.jssrc/test/integration_tests/nsfs/test_nsfs_integration.js
🧠 Learnings (1)
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/system_services/schemas/nsfs_account_schema.jssrc/sdk/accountspace_fs.jssrc/cmd/manage_nsfs.jssrc/test/utils/coretest/nc_coretest.jssrc/api/common_api.js
🧬 Code graph analysis (1)
src/api/common_api.js (16)
src/api/account_api.js (1)
SensitiveString(4-4)src/cmd/manage_nsfs.js (1)
SensitiveString(25-25)src/sdk/accountspace_fs.js (1)
SensitiveString(16-16)src/sdk/bucketspace_fs.js (1)
SensitiveString(28-28)src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
SensitiveString(21-21)src/test/utils/coretest/nc_coretest.js (1)
SensitiveString(10-10)src/server/common_services/auth_server.js (1)
SensitiveString(12-12)src/util/account_util.js (1)
SensitiveString(9-9)src/server/system_services/bucket_server.js (1)
SensitiveString(14-14)src/server/system_services/account_server.js (1)
SensitiveString(15-15)src/server/system_services/schemas/account_schema.js (1)
SensitiveString(4-4)src/server/system_services/schemas/bucket_schema.js (1)
SensitiveString(4-4)src/sdk/config_fs.js (1)
SensitiveString(13-13)src/cmd/nsfs.js (1)
SensitiveString(38-38)src/endpoint/s3/ops/s3_get_bucket_logging.js (1)
SensitiveString(4-4)src/server/system_services/tier_server.js (1)
SensitiveString(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: ceph-s3-tests / Ceph S3 Tests
- GitHub Check: mint-nc-tests / Mint NC Tests
- GitHub Check: mint-tests / Mint Tests
- GitHub Check: run-nc-unit-tests / Non Containerized Unit Tests
- GitHub Check: warp-nc-tests / Warp NC Tests
- GitHub Check: warp-tests / Warp Tests
- GitHub Check: ceph-nsfs-s3-tests / NSFS Ceph S3 Tests
- GitHub Check: run-sanity-tests / Sanity Tests
- GitHub Check: run-sanity-ssl-tests / Sanity SSL Tests
- GitHub Check: run-unit-tests / Unit Tests
- GitHub Check: run-unit-tests-postgres / Unit Tests with Postgres
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (11)
src/sdk/accountspace_fs.js (1)
590-613: Propagating custom_bucket_path_allowed_list to new users is consistent and safeCopying
custom_bucket_path_allowed_listfromrequesting_account.nsfs_account_configinto new user defaults aligns with how other NSFS fields are propagated and ensures IAM users respect the same bucket-path policy as their owner.src/api/account_api.js (1)
280-313: Schema extension for custom_bucket_path_allowed_list in update_account_s3_access looks correctAdding
custom_bucket_path_allowed_listtonsfs_account_confighere matches the common_api and nsfs_account_schema definitions and keeps the update path in sync with account creation / manage_nsfs flows.src/test/utils/coretest/nc_coretest.js (1)
327-343: Test helpers wiring custom_bucket_path_allowed_list into manage_nsfs CLI is aligned with runtime changesPropagating
nsfs_account_config.custom_bucket_path_allowed_listinto the add/update account CLI options keeps the NC core tests compatible with the new field and allows exercising the allow-list behavior end‑to‑end.Also applies to: 431-439
src/api/common_api.js (1)
1445-1469: nsfs_account_config schema now includes custom_bucket_path_allowed_list in both variantsAdding
custom_bucket_path_allowed_list: { type: 'string' }to both nsfs_account_config oneOf branches is consistent with the rest of the PR (account_api, nsfs_account_schema, manage_nsfs) and maintains backward compatibility since it’s not required.src/server/system_services/schemas/nsfs_account_schema.js (1)
77-105: NSFS account schema aligned with new custom_bucket_path_allowed_list propertyIncluding
custom_bucket_path_allowed_listin both nsfs_account_config shapes keeps the on‑disk schema compatible with the updated common/account APIs and manage_nsfs tooling; the field is optional, so existing accounts remain valid.src/test/integration_tests/nsfs/test_nsfs_integration.js (3)
2-2: Eslint max-lines override is reasonableRaising
max-linesto 3000 is consistent with the current file size (~2500 lines) and avoids spurious lint failures without behavioral impact.
84-91: New S3 client variable for custom bucket-path tests is scoped correctly
let s3_correct_uid_bucket_path;is in the same scope as the other S3 clients and used only in the new custom-bucket-path tests, so there’s no leakage or shadowing risk.
393-416: Negative test for header without allowed-list looks correctThis test validates that using
config.NSFS_CUSTOM_BUCKET_PATH_HTTP_HEADERwithout configuringcustom_bucket_path_allowed_liston the account yieldsAccessDeniedand does not create the target directory, which matches the intended security behavior.src/manage_nsfs/manage_nsfs_help_utils.js (2)
133-147: Add-account help for--custom_bucket_path_allowed_listis aligned with semanticsThe new flag description clearly conveys that the value is a colon-separated list of allowed custom bucket paths and uses the same option name as the internal wiring (
custom_bucket_path_allowed_list), so users will discover the feature viaaccount add --help.
160-175: Update-account help correctly documents override/unset behaviorThe update flag text explains that
--custom_bucket_path_allowed_listoverrides the existing list and can be unset with'', which matches how UNSETTABLE options are handled in the constants. This keeps CLI behavior and help in sync.src/manage_nsfs/manage_nsfs_constants.js (1)
48-54: CLI wiring forcustom_bucket_path_allowed_listis consistent and completeIncluding
custom_bucket_path_allowed_listin:
VALID_OPTIONS_ACCOUNT.add/.update,OPTION_TYPEas a'string', andUNSETTABLE_OPTIONS_OBJwithCLI_EMPTY_STRINGensures the new flag is accepted for account add/update, typed correctly, and can be unset via
''as documented in the help. This lines up cleanly with the account schema and CLI help text for the feature.Also applies to: 119-127, 193-201
|
@nadavMiz if you could take a look please |
|
jackyalbo
force-pushed
the
jacky-nc-fixes
branch
from
9bbc2a5 to
e16e253
Compare
Added tests and updated documentation. @nadavMiz Please take another look. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/endpoint/s3/ops/s3_put_bucket.js (1)
12-13: Ensure downstream validation/authorization forcustom_bucket_pathis robustPassing the header value straight into
create_bucketis fine here, as long as the SDK layer (e.g.,bucketspace_fs/validate_bucket_creation) fully normalizes and validates it (absolute path, traversal-safe against allowed prefixes, and authorized for the account). Please double‑check that the downstream logic now does this so that this new header can't be used for path traversal or unauthorized locations.
🧹 Nitpick comments (5)
docs/NooBaaNonContainerized/AccountsAndBuckets.md (1)
41-42: Polish wording and indentation forcustom_bucket_path_allowed_listentryThe semantics read fine, but I’d suggest tightening the text and fixing the list indentation to satisfy markdownlint:
- - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons). + - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, it can override the default bucket path location (under `new_buckets_path`) using the `x-noobaa-custom-bucket-path` HTTP header. The bucket directory is created only if the requested path is under one of the allowed paths in `custom_bucket_path_allowed_list`. Must be a colon-separated list of absolute paths.src/test/integration_tests/nc/cli/test_nc_account_cli.test.js (1)
1245-1259: Strengthen “unset custom_bucket_path_allowed_list” test to cover set→unsetRight now this test calls UPDATE with
CLI_UNSET_EMPTY_STRINGwhencustom_bucket_path_allowed_listwas never set, then verifies it’s undefined, and finally sets a non‑empty value. That doesn’t prove that an existing value can be unset.Consider first setting a non‑empty
custom_bucket_path_allowed_list, then unsetting it, for example:- const { name } = defaults; - //in exec_manage_cli an empty string is passed as no input. so operation fails on invalid type. use the string '' instead - const account_options = { config_root, name, custom_bucket_path_allowed_list: CLI_UNSET_EMPTY_STRING}; + const { name } = defaults; + // first set a concrete list + const account_options = { config_root, name, custom_bucket_path_allowed_list: "/some/path/:/another/path/" }; + const action = ACTIONS.UPDATE; + await exec_manage_cli(type, action, account_options); + let new_account_details = await config_fs.get_account_by_name(name, config_fs_account_options); + expect(new_account_details.nsfs_account_config.custom_bucket_path_allowed_list) + .toBe("/some/path/:/another/path/"); + + // now unset it using the special empty-string marker + account_options.custom_bucket_path_allowed_list = CLI_UNSET_EMPTY_STRING; const action = ACTIONS.UPDATE; await exec_manage_cli(type, action, account_options); let new_account_details = await config_fs.get_account_by_name(name, config_fs_account_options); expect(new_account_details.nsfs_account_config.custom_bucket_path_allowed_list).toBeUndefined(); - - //set new_buckets_path value back to its original value - account_options.custom_bucket_path_allowed_list = "/some/path/:/another/path/"; - await exec_manage_cli(type, action, account_options); - new_account_details = await config_fs.get_account_by_name(name, config_fs_account_options); - expect(new_account_details.nsfs_account_config.custom_bucket_path_allowed_list).toBe("/some/path/:/another/path/");This would better exercise the unset semantics similar to the existing
new_buckets_pathtest.src/server/system_services/schemas/nsfs_account_schema.js (1)
77-105: Schema wiring forcustom_bucket_path_allowed_listis correct; consider tightening validationAdding
custom_bucket_path_allowed_listto bothnsfs_account_configvariants is consistent and unblocks API/CLI usage.If you want stronger early validation, consider refining this from just
{ type: 'string' }to something that reflects the expected format (e.g., absolute paths, colon‑separated), viapatternor a dedicatedcommon_apidefinition. Not mandatory, but it would catch obvious misconfigurations at schema‑validation time instead of later.docs/NooBaaNonContainerized/NooBaaCLI.md (1)
79-80: Clarifycustom_bucket_path_allowed_listflag descriptions and align list indentationThe new flag wiring looks right; the docs could be a bit clearer and will avoid markdownlint complaints if aligned with surrounding bullets. Suggested tweaks:
@@ Add Account usage -noobaa-cli account add --name <account_name> --uid <uid> --gid <gid> [--user] -[--new_buckets_path][--access_key][--secret_key][--fs_backend] -[--allow_bucket_creation][--force_md5_etag][--anonymous][--from_file][--iam_operate_on_root_account][--default_connection][--custom_bucket_path_allowed_list] +noobaa-cli account add --name <account_name> --uid <uid> --gid <gid> [--user] +[--new_buckets_path][--access_key][--secret_key][--fs_backend] +[--allow_bucket_creation][--force_md5_etag][--anonymous][--from_file][--iam_operate_on_root_account][--default_connection][--custom_bucket_path_allowed_list] @@ Add Account flags -- `custom_bucket_path_allowed_list` - - Type: String - - Description: Specifies an allowed list where this account can create buckets in using - x-noobaa-custom-bucket-path header in create_bucket +- `custom_bucket_path_allowed_list` + - Type: String + - Description: Colon-separated list of absolute paths where this account may create buckets when using the `x-noobaa-custom-bucket-path` S3 header (for example: `/gpfs/data/custom1/:/gpfs/data/custom2/`). @@ Update Account usage -noobaa-cli account update --name <account_name> [--new_name][--uid][--gid][--user] -[--new_buckets_path][--access_key][--secret_key][--regenerate][--fs_backend] -[--allow_bucket_creation][--force_md5_etag][--anonymous][--iam_operate_on_root_account][--default_connection] -[--custom_bucket_path_allowed_list] +noobaa-cli account update --name <account_name> [--new_name][--uid][--gid][--user] +[--new_buckets_path][--access_key][--secret_key][--regenerate][--fs_backend] +[--allow_bucket_creation][--force_md5_etag][--anonymous][--iam_operate_on_root_account][--default_connection] +[--custom_bucket_path_allowed_list] @@ Update Account flags -- `custom_bucket_path_allowed_list` - - Type: String - - Description: Specifies an allowed list where this account can create buckets in using - x-noobaa-custom-bucket-path header in create_bucket +- `custom_bucket_path_allowed_list` + - Type: String + - Description: Colon-separated list of absolute paths to override the allowed custom bucket paths for this account when using the `x-noobaa-custom-bucket-path` S3 header. Unset with `''`.This keeps indentation consistent with the other bullets and makes the behavior of the flag more explicit.
Also applies to: 143-147, 154-158, 225-229
src/manage_nsfs/manage_nsfs_help_utils.js (1)
146-147: Help text for--custom_bucket_path_allowed_listlooks good; minor punctuation nitThe new flag descriptions under
ACCOUNT_FLAGS_ADDandACCOUNT_FLAGS_UPDATEare clear and consistent with the docs. You might slightly polish the update text for readability:- --custom_bucket_path_allowed_list <string> (optional) Update the list of allowed custom bucket paths, separated by colons (:) example: '/gpfs/data/custom1/:/gpfs/data/custom2/' (override;unset with '') + --custom_bucket_path_allowed_list <string> (optional) Update the list of allowed custom bucket paths, separated by colons (:) example: '/gpfs/data/custom1/:/gpfs/data/custom2/' (override; unset with '')Otherwise this wiring is solid.
Also applies to: 174-175
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
config.js(1 hunks)docs/NooBaaNonContainerized/AccountsAndBuckets.md(1 hunks)docs/NooBaaNonContainerized/NooBaaCLI.md(4 hunks)src/api/account_api.js(1 hunks)src/api/bucket_api.js(1 hunks)src/api/common_api.js(1 hunks)src/cmd/manage_nsfs.js(2 hunks)src/endpoint/s3/ops/s3_put_bucket.js(1 hunks)src/manage_nsfs/manage_nsfs_constants.js(3 hunks)src/manage_nsfs/manage_nsfs_help_utils.js(2 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/bucketspace_fs.js(1 hunks)src/server/system_services/schemas/nsfs_account_schema.js(2 hunks)src/test/integration_tests/nc/cli/test_nc_account_cli.test.js(2 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(3 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/utils/coretest/nc_coretest.js
- src/sdk/bucketspace_fs.js
- src/manage_nsfs/manage_nsfs_constants.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/api/common_api.js
- config.js
- src/sdk/accountspace_fs.js
- src/cmd/manage_nsfs.js
- src/api/account_api.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/nc/cli/test_nc_account_cli.test.js
🧠 Learnings (2)
📚 Learning: 2025-07-30T10:20:09.402Z
Learnt from: liranmauda
Repo: noobaa/noobaa-core PR: 9174
File: config.js:1044-1046
Timestamp: 2025-07-30T10:20:09.402Z
Learning: The NooBaa config.js file has a built-in environment variable override system via `load_config_env_overrides()` function. Any config property can be overridden using environment variables prefixed with `CONFIG_JS_`. For example, `CONFIG_JS_NC_HEALTH_BUCKETS_COUNT_LIMIT=10000` would override `config.NC_HEALTH_BUCKETS_COUNT_LIMIT`. The system handles type conversion automatically and provides consistent override patterns across all config values.
Applied to files:
docs/NooBaaNonContainerized/AccountsAndBuckets.md
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/system_services/schemas/nsfs_account_schema.js
🧬 Code graph analysis (1)
src/test/integration_tests/nc/cli/test_nc_account_cli.test.js (3)
src/manage_nsfs/manage_nsfs_constants.js (1)
ACTIONS(17-23)config.js (1)
type(1212-1212)src/test/system_tests/test_utils.js (1)
CLI_UNSET_EMPTY_STRING(27-27)
🪛 markdownlint-cli2 (0.18.1)
docs/NooBaaNonContainerized/AccountsAndBuckets.md
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
docs/NooBaaNonContainerized/NooBaaCLI.md
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/api/bucket_api.js (1)
36-36: API surface extension forcustom_bucket_pathlooks consistentAdding
custom_bucket_pathas an optional string param oncreate_bucketaligns with the new S3 header and downstream handling; no further schema changes are strictly required here.
Signed-off-by: jackyalbo <[email protected]>
jackyalbo
force-pushed
the
jacky-nc-fixes
branch
from
e16e253 to
dae43d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
config.js(1 hunks)docs/NooBaaNonContainerized/AccountsAndBuckets.md(1 hunks)docs/NooBaaNonContainerized/NooBaaCLI.md(4 hunks)src/api/account_api.js(1 hunks)src/api/bucket_api.js(1 hunks)src/api/common_api.js(1 hunks)src/cmd/manage_nsfs.js(2 hunks)src/endpoint/s3/ops/s3_put_bucket.js(1 hunks)src/manage_nsfs/manage_nsfs_constants.js(3 hunks)src/manage_nsfs/manage_nsfs_help_utils.js(2 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/bucketspace_fs.js(1 hunks)src/server/system_services/schemas/nsfs_account_schema.js(2 hunks)src/test/integration_tests/nc/cli/test_nc_account_cli.test.js(2 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(3 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/cmd/manage_nsfs.js
- src/test/integration_tests/nc/cli/test_nc_account_cli.test.js
- src/sdk/bucketspace_fs.js
- src/server/system_services/schemas/nsfs_account_schema.js
- src/endpoint/s3/ops/s3_put_bucket.js
- config.js
- src/api/common_api.js
- src/api/bucket_api.js
- src/test/utils/coretest/nc_coretest.js
- src/sdk/accountspace_fs.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T10:20:09.402Z
Learnt from: liranmauda
Repo: noobaa/noobaa-core PR: 9174
File: config.js:1044-1046
Timestamp: 2025-07-30T10:20:09.402Z
Learning: The NooBaa config.js file has a built-in environment variable override system via `load_config_env_overrides()` function. Any config property can be overridden using environment variables prefixed with `CONFIG_JS_`. For example, `CONFIG_JS_NC_HEALTH_BUCKETS_COUNT_LIMIT=10000` would override `config.NC_HEALTH_BUCKETS_COUNT_LIMIT`. The system handles type conversion automatically and provides consistent override patterns across all config values.
Applied to files:
docs/NooBaaNonContainerized/AccountsAndBuckets.md
🪛 markdownlint-cli2 (0.18.1)
docs/NooBaaNonContainerized/AccountsAndBuckets.md
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
docs/NooBaaNonContainerized/NooBaaCLI.md
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
226-226: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (8)
src/api/account_api.js (1)
311-311: LGTM - Schema extension looks good.The new field is correctly added to the schema with appropriate typing. Format validation (colon-separated paths, absolute path checks) is handled at the application layer per the PR design.
docs/NooBaaNonContainerized/NooBaaCLI.md (2)
79-79: LGTM - Usage line correctly updated.The new flag is appropriately added to the usage string.
157-157: LGTM - Update usage correctly includes the new flag.src/manage_nsfs/manage_nsfs_help_utils.js (2)
146-146: LGTM - Help text is clear and consistent.The flag documentation includes a helpful example and follows the established pattern.
174-174: LGTM - Update help text correctly documents override and unset behavior.Consistent with other unsettable options in the CLI.
src/manage_nsfs/manage_nsfs_constants.js (3)
49-50: LGTM - Option correctly registered for add and update actions.The new option is properly added to the validation sets.
126-126: LGTM - Type mapping correctly defined.The string type is appropriate for this option.
200-200: LGTM - Option correctly marked as unsettable.Using
CLI_EMPTY_STRINGis appropriate, allowing users to clear the allowed list with an empty string.
| - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons). | ||
|
|
There was a problem hiding this comment.
Fix markdown list indentation.
The bullet point has incorrect indentation (2 spaces instead of 0, as it should align with other properties at the same level).
Apply this diff to fix the indentation:
- - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons).
+- `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons). 📝 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.
| - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons). | |
| - `custom_bucket_path_allowed_list` - When an account creates a bucket using the S3 protocol, He can override the default bucket path location (under new_buckets_path) using `x-noobaa-custom-bucket-path` HTTP header. This directory will be created only if this path will be under one of the provided allowed list paths in custom_bucket_path_allowed_list. Must be a list of absolute paths (divided by colons). |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/NooBaaNonContainerized/AccountsAndBuckets.md around lines 41 to 42, the
markdown bullet for `custom_bucket_path_allowed_list` is indented with two
leading spaces instead of being flush with other top-level properties; remove
the two leading spaces so the list item aligns with the other properties (keep
the text and inline code/backticks unchanged).
| - `custom_bucket_path_allowed_list` | ||
| - Type: String | ||
| - Description: Specifies an allowed list where this account can create buckets in using | ||
| x-noobaa-custom-bucket-path header in create_bucket |
There was a problem hiding this comment.
Fix markdown list indentation.
The description lines have inconsistent indentation (4 spaces instead of 2).
Apply this diff to fix the indentation:
- `custom_bucket_path_allowed_list`
- - Type: String
- - Description: Specifies an allowed list where this account can create buckets in using
- x-noobaa-custom-bucket-path header in create_bucket
+ - Type: String
+ - Description: Specifies an allowed list where this account can create buckets in using
+ x-noobaa-custom-bucket-path header in create_bucket🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
144-144: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/NooBaaNonContainerized/NooBaaCLI.md around lines 143 to 146, the
markdown list for `custom_bucket_path_allowed_list` has inconsistent
indentation: the description lines use 4 spaces instead of the surrounding
2-space list indentation; update the indentation so the "Type:" and
"Description:" lines are indented by 2 spaces under the list item (align with
the backtick item), preserving content and line breaks.
| - `custom_bucket_path_allowed_list` | ||
| - Type: String | ||
| - Description: Specifies an allowed list where this account can create buckets in using | ||
| x-noobaa-custom-bucket-path header in create_bucket |
There was a problem hiding this comment.
Fix markdown list indentation.
The description lines have inconsistent indentation (4 spaces instead of 2).
Apply this diff to fix the indentation:
- `custom_bucket_path_allowed_list`
- - Type: String
- - Description: Specifies an allowed list where this account can create buckets in using
- x-noobaa-custom-bucket-path header in create_bucket
+ - Type: String
+ - Description: Specifies an allowed list where this account can create buckets in using
+ x-noobaa-custom-bucket-path header in create_bucket📝 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.
| - `custom_bucket_path_allowed_list` | |
| - Type: String | |
| - Description: Specifies an allowed list where this account can create buckets in using | |
| x-noobaa-custom-bucket-path header in create_bucket | |
| - `custom_bucket_path_allowed_list` | |
| - Type: String | |
| - Description: Specifies an allowed list where this account can create buckets in using | |
| x-noobaa-custom-bucket-path header in create_bucket |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
226-226: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
227-227: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/NooBaaNonContainerized/NooBaaCLI.md around lines 225 to 228, the
markdown list item for `custom_bucket_path_allowed_list` has description lines
indented with 4 spaces instead of the expected 2, breaking list formatting;
update the description lines to use 2-space indentation so they align with the
list item and other entries, preserving the same text and hyphenation.
4 participants
Describe the Problem
Adding support for an out-of-S3 header to let the user control where the bucket is supposed to be created.
x-noobaa-custom-bucket-path - will point to where the user wants their new bucket to be saved in the file system.
Explain the Changes
config.NSFS_CUSTOM_BUCKET_PATH_ALLOWED_LIST = '/ibm1/:/ibm3/'
or as part of the account, nsfs_account_config, for example:
nsfs_account_config: {
uid: 100,
gid: 100,
new_buckets_path: '/gpfs/new_buckets',
nsfs_only: false,
custom_bucket_path_allowed_list:
/gpfs1/:/gpfs2/,}
If it exists, the account's custom_bucket_path_allowed_list will override the system's one.
If neither does, we will drop the request with 'access denied'.
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
custom_bucket_path_allowed_listin config.js and/or in your accountx-noobaa-custom-bucket-pathheader as part of S3 put_bucket request (using account creds)Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.