From 12c6d90e22951493a53366bc340934b48dc1898e Mon Sep 17 00:00:00 2001 From: shirady <57721533+shirady@users.noreply.github.com> Date: Wed, 1 Jan 2025 15:15:25 +0200 Subject: [PATCH] NC | NSFS | Add stat to account_id_cache 1. In config.js move the definition of ACCOUNTS_ID_CACHE_EXPIRY near the the configurations of OBJECT_SDK_BUCKET_CACHE_EXPIRY_MS and OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS. 2. In accountspace_fs.js: - Remove the Number conversion as the value is number (was Number(config.ACCOUNTS_ID_CACHE_EXPIRY)). - Change the JSDoc of params so it will be easier to read. - Change the load inside function to be get_identity_by_id_and_stat_file so we will have the identity with the stat as property inside it. - Add the validate method, and add the functions _validate_account_id and check_same_stat_account that were partially copied from object_sdk. 3. In config_fs add the function get_identity_by_id_and_stat_file and stat_account_config_file_by_identity that would try to stat the account config file (covering the identity, accounts_by_name - new path, accounts - old path). 4. In object_sdk.js add a new line between anonymous_access_key bucket_namespace_cache for easier readability, fix a typo in a comment. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com> --- config.js | 9 +- src/sdk/accountspace_fs.js | 55 +++++++++++-- src/sdk/config_fs.js | 75 ++++++++++++++++- src/sdk/object_sdk.js | 3 +- .../test_nc_with_a_couple_of_forks.js | 82 ++++++++++++++++++- 5 files changed, 210 insertions(+), 14 deletions(-) diff --git a/config.js b/config.js index dbea63a3ec..17a03f06d2 100644 --- a/config.js +++ b/config.js @@ -635,16 +635,24 @@ config.REMOTE_NOOAA_NAMESPACE = `remote-${config.KUBE_APP_LABEL}`; /////////////////////////////// config.INLINE_MAX_SIZE = 4096; +/////////////////////////////// +// CACHE (ACCOUNT, BUCKET) // +/////////////////////////////// + // Object SDK bucket cache expiration time config.OBJECT_SDK_BUCKET_CACHE_EXPIRY_MS = 60000; // Object SDK account cache expiration time config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS = Number(process.env.ACCOUNTS_CACHE_EXPIRY) || 10 * 60 * 1000; // TODO: Decide on a time that we want to invalidate +// Accountspace_fs account id cache expiration time +config.ACCOUNTS_ID_CACHE_EXPIRY = 3 * 60 * 1000; // TODO: Decide on a time that we want to invalidate // Object SDK bucket_namespace_cache allow stat of the config file config.NC_ENABLE_BUCKET_NS_CACHE_STAT_VALIDATION = true; // Object SDK account_cache allow stat of the config file config.NC_ENABLE_ACCOUNT_CACHE_STAT_VALIDATION = true; +// accountspace_fs allow stat of the config file +config.NC_ENABLE_ACCOUNT_ID_CACHE_STAT_VALIDATION = true; ////////////////////////////// // OPERATOR RELATED // @@ -932,7 +940,6 @@ config.NC_DISABLE_HEALTH_ACCESS_CHECK = false; config.NC_DISABLE_POSIX_MODE_ACCESS_CHECK = true; config.NC_DISABLE_SCHEMA_CHECK = false; -config.ACCOUNTS_ID_CACHE_EXPIRY = 3 * 60 * 1000; ////////// GPFS ////////// config.GPFS_DOWN_DELAY = 1000; diff --git a/src/sdk/accountspace_fs.js b/src/sdk/accountspace_fs.js index 8d5b92c913..969d6a2dee 100644 --- a/src/sdk/accountspace_fs.js +++ b/src/sdk/accountspace_fs.js @@ -28,22 +28,61 @@ const dummy_service_name = 's3'; const account_id_cache = new LRUCache({ name: 'AccountIDCache', - // TODO: Decide on a time that we want to invalidate - expiry_ms: Number(config.ACCOUNTS_ID_CACHE_EXPIRY), + expiry_ms: config.ACCOUNTS_ID_CACHE_EXPIRY, make_key: ({ _id }) => _id, /** * Accounts are added to the cache based on id, Default value for show_secrets and decrypt_secret_key will be true, * and show_secrets and decrypt_secret_key `false` only when we load cache from the health script, * health script doesn't need to fetch or decrypt the secret. - * @param {{ _id: string, - * show_secrets?: boolean, - * decrypt_secret_key?: boolean, - * config_fs: import('./config_fs').ConfigFS }} params + * @param {{ + * _id: string, + * show_secrets?: boolean, + * decrypt_secret_key?: boolean, + * config_fs: ConfigFS + * }} params */ - load: async ({ _id, show_secrets = true, decrypt_secret_key = true, config_fs}) => - config_fs.get_identity_by_id(_id, CONFIG_TYPES.ACCOUNT, { show_secrets: show_secrets, decrypt_secret_key: decrypt_secret_key }), + load: async ({ _id, show_secrets = true, decrypt_secret_key = true, config_fs }) => + config_fs.get_identity_by_id_and_stat_file( + _id, CONFIG_TYPES.ACCOUNT, { show_secrets: show_secrets, decrypt_secret_key: decrypt_secret_key }), + validate: (params, data) => _validate_account_id(params, data), }); +/** _validate_account_id is an additional layer (to expiry_ms) + * to checks the stat of the config file + * @param {object} data + * @param {object} params + * @returns Promise<{boolean>} + */ +async function _validate_account_id(params, data) { + if (config.NC_ENABLE_ACCOUNT_ID_CACHE_STAT_VALIDATION) { + const same_stat = await check_same_stat_account(params._id, params.name, params.stat, data.config_fs); + if (!same_stat) { // config file of account was changed + return false; + } + } + return true; +} + + /** + * check_same_stat_account will return true the config file was not changed + * in case we had any issue (for example error during stat) the returned value will be undefined + * @param {string} id + * @param {string} account_name + * @param {nb.NativeFSStats} account_stat + * @param {ConfigFS} config_fs + * @returns Promise<{boolean|undefined>} + */ + async function check_same_stat_account(id, account_name, account_stat, config_fs) { + if (!config_fs) return; + try { + const current_stat = await config_fs.stat_account_config_file_by_identity(id, account_name); + if (current_stat) { + return current_stat.ino === account_stat.ino && current_stat.mtimeNsBigint === account_stat.mtimeNsBigint; + } + } catch (err) { + dbg.warn('check_same_stat_account: current_stat got an error', err, 'ignoring...'); + } + } /** * @param {Object} requested_account diff --git a/src/sdk/config_fs.js b/src/sdk/config_fs.js index 914c790ed0..1ed9c71659 100644 --- a/src/sdk/config_fs.js +++ b/src/sdk/config_fs.js @@ -378,6 +378,29 @@ class ConfigFS { return identity; } + /** + * get_identity_by_id_and_stat_file returns the full account/user data and stat the file: + * @param {string} id + * @param {string} [type] + * @param {{show_secrets?: boolean, decrypt_secret_key?: boolean, silent_if_missing?: boolean}} [options] + * @returns {Promise} + */ + async get_identity_by_id_and_stat_file(id, type, options = {}) { + let identity; + try { + identity = await this.get_identity_by_id(id, type, options); + if (!identity && options.silent_if_missing) { + return undefined; // we don't have the identity, so we can't add the property of stat to it + } + const stat = await this.stat_account_config_file_by_identity(id, identity.name); + identity.stat = stat; + return identity; // this identity object should have also a stat property + } catch (err) { + dbg.error('get_identity_by_id_and_stat_file: got an error', err); + throw err; + } + } + /** * search_accounts_by_id searches old accounts directory and finds an account that its _id matches the given id param * @param {string} id @@ -450,7 +473,7 @@ class ConfigFS { } /** - * stat_account_config_file will return the stat output on account config file + * stat_account_config_file will return the stat output on account config file by access key * please notice that stat might throw an error - you should wrap it with try-catch and handle the error * Note: access_key type of anonymous_access_key is a symbol, otherwise it is a string (not SensitiveString) * @param {Symbol|string} access_key @@ -468,6 +491,38 @@ class ConfigFS { return nb_native().fs.stat(this.fs_context, path_for_account_or_user_config_file); } + /** + * stat_account_config_file_by_identity will return the stat output on account config file by id or by account name + * 1. try by identity path + * 2. if not found - try by account name with new accounts path + * 3. if not found - try by account name with old accounts path + * 4. else throw an error + * @param {string} id + * @param {string} account_name + * @returns {Promise} + */ + async stat_account_config_file_by_identity(id, account_name) { + const options = { silent_if_missing: true }; + + const identity_path = this.get_identity_path_by_id(id); + dbg.log2('stat_account_config_file_by_identity: will try to stat by identity (identities path)'); + let stat = await this.stat_config_file_general_use(identity_path, options); + if (stat) return stat; + + dbg.log2('stat_account_config_file_by_identity: will try to stat by account name (new accounts path)'); + const account_name_new_path = this.get_account_path_by_name(account_name); + stat = await this.stat_config_file_general_use(account_name_new_path, options); + if (stat) return stat; + + dbg.log2('stat_account_config_file_by_identity: will try to stat by account name (old accounts path)'); + const account_name_old_path = this._get_old_account_path_by_name(account_name); + stat = await this.stat_config_file_general_use(account_name_old_path, options); + if (stat) return stat; + + dbg.error(`stat_account_config_file_by_identity: could not stat identity by id ${id} or by account name ${account_name} (new and old accounts path)`); + throw new Error('Could not stat account (identities path, new accounts path and old accounts path)'); + } + /** * is_account_exists_by_name returns true if account config path exists in config dir * if account does not exist and it's a regular account (not an IAM user) @@ -1255,6 +1310,24 @@ class ConfigFS { get_hosts_data(system_data) { return _.omit(system_data, 'config_directory'); } + + /** stat_config_file_general_use will stat a file by a path + * if the file is not found: + * - using the options with silent_if_missing true on ENOENT it would return undefined + * - else it would throw an error + * @param {string} path_to_stat + * @param {{ silent_if_missing: any; }} options + */ + async stat_config_file_general_use(path_to_stat, options) { + try { + const stat_by_identity_path = await nb_native().fs.stat(this.fs_context, path_to_stat); + return stat_by_identity_path; + } catch (err) { + if (err.code === 'ENOENT' && options.silent_if_missing) return; + dbg.error('stat_account_config_file_by_identity: could not stat by identity ID, got an error', err); + throw err; + } + } } // EXPORTS diff --git a/src/sdk/object_sdk.js b/src/sdk/object_sdk.js index 031ec858e3..ae10d267cf 100644 --- a/src/sdk/object_sdk.js +++ b/src/sdk/object_sdk.js @@ -27,6 +27,7 @@ const BucketSpaceNB = require('./bucketspace_nb'); const { RpcError } = require('../rpc'); const anonymous_access_key = Symbol('anonymous_access_key'); + const bucket_namespace_cache = new LRUCache({ name: 'ObjectSDK-Bucket-Namespace-Cache', // This is intentional. Cache entry expiration is handled by _validate_bucket_namespace(). @@ -93,7 +94,7 @@ async function _validate_account(data, params) { const bs_allow_stat_account = Boolean(bs.check_same_stat_account); if (bs_allow_stat_account && config.NC_ENABLE_ACCOUNT_CACHE_STAT_VALIDATION) { const same_stat = await bs.check_same_stat_account(params.access_key, data.stat); - if (!same_stat) { // config file of bucket was changed + if (!same_stat) { // config file of account was changed return false; } } diff --git a/src/test/unit_tests/test_nc_with_a_couple_of_forks.js b/src/test/unit_tests/test_nc_with_a_couple_of_forks.js index f6158155d8..6d2759cefc 100644 --- a/src/test/unit_tests/test_nc_with_a_couple_of_forks.js +++ b/src/test/unit_tests/test_nc_with_a_couple_of_forks.js @@ -9,8 +9,7 @@ const P = require('../../util/promise'); const mocha = require('mocha'); const assert = require('assert'); const fs_utils = require('../../util/fs_utils'); -const { TMP_PATH, generate_nsfs_account, get_new_buckets_path_by_test_env, generate_s3_client, - get_coretest_path, exec_manage_cli } = require('../system_tests/test_utils'); +const { TMP_PATH, generate_nsfs_account, get_new_buckets_path_by_test_env, generate_s3_client, get_coretest_path, exec_manage_cli } = require('../system_tests/test_utils'); const { TYPES, ACTIONS } = require('../../manage_nsfs/manage_nsfs_constants'); const ManageCLIResponse = require('../../manage_nsfs/manage_nsfs_cli_responses').ManageCLIResponse; @@ -18,7 +17,8 @@ const coretest_path = get_coretest_path(); const coretest = require(coretest_path); const setup_options = { forks: 2, debug: 5 }; coretest.setup(setup_options); -const { rpc_client, EMAIL, get_current_setup_options, stop_nsfs_process, start_nsfs_process, config_dir_name } = coretest; +const { rpc_client, EMAIL, get_current_setup_options, stop_nsfs_process, start_nsfs_process, + config_dir_name, NC_CORETEST_CONFIG_FS, NC_CORETEST_STORAGE_PATH } = coretest; const CORETEST_ENDPOINT = coretest.get_http_address(); @@ -52,6 +52,7 @@ mocha.describe('operations with a couple of forks', async function() { mocha.after(async () => { fs_utils.folder_delete(`${config_root}`); + fs_utils.folder_delete(`${new_bucket_path_param}`); }); mocha.it('versioning change with a couple of forks', async function() { @@ -147,4 +148,79 @@ mocha.describe('operations with a couple of forks', async function() { // cleanup await s3_uid5_after_access_keys_update.deleteBucket({ Bucket: bucket_name2 }); }); + + mocha.it('head a bucket after account update (change fs_backend)', async function() { + // create additional account + const account_name = 'Oliver'; + const account_options_create = { account_name, uid: 6001, gid: 6001, config_root: config_dir_name }; + await fs_utils.create_fresh_path(new_bucket_path_param); + await fs.promises.chown(new_bucket_path_param, account_options_create.uid, account_options_create.gid); + await fs.promises.chmod(new_bucket_path_param, 0o700); + const access_details = await generate_nsfs_account(rpc_client, EMAIL, new_bucket_path_param, account_options_create); + // check the account status + const account_options_status = { config_root: config_dir_name, name: account_name}; + const res_account_status = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.STATUS, account_options_status); + assert.equal(JSON.parse(res_account_status).response.code, ManageCLIResponse.AccountStatus.code); + // generate the s3 client +const s3_uid6001 = generate_s3_client(access_details.access_key, + access_details.secret_key, CORETEST_ENDPOINT); + // check the connection for the new account (can be any of the forks) + const res_list_buckets = await s3_uid6001.listBuckets({}); + assert.equal(res_list_buckets.$metadata.httpStatusCode, 200); + // create a bucket + const bucket_name3 = 'bucket3'; + const res_bucket_create = await s3_uid6001.createBucket({ Bucket: bucket_name3 }); + assert.equal(res_bucket_create.$metadata.httpStatusCode, 200); + // head the bucket + const res_head_bucket1 = await s3_uid6001.headBucket({Bucket: bucket_name3}); + assert.equal(res_head_bucket1.$metadata.httpStatusCode, 200); + // update the account + const account_options_update = { config_root: config_dir_name, name: account_name, fs_backend: 'GPFS'}; + const res_account_update = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.UPDATE, account_options_update); + assert.equal(JSON.parse(res_account_update).response.code, ManageCLIResponse.AccountUpdated.code); + // head the bucket (again) + const res_head_bucket2 = await s3_uid6001.headBucket({Bucket: bucket_name3}); + assert.equal(res_head_bucket2.$metadata.httpStatusCode, 200); + + // cleanup + await s3_uid6001.deleteBucket({ Bucket: bucket_name3 }); + }); + + mocha.it('create a bucket after account update (change buckets_path)', async function() { + // create an additional account + const account_name = 'John'; + const account_options_create = { account_name, uid: 7001, gid: 7001, config_root: config_dir_name }; + // reuse NC_CORETEST_STORAGE_PATH as new_buckets_path (no need for create fresh path, chmod and chown) + const access_details = await generate_nsfs_account(rpc_client, EMAIL, NC_CORETEST_STORAGE_PATH, account_options_create); + // check the account status + const account_options_status = { config_root: config_dir_name, name: account_name}; + const res_account_status = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.STATUS, account_options_status); + assert.equal(JSON.parse(res_account_status).response.code, ManageCLIResponse.AccountStatus.code); + // generate the s3 client + const s3_uid6001 = generate_s3_client(access_details.access_key, + access_details.secret_key, CORETEST_ENDPOINT); + // check the connection for the new account (can be any of the forks) + const res_list_buckets = await s3_uid6001.listBuckets({}); + assert.equal(res_list_buckets.$metadata.httpStatusCode, 200); + // update the account - change its new_bucket_path + const new_bucket_path_param2 = path.join(TMP_PATH, 'nc_coretest_storage_root_path2/'); + await fs_utils.create_fresh_path(new_bucket_path_param2); + await fs.promises.chown(new_bucket_path_param2, account_options_create.uid, account_options_create.gid); + await fs.promises.chmod(new_bucket_path_param2, 0o700); + const account_options_update = { config_root: config_dir_name, name: account_name, new_buckets_path: new_bucket_path_param2}; + const res_account_update = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.UPDATE, account_options_update); + assert.equal(JSON.parse(res_account_update).response.code, ManageCLIResponse.AccountUpdated.code); + // create a bucket + const bucket_name4 = 'bucket4'; + const res_bucket_create = await s3_uid6001.createBucket({ Bucket: bucket_name4 }); + assert.equal(res_bucket_create.$metadata.httpStatusCode, 200); + // validate the bucket was created in the updated path + const bucket4 = await NC_CORETEST_CONFIG_FS.get_bucket_by_name(bucket_name4); + const expected_bucket_path = path.join(new_bucket_path_param2, bucket_name4); + assert.equal(bucket4.path, expected_bucket_path); + + // cleanup + await s3_uid6001.deleteBucket({ Bucket: bucket_name4 }); + await fs.promises.rm(new_bucket_path_param2, { recursive: true }); + }); });