NSFS | NC | Handle concurrency when reading entries and they deleted by another process

1. fix concurrency issue on delete account (when a bucket was deleted during deletion)
2. fix concurrency issue on list buckets (when a bucket was deleted during deletion)
3. fix concurrency issue on s3 api list buckets (when a bucket was deleted during deletion)
4. add TODO in accountspace_fs
5. add test (not related to the issue, was added during debug)

Signed-off-by: shirady <[email protected]>
(cherry picked from commit c42e64d)

Author: shirady

src/cmd/manage_nsfs.js

@@ -20,7 +20,7 @@ const { print_usage } = require('../manage_nsfs/manage_nsfs_help_utils');
 const { TYPES, ACTIONS, LIST_ACCOUNT_FILTERS, LIST_BUCKET_FILTERS,
     GLACIER_ACTIONS } = require('../manage_nsfs/manage_nsfs_constants');
 const { throw_cli_error, write_stdout_response, get_config_file_path, get_symlink_config_file_path,
-    get_config_data, get_boolean_or_string_value} = require('../manage_nsfs/manage_nsfs_cli_utils');
+    get_config_data, get_boolean_or_string_value, get_config_data_if_exists } = require('../manage_nsfs/manage_nsfs_cli_utils');
 const { validate_input_types, validate_bucket_args, validate_account_args,
         verify_delete_account, validate_whitelist_arg, verify_whitelist_ips,
         _validate_access_keys } = require('../manage_nsfs/manage_nsfs_validations');
@@ -613,7 +613,8 @@ async function list_config_files(type, config_path, wide, show_secrets, filters)
         if (entry.name.endsWith('.json')) {
             if (wide || should_filter) {
                 const full_path = path.join(config_path, entry.name);
-                const data = await get_config_data(config_root_backend, full_path, show_secrets || should_filter);
+                const data = await get_config_data_if_exists(config_root_backend, full_path, show_secrets || should_filter);
+                if (!data) return undefined;
                 // decryption causing mkm initalization
                 // decrypt only if data has access_keys and show_secrets = true (no need to decrypt if show_secrets = false but should_filter = true)
                 if (data.access_keys && show_secrets) data.access_keys = await nc_mkm.decrypt_access_keys(data);
@@ -626,6 +627,7 @@ async function list_config_files(type, config_path, wide, show_secrets, filters)
         }
     });
     // it inserts undefined for the entry '.noobaa-config-nsfs' and we wish to remove it
+    // in case the entry was deleted during the list it also inserts undefined
     config_files_list = config_files_list.filter(item => item);
 
     return config_files_list;

src/manage_nsfs/manage_nsfs_cli_utils.js

@@ -1,6 +1,7 @@
 /* Copyright (C) 2024 NooBaa */
 'use strict';
 
+const dbg = require('../util/debug_module')(__filename);
 const _ = require('lodash');
 const path = require('path');
 const nb_native = require('../util/nb_native');
@@ -53,6 +54,23 @@ async function get_config_data(config_root_backend, config_file_path, show_secre
     return config_data;
 }
 
+/**
+ * get_config_data_if_exists will read a config file and return its content 
+ * while omitting secrets if show_secrets flag was not provided
+ * if the config file was deleted (encounter ENOENT error) - continue (returns undefined)
+ * @param {string} config_file_path
+ * @param {boolean} [show_secrets]
+ */
+async function get_config_data_if_exists(config_root_backend, config_file_path, show_secrets = false) {
+    try {
+        const config_data = await get_config_data(config_root_backend, config_file_path, show_secrets);
+        return config_data;
+    } catch (err) {
+        dbg.warn('get_config_data_if_exists: with config_file_path', config_file_path, 'got an error', err);
+        if (err.code !== 'ENOENT') throw err;
+    }
+}
+
 /**
  * get_bucket_owner_account will return the account of the bucket_owner
  * otherwise it would throw an error
@@ -118,3 +136,5 @@ exports.get_boolean_or_string_value = get_boolean_or_string_value;
 exports.get_config_data = get_config_data;
 exports.get_bucket_owner_account = get_bucket_owner_account;
 exports.get_options_from_file = get_options_from_file;
+exports.get_config_data_if_exists = get_config_data_if_exists;
+

src/manage_nsfs/manage_nsfs_validations.js

@@ -12,7 +12,7 @@ const native_fs_utils = require('../util/native_fs_utils');
 const ManageCLIError = require('../manage_nsfs/manage_nsfs_cli_errors').ManageCLIError;
 const bucket_policy_utils = require('../endpoint/s3/s3_bucket_policy_utils');
 const { throw_cli_error, get_config_file_path, get_bucket_owner_account,
-    get_config_data, get_options_from_file } = require('../manage_nsfs/manage_nsfs_cli_utils');
+    get_options_from_file, get_config_data_if_exists } = require('../manage_nsfs/manage_nsfs_cli_utils');
 const { TYPES, ACTIONS, VALID_OPTIONS, OPTION_TYPE, FROM_FILE, BOOLEAN_STRING_VALUES,
     GLACIER_ACTIONS, LIST_UNSETABLE_OPTIONS } = require('../manage_nsfs/manage_nsfs_constants');
 
@@ -335,11 +335,12 @@ function _validate_access_keys(access_key, secret_key) {
 async function verify_delete_account(config_root_backend, buckets_dir_path, account_name) {
     const fs_context = native_fs_utils.get_process_fs_context(config_root_backend);
     const entries = await nb_native().fs.readdir(fs_context, buckets_dir_path);
+    let data;
     await P.map_with_concurrency(10, entries, async entry => {
         if (entry.name.endsWith('.json')) {
             const full_path = path.join(buckets_dir_path, entry.name);
-            const data = await get_config_data(config_root_backend, full_path);
-            if (data.bucket_owner === account_name) {
+            data = await get_config_data_if_exists(config_root_backend, full_path);
+            if (data && data.bucket_owner === account_name) {
                 const detail_msg = `Account ${account_name} has bucket ${data.name}`;
                 throw_cli_error(ManageCLIError.AccountDeleteForbiddenHasBuckets, detail_msg);
             }

src/sdk/bucketspace_fs.js

@@ -228,7 +228,15 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
                 return;
             }
             const bucket_name = this.get_bucket_name(entry.name);
-            const bucket = await object_sdk.read_bucket_sdk_config_info(bucket_name);
+            let bucket;
+            try {
+                bucket = await object_sdk.read_bucket_sdk_config_info(bucket_name);
+            } catch (err) {
+                dbg.warn('list_buckets: read_bucket_sdk_config_info of bucket', bucket_name, 'got an error', err);
+                // in case the config file was deleted during the bucket list - we will continue
+                if (err.rpc_code !== 'NO_SUCH_BUCKET') throw err;
+            }
+            if (!bucket) return;
             const bucket_policy_accessible = await this.has_bucket_action_permission(bucket, account, 's3:ListBucket');
             if (!bucket_policy_accessible) return;
             const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);

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

@@ -925,6 +925,13 @@ describe('manage nsfs cli account flow', () => {
             expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.MissingIdentifier.code);
         });
 
+        it('cli list filter by access key (non existing) and name (of account3) - (none)', async () => {
+            const account_options = { config_root, name: 'account3', access_key: 'non-existing-access-key' };
+            const action = ACTIONS.LIST;
+            const res = await exec_manage_cli(TYPES.ACCOUNT, action, account_options);
+            expect(JSON.parse(res).response.reply.map(item => item.name))
+                .toEqual([]);
+        });
     });
 
     describe('cli delete account', () => {