Merge pull request #8881 from romayalon/romy-backports-5.18

NC | Backports - 5.18.z

Author: nimrod-becker

config.js

@@ -950,6 +950,9 @@ config.ANONYMOUS_ACCOUNT_NAME = 'anonymous';
 
 config.NFSF_UPLOAD_STREAM_MEM_THRESHOLD = 8 * 1024 * 1024;
 
+// we want to change our handling related to EACCESS error
+config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES = false;
+
 ////////////////////////////
 // NSFS NON CONTAINERIZED //
 ////////////////////////////

docs/NooBaaNonContainerized/NooBaaCLI.md

@@ -838,4 +838,57 @@ sudo noobaa-cli connection add --from_file <options_connection_JSON_file_path>
 ### White List Server IP command example
 ```
 sudo noobaa-cli whitelist --ips ["127.0.0.1", "192.0.10.0", "3002:0bd6:0000:0000:0000:ee00:0033:6778"]'
-```
+```
+
+### Known issues
+
+1. Bucket with suffix `.json`
+    
+    When the customer creates a bucket with the suffix `.json`, suffix is removed from the bucket config file name. 
+    
+    For example 
+    ```
+    sudo noobaa-cli bucket add --name my-bucket1.json --owner <owner_name> --path <path>
+    ```
+    Actual file name : `{config_root}\buckets\my-test-bucket.json`
+
+    Expected file name : `{config_root}\buckets\my-test-bucket.json.json`
+    
+    I/O is not possible in this bucket. This issue is only reproducible in older versions(5.18.0 and older).
+    Issue fixed version is <TBD>
+    
+    solution :
+    * Recreate the bucket with same name in new version
+    ```
+    sudo noobaa-cli bucket add --name my-bucket1.json --owner <owner_name> --path <path> 
+    ```
+    * Delete invalid bucket
+    ```
+    bucket delete --name my-bucket1 (bucket name without suffix `.json`)
+    ```
+
+2. Account with suffix `.symlink`
+    
+    When the customer creates an account with the suffix `.symlink`, suffix is removed from the account config file name. 
+    
+    For example 
+    ```
+    sudo noobaa-cli account add --name acc_symlink.symlink --new_buckets_path <path> --uid <uid> --gid <gid>
+    ```
+    Actual file name : `{config_root}\accounts_by_name\acc_symlink.symlink`
+    
+    Expected file name : `{config_root}\accounts_by_name\acc_symlink.symlink.symlink`
+    
+    This issue is only reproducible in older versions(5.18.0 and older).
+    Issue fixed version is <TBD>
+    
+    solution :
+    * Recreate the account with same name in new version
+    ```
+    sudo noobaa-cli account add --name acc_symlink.symlink --new_buckets_path <path> --uid <uid> --gid <gid>
+    ```
+    * Delete invalid account
+    ```
+    sudo noobaa-cli account delete --name acc_symlink (account name without suffix `.symlink`)
+    ```
+

src/endpoint/s3/ops/s3_get_object.js

@@ -48,7 +48,7 @@ async function get_object(req, res) {
             throw new S3Error(S3Error.InvalidObjectState);
         }
     }
-
+    http_utils.set_response_headers_from_request(req, res);
     const obj_size = object_md.size;
     const params = {
         object_md,

src/endpoint/s3/ops/s3_head_object.js

@@ -28,6 +28,7 @@ async function head_object(req, res) {
 
     s3_utils.set_response_object_md(res, object_md);
     s3_utils.set_encryption_response_headers(req, res, object_md.encryption);
+    http_utils.set_response_headers_from_request(req, res);
 }
 
 module.exports = {

src/native/fs/fs_napi.cpp

@@ -772,6 +772,11 @@ struct FSWrapWorker : public FSWorker
 
 /**
  * Stat is an fs op
+ * 
+ * Note: this stat operation contains the system call of open.
+ *       Currently, we use it in list objects, but might want to create a different stat call
+ *       (or add changes inside this) to avoid permission check during list objects
+ *       while we stat each file (to avoid EACCES error)
  */
 struct Stat : public FSWorker
 {

src/sdk/config_fs.js

@@ -103,13 +103,13 @@ class ConfigFS {
 
     /**
      * add_config_file_suffix returns the config_file_name follwed by the given suffix
+     * Bucket name can have suffix `.json`. This will make sure the bucket name and bucket config file have the same name.
      * @param {string} config_file_name
      * @param {string} suffix
      * @returns {string} 
      */
     add_config_file_suffix(config_file_name, suffix) {
         if (!config_file_name) dbg.warn(`Config file name is missing - ${config_file_name}`);
-        if (String(config_file_name).endsWith(suffix)) return config_file_name;
         return config_file_name + suffix;
     }
 

src/sdk/namespace_fs.js

@@ -713,10 +713,19 @@ class NamespaceFS {
                     if (!delimiter && r.common_prefix) {
                         await process_dir(r.key);
                     } else {
-                        if (pos < results.length) {
-                            results.splice(pos, 0, r);
-                        } else {
-                            results.push(r);
+                        const entry_path = path.join(this.bucket_path, r.key);
+                        // If entry is outside of bucket, returns stat of symbolic link
+                        const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
+                        const stat = await native_fs_utils.stat_if_exists(fs_context, entry_path,
+                            use_lstat, config.NSFS_LIST_IGNORE_ENTRY_ON_EACCES);
+                        if (stat) {
+                            r.stat = stat;
+                            // add the result only if we have the stat information
+                            if (pos < results.length) {
+                                results.splice(pos, 0, r);
+                            } else {
+                                results.push(r);
+                            }
                         }
                         if (results.length > limit) {
                             results.length = limit;
@@ -756,6 +765,11 @@ class NamespaceFS {
                     await insert_entry_to_results_arr(r);
                 };
 
+                // our current mechanism - list the files and skipping inaccessible directory (invisible in the list).
+                // We use this check_access in case the directory is not accessible inside a bucket.
+                // In a directory if we don’t have access to the directory, we want to skip the directory and its sub directories from the list.
+                // We did it outside to avoid undefined values in the cache.
+                // Note: It is not the same case as a file without permission.
                 if (!(await this.check_access(fs_context, dir_path))) return;
                 try {
                     if (list_versions) {
@@ -877,13 +891,6 @@ class NamespaceFS {
 
             const prefix_dir_key = prefix.slice(0, prefix.lastIndexOf('/') + 1);
             await process_dir(prefix_dir_key);
-            await Promise.all(results.map(async r => {
-                if (r.common_prefix) return;
-                const entry_path = path.join(this.bucket_path, r.key);
-                //If entry is outside of bucket, returns stat of symbolic link
-                const use_lstat = !(await this._is_path_in_bucket_boundaries(fs_context, entry_path));
-                r.stat = await nb_native().fs.stat(fs_context, entry_path, { use_lstat });
-            }));
             const res = {
                 objects: [],
                 common_prefixes: [],
@@ -1007,10 +1014,15 @@ class NamespaceFS {
             file_path = await this._find_version_path(fs_context, params);
             await this._check_path_in_bucket_boundaries(fs_context, file_path);
 
-            // NOTE: don't move this code after the open
-            // this can lead to ENOENT failures due to file not exists when content size is 0
-            // if entry is a directory object and its content size = 0 - return empty response
-                    if (await this._is_empty_directory_content(file_path, fs_context, params)) return null;
+                    // NOTE: don't move this code after the open
+                    // this can lead to ENOENT failures due to file not exists when content size is 0
+                    // if entry is a directory object and its content size = 0 - return empty response
+                    if (await this._is_empty_directory_content(file_path, fs_context, params)) {
+                        res.end();
+                        // since we don't write anything to the stream wait_finished might not be needed. added just in case there is a delay
+                        await stream_utils.wait_finished(res, { signal: object_sdk.abort_controller.signal });
+                        return null;
+                    }
 
             file = await nb_native().fs.open(
                 fs_context,
@@ -1427,10 +1439,10 @@ class NamespaceFS {
         // will retry renaming a file in case of parallel deleting of the destination path
         for (;;) {
             try {
-                await native_fs_utils._make_path_dirs(dest_path, fs_context);
                 if (this._is_versioning_disabled() ||
                     (this._is_versioning_enabled() && is_dir_content)) {
-                // dir_content is not supported in versioning, hence we will treat it like versioning disabled
+                    // dir_content is not supported in versioning, hence we will treat it like versioning disabled
+                    await native_fs_utils._make_path_dirs(dest_path, fs_context);
                     if (open_mode === 'wt') {
                         await target_file.linkfileat(fs_context, dest_path);
                     } else {
@@ -1478,6 +1490,8 @@ class NamespaceFS {
             try {
                 let new_ver_info;
                 let latest_ver_info;
+                // dir might be deleted by other thread. will recreacte if missing
+                await native_fs_utils._make_path_dirs(latest_ver_path, fs_context);
                 if (is_gpfs) {
                     const latest_ver_info_exist = await native_fs_utils.is_path_exists(fs_context, latest_ver_path);
                     gpfs_options = await this._open_files_gpfs(fs_context, new_ver_tmp_path, latest_ver_path, upload_file,
@@ -2558,12 +2572,32 @@ class NamespaceFS {
         }
     }
 
+    /**
+     * _delete_path_dirs deletes all the paths in the hierarchy that are empty after a successful delete
+     * if the original file_path to be deleted is a regular object which means file_path is not a directory and it's not a directory object path - 
+     * before deletion of the parent directory  - 
+     * if the parent directory is a directory object (has CONTENT_DIR xattr) - stop the deletion loop
+     * else - delete the directory - if dir is not empty it will stop at the first non empty dir
+     * NOTE - the directory object check is needed because when object size is zero we won't create a .folder file and the dir will be empty
+     * therefore the deletion will succeed although we shouldn't delete the directory object
+     * @param {String} file_path 
+     * @param {nb.NativeFSContext} fs_context 
+     */
     async _delete_path_dirs(file_path, fs_context) {
         try {
-            let dir = path.dirname(file_path);
-            while (dir !== this.bucket_path) {
-                await nb_native().fs.rmdir(fs_context, dir);
-                dir = path.dirname(dir);
+            let dir_path = path.dirname(file_path);
+            const deleted_file_is_dir = file_path.endsWith('/');
+            const deleted_file_is_dir_object = file_path.endsWith(config.NSFS_FOLDER_OBJECT_NAME);
+            let should_check_dir_path_is_content_dir = !deleted_file_is_dir && !deleted_file_is_dir_object;
+            while (dir_path !== this.bucket_path) {
+                if (should_check_dir_path_is_content_dir) {
+                    const dir_stat = await nb_native().fs.stat(fs_context, dir_path);
+                    const file_is_disabled_dir_content = dir_stat.xattr && dir_stat.xattr[XATTR_DIR_CONTENT] !== undefined;
+                    if (file_is_disabled_dir_content) break;
+                }
+                await nb_native().fs.rmdir(fs_context, dir_path);
+                dir_path = path.dirname(dir_path);
+                should_check_dir_path_is_content_dir = true;
             }
         } catch (err) {
             if (err.code !== 'ENOTEMPTY' &&
@@ -2992,10 +3026,18 @@ class NamespaceFS {
         return res;
     }
 
-    // delete version_id -
-    // 1. get version info, if it's empty - return
-    // 2. unlink key
-    // 3. if version is latest version - promote second latest -> latest
+    /**
+     * delete version_id does the following - 
+     * 1. get version info, if it's empty - return
+     * 2. unlink key
+     * 3. if version is latest version - promote second latest -> latest
+     * 4. if it's the latest version - delete the directory hirerachy of the key if it's empty
+     *    if it's a past version - delete .versions/ and the directory hirerachy if it's empty
+     * @param {nb.NativeFSContext} fs_context 
+     * @param {String} file_path 
+     * @param {Object} params 
+     * @returns {Promise<{deleted_delete_marker?: string, version_id?: string}>}
+     */
     async _delete_version_id(fs_context, file_path, params) {
         // TODO optimization - GPFS link overrides, no need to unlink before promoting, but if there is nothing to promote we should unlink
         const del_obj_version_info = await this._delete_single_object_versioned(fs_context, params.key, params.version_id);

src/test/unit_tests/jest_tests/test_nc_nsfs_account_cli.test.js

@@ -570,6 +570,46 @@ describe('manage nsfs cli account flow', () => {
             const res = await exec_manage_cli(type, action, account_options);
             expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.InvalidSupplementalGroupsList.code);
         });
+
+        it('cli account add - with account name suffix `.json`', async function() {
+            const action = ACTIONS.ADD;
+            const { type, new_buckets_path, uid, gid } = defaults;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            const name = 'account_suffix.json';
+            const account_options = { config_root, name, new_buckets_path, uid, gid };
+            const res = await exec_manage_cli(type, action, account_options);
+            const parse_res = JSON.parse(res);
+            expect(parse_res.response.code).toEqual(ManageCLIResponse.AccountCreated.code);
+            expect(parse_res.response.reply.name).toEqual(name);
+            const account = await config_fs.get_account_by_name(name);
+            expect(account.name).toBe(name);
+            const res1 = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.STATUS, { name, config_root });
+            expect(JSON.parse(res1).response.reply.name).toBe(name);
+            const res_list = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.LIST, { config_root });
+            expect(JSON.parse(res_list).response.reply.map(item => item.name))
+                .toEqual(expect.arrayContaining([name]));
+        });
+
+        it('cli account add - with account name suffix `.symlink`', async function() {
+            const action = ACTIONS.ADD;
+            const { type, new_buckets_path, uid, gid } = defaults;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            const name = 'account_suffix.symlink';
+            const account_options = { config_root, name, new_buckets_path, uid, gid };
+            const res = await exec_manage_cli(type, action, account_options);
+            const parse_res = JSON.parse(res);
+            expect(parse_res.response.code).toEqual(ManageCLIResponse.AccountCreated.code);
+            expect(parse_res.response.reply.name).toEqual(name);
+            const account = await config_fs.get_account_by_name(name);
+            expect(account.name).toBe(name);
+            const res1 = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.STATUS, { name, config_root });
+            expect(JSON.parse(res1).response.reply.name).toBe(name);
+            const res_list = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.LIST, { config_root });
+            expect(JSON.parse(res_list).response.reply.map(item => item.name))
+                .toEqual(expect.arrayContaining([name]));
+        });
     });
 
     describe('cli update account', () => {

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js

@@ -27,7 +27,8 @@ describe('manage nsfs cli bucket flow', () => {
         const config_root = path.join(tmp_fs_path, 'config_root_manage_nsfs');
         const config_fs = new ConfigFS(config_root);
         const root_path = path.join(tmp_fs_path, 'root_path_manage_nsfs/');
-        const bucket_storage_path = path.join(tmp_fs_path, 'root_path_manage_nsfs', 'bucket1');
+        const bucket_storage_path = path.join(root_path, 'bucket1');
+        const bucket_json_storage_path = path.join(root_path, 'bucket3.json');
         set_nc_config_dir_in_config(config_root);
 
         const account_defaults = {
@@ -54,6 +55,12 @@ describe('manage nsfs cli bucket flow', () => {
             path: bucket_storage_path,
         };
 
+        const bucket_suffix_json = {
+            name: 'bucket3.json',
+            owner: account_defaults.name,
+            path: bucket_json_storage_path,
+        };
+
         beforeEach(async () => {
             await fs_utils.create_fresh_path(root_path);
             await fs_utils.create_fresh_path(bucket_storage_path);
@@ -242,6 +249,22 @@ describe('manage nsfs cli bucket flow', () => {
             await assert_bucket(bucket, bucket_options, config_fs);
             expect(bucket.s3_policy).toStrictEqual(bucket_policy);
         });
+
+        it('cli create bucket - with bucket name suffix `.json`', async () => {
+            await fs_utils.create_fresh_path(bucket_json_storage_path);
+            await fs_utils.file_must_exist(bucket_json_storage_path);
+            const action = ACTIONS.ADD;
+            const bucket_options = { config_root, ...bucket_suffix_json };
+            await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
+            const bucket = await config_fs.get_bucket_by_name(bucket_suffix_json.name);
+            expect(bucket).toBeDefined();
+            expect(bucket.name).toBe(bucket_options.name);
+            const res = await exec_manage_cli(TYPES.BUCKET, ACTIONS.STATUS, { name: bucket_suffix_json.name, config_root });
+            expect(JSON.parse(res).response.reply.name).toBe(bucket_options.name);
+            const res_list = await exec_manage_cli(TYPES.BUCKET, ACTIONS.LIST, { config_root });
+            expect(JSON.parse(res_list).response.reply.map(item => item.name))
+                .toEqual(expect.arrayContaining([bucket_options.name]));
+        });
     });
 
     describe('cli create bucket using from_file', () => {