NC | Lifecycle | An internal file/folder should be blocked for deletion by the lifecycle worker #9073
Conversation
…by the lifecycle worker Signed-off-by: Romy <35330373+romayalon@users.noreply.github.com>
WalkthroughThe changes expand test coverage for object deletion with lifecycle filters, particularly adding negative test cases for internal and special files. Assertion helpers were updated for flexibility. The lifecycle filter logic was enhanced to exclude temporary, folder marker, and versioned objects. A debug log was removed from a CORS test. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/test/unit_tests/jest_tests/test_nc_lifecycle.test.js (1)
235-246: Duplicate test case detected.This test appears to be identical to the previous test (lines 221-232) with the same test description and logic. Consider either removing this duplicate or modifying it to test a different scenario.
Remove the duplicate test or modify it to test a different aspect:
- it('delete_multiple_objects - filter should fail on internal .versions file - versioning ENABLED bucket', async () => { + it('delete_multiple_objects - filter should fail on internal .versions file with explicit prefix - versioning ENABLED bucket', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/unit_tests/jest_tests/test_nc_lifecycle.test.js(3 hunks)src/test/unit_tests/test_s3_ops.js(0 hunks)src/util/lifecycle_utils.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/test_s3_ops.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/util/lifecycle_utils.js (11)
src/endpoint/s3/ops/s3_put_object.js (2)
object_info(84-89)config(9-9)src/test/unit_tests/jest_tests/test_nc_lifecycle.test.js (1)
config(10-10)src/test/unit_tests/test_s3_ops.js (1)
config(10-10)src/manage_nsfs/health.js (1)
config(7-7)src/test/system_tests/test_utils.js (1)
config(10-10)src/util/http_utils.js (1)
config(19-19)src/manage_nsfs/nc_lifecycle.js (1)
config(10-10)src/cmd/manage_nsfs.js (1)
config(18-18)src/sdk/namespace_fs.js (1)
config(13-13)src/test/unit_tests/test_lifecycle.js (1)
config(20-20)src/test/unit_tests/jest_tests/test_nc_lifecycle_posix_integration.test.js (1)
config(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-sanity-ssl-tests
- GitHub Check: ceph-s3-tests
- GitHub Check: warp-nc-tests
- GitHub Check: run-sanity-tests
- GitHub Check: warp-tests
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-unit-tests-postgres
- GitHub Check: run-package-lock-validation
- GitHub Check: nsfs-ceph-s3-tests
- GitHub Check: run-unit-tests
- GitHub Check: run-nc-unit-tests
🔇 Additional comments (7)
src/util/lifecycle_utils.js (1)
151-154:✅ Verification successful
Security enhancement correctly implemented.
The pre-filtering logic effectively prevents lifecycle operations from targeting internal system files, temporary files, and versioned objects. The placement before other filters ensures these exclusions take absolute precedence.
Please verify that the config constants are properly defined and have appropriate values:
🏁 Script executed:
#!/bin/bash # Verify config constants used in the lifecycle filter rg -A 2 -B 2 "NSFS_TEMP_DIR_NAME|NSFS_FOLDER_OBJECT_NAME" --type jsLength of output: 21209
Lifecycle filter constants verified and approved
Verified that the two config constants used in
build_lifecycle_filterare correctly defined inconfig.js:
config.NSFS_TEMP_DIR_NAME = '.noobaa-nsfs'config.NSFS_FOLDER_OBJECT_NAME = '.folder'There is currently no config constant for the version‐path check (
'.versions/'). To avoid a magic string, consider adding something like:// in config.js config.NSFS_VERSIONS_DIR_NAME = '.versions';and updating the filter:
- if (object_info.key.includes('.versions/')) return false; + if (object_info.key.includes(`${config.NSFS_VERSIONS_DIR_NAME}/`)) return false;src/test/unit_tests/jest_tests/test_nc_lifecycle.test.js (6)
95-102: Well-designed test for internal multipart upload file exclusion.The test correctly creates a multipart upload, constructs the internal file path, and verifies that the lifecycle filter blocks deletion with an empty prefix filter.
104-111: Good coverage for explicit prefix targeting internal files.This test ensures that even when the lifecycle filter explicitly targets the internal file path with a matching prefix, the deletion is still blocked. This validates the absolute nature of the internal file exclusion.
113-121: Effective test for folder object exclusion with empty prefix.The test correctly creates a directory object and verifies that the associated
.foldermarker file is protected from lifecycle deletion, even with a broad empty prefix filter.
123-131: Comprehensive test for folder object exclusion with matching prefix.This test validates that folder marker objects remain protected even when the lifecycle filter prefix specifically targets the directory path. This ensures robust protection of internal folder structures.
221-232: Excellent test coverage for versioned file protection.The test properly sets up versioning, creates multiple versions, and verifies that internal
.versions/files are excluded from lifecycle deletion. The version file path construction is correct.
629-641: Helper function enhancement improves test flexibility.The modification to accept an optional
delete_objects_arrparameter and dynamically determine the assertion key makes the helper function more versatile for testing different object keys beyond the globalkeyvariable.The logic correctly falls back to the global
keywhendelete_objects_arris not provided, maintaining backward compatibility with existing tests.
…dirs` Docstrings generation was requested by @romayalon. * #9073 (comment) The following files were modified: * `src/util/lifecycle_utils.js`
|
Note Generated docstrings for this pull request at #9074 |
romayalon
changed the title
| @@ -148,6 +148,11 @@ function build_lifecycle_filter(params) { | |||
| * @param {Object} object_info | |||
| */ | |||
| return function(object_info) { | |||
| // fail if object is a temp file/part or a folder object or a versioned object | |||
There was a problem hiding this comment.
this logic is shared with list-object. and we would need to change in both cases in case there will be a new hidden file/folder added. maybe we should add a utility function to check if file is internal directory?
There was a problem hiding this comment.
can you explain why would we need to change in 2 different places if this function is shared?
There was a problem hiding this comment.
they are both intended to ignore internal directories when going over files. if there is a new internal directory. we would need to ignore it in both
There was a problem hiding this comment.
indeed, since both places using this function why would we need to change in 2 different places? or I don't get your point...
There was a problem hiding this comment.
I mean list-object in general regardless of lifecycle:
if (!ent.name.startsWith(prefix_ent) ||
ent.name < marker_curr ||
ent.name === this.get_bucket_tmpdir_name() ||
(ent.name === config.NSFS_FOLDER_OBJECT_NAME && is_disabled_dir_content) ||
this._is_hidden_version_path(ent.name)) {
return;
}
Describe the Problem
An internal file/folder should be blocked for deletion by the lifecycle worker, we are not supposed to see an issue due to the whole flow but I added it in order to be extra safe on the filter func as well.
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
sudo jest --testRegex=jest_tests/test_nc_lifecycle.testSummary by CodeRabbit