Skip to content

Commit 7bc814b

Browse files
authored
Merge pull request #8585 from shirady/nsfs-nc-account-cache-with-stat
NC | NSFS | Add stat to `account_cache`
2 parents 5bb3175 + b2ce224 commit 7bc814b

File tree

7 files changed

+161
-13
lines changed

7 files changed

+161
-13
lines changed

config.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,9 +637,14 @@ config.INLINE_MAX_SIZE = 4096;
637637

638638
// Object SDK bucket cache expiration time
639639
config.OBJECT_SDK_BUCKET_CACHE_EXPIRY_MS = 60000;
640+
// Object SDK account cache expiration time
641+
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
642+
640643

641644
// Object SDK bucket_namespace_cache allow stat of the config file
642645
config.NC_ENABLE_BUCKET_NS_CACHE_STAT_VALIDATION = true;
646+
// Object SDK account_cache allow stat of the config file
647+
config.NC_ENABLE_ACCOUNT_CACHE_STAT_VALIDATION = true;
643648

644649
//////////////////////////////
645650
// OPERATOR RELATED //

src/sdk/bucketspace_fs.js

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
6666
return fs_context;
6767
}
6868

69+
// TODO: account function should be handled in accountspace_fs
6970
async read_account_by_access_key({ access_key }) {
7071
try {
7172
if (!access_key) throw new Error('no access key');
@@ -85,6 +86,12 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
8586
if (account.nsfs_account_config.distinguished_name) {
8687
account.nsfs_account_config.distinguished_name = new SensitiveString(account.nsfs_account_config.distinguished_name);
8788
}
89+
try {
90+
account.stat = await this.config_fs.stat_account_config_file(access_key);
91+
} catch (err) {
92+
dbg.warn(`BucketspaceFS.read_account_by_access_key could not stat_account_config_file` +
93+
`of account id: ${account._id} account name: ${account.name.unwrap()}`);
94+
}
8895
return account;
8996
} catch (err) {
9097
dbg.error('BucketSpaceFS.read_account_by_access_key: failed with error', err);
@@ -186,18 +193,39 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
186193
}
187194

188195
/**
189-
* check_same_stat will return true the config file was not changed
196+
* check_same_stat_bucket will return true the config file was not changed
197+
* in case we had any issue (for example error during stat) the returned value will be undefined
198+
* @param {string} bucket_name
190199
* @param {nb.NativeFSStats} bucket_stat
191-
* @returns Promise<{boolean>}
200+
* @returns Promise<{boolean|undefined>}
192201
*/
193-
async check_same_stat(bucket_name, bucket_stat) {
202+
async check_same_stat_bucket(bucket_name, bucket_stat) {
194203
try {
195204
const current_stat = await this.config_fs.stat_bucket_config_file(bucket_name);
196205
if (current_stat) {
197206
return current_stat.ino === bucket_stat.ino && current_stat.mtimeNsBigint === bucket_stat.mtimeNsBigint;
198207
}
199208
} catch (err) {
200-
dbg.warn('check_same_stat: current_stat got an error', err, 'ignoring...');
209+
dbg.warn('check_same_stat_bucket: current_stat got an error', err, 'ignoring...');
210+
}
211+
}
212+
213+
/**
214+
* check_same_stat_account will return true the config file was not changed
215+
* in case we had any issue (for example error during stat) the returned value will be undefined
216+
* @param {Symbol|string} access_key
217+
* @param {nb.NativeFSStats} account_stat
218+
* @returns Promise<{boolean|undefined>}
219+
*/
220+
// TODO: account function should be handled in accountspace_fs
221+
async check_same_stat_account(access_key, account_stat) {
222+
try {
223+
const current_stat = await this.config_fs.stat_account_config_file(access_key);
224+
if (current_stat) {
225+
return current_stat.ino === account_stat.ino && current_stat.mtimeNsBigint === account_stat.mtimeNsBigint;
226+
}
227+
} catch (err) {
228+
dbg.warn('check_same_stat_account: current_stat got an error', err, 'ignoring...');
201229
}
202230
}
203231

src/sdk/config_fs.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ const { RpcError } = require('../rpc');
1717
const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
1818
const nsfs_schema_utils = require('../manage_nsfs/nsfs_schema_utils');
1919
const { version_compare } = require('../upgrade/upgrade_utils');
20+
const { anonymous_access_key } = require('./object_sdk');
2021

2122
/** @typedef {import('fs').Dirent} Dirent */
2223

@@ -448,6 +449,25 @@ class ConfigFS {
448449
return path.join(this.old_accounts_dir_path, this.json(account_name));
449450
}
450451

452+
/**
453+
* stat_account_config_file will return the stat output on account config file
454+
* please notice that stat might throw an error - you should wrap it with try-catch and handle the error
455+
* Note: access_key type of anonymous_access_key is a symbol, otherwise it is a string (not SensitiveString)
456+
* @param {Symbol|string} access_key
457+
* @returns {Promise<nb.NativeFSStats>}
458+
*/
459+
stat_account_config_file(access_key) {
460+
let path_for_account_or_user_config_file;
461+
if (typeof access_key === 'symbol' && access_key === anonymous_access_key) { // anonymous account case
462+
path_for_account_or_user_config_file = this.get_account_path_by_name(config.ANONYMOUS_ACCOUNT_NAME);
463+
} else if (typeof access_key === 'string') { // rest of the cases
464+
path_for_account_or_user_config_file = this.get_account_or_user_path_by_access_key(access_key);
465+
} else { // we should not get here
466+
throw new Error(`access_key must be a from valid type ${typeof access_key} ${access_key}`);
467+
}
468+
return nb_native().fs.stat(this.fs_context, path_for_account_or_user_config_file);
469+
}
470+
451471
/**
452472
* is_account_exists_by_name returns true if account config path exists in config dir
453473
* if account does not exist and it's a regular account (not an IAM user)

src/sdk/nb.d.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -824,7 +824,8 @@ interface BucketSpace {
824824

825825
read_account_by_access_key({ access_key: string }): Promise<any>;
826826
read_bucket_sdk_info({ name: string }): Promise<any>;
827-
check_same_stat(bucket_name: string, bucket_stat: nb.NativeFSStats); // only implemented in bucketspace_fs
827+
check_same_stat_bucket(bucket_name: string, bucket_stat: nb.NativeFSStats); // only implemented in bucketspace_fs
828+
check_same_stat_account(account_name: string|Symbol, account_stat: nb.NativeFSStats); // only implemented in bucketspace_fs
828829

829830
list_buckets(params: object, object_sdk: ObjectSDK): Promise<any>;
830831
read_bucket(params: object): Promise<any>;

src/sdk/object_sdk.js

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,10 @@ const bucket_namespace_cache = new LRUCache({
4545
validate: (data, params) => params.sdk._validate_bucket_namespace(data, params),
4646
});
4747

48+
// TODO: account_cache should be in account_sdk
4849
const account_cache = new LRUCache({
4950
name: 'AccountCache',
50-
// TODO: Decide on a time that we want to invalidate
51-
expiry_ms: Number(process.env.ACCOUNTS_CACHE_EXPIRY) || 10 * 60 * 1000,
51+
expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS,
5252
/**
5353
* Set type for the generic template
5454
* @param {{
@@ -58,6 +58,7 @@ const account_cache = new LRUCache({
5858
*/
5959
make_key: ({ access_key }) => access_key,
6060
load: async ({ bucketspace, access_key }) => bucketspace.read_account_by_access_key({ access_key }),
61+
validate: (data, params) => _validate_account(data, params),
6162
});
6263

6364
const dn_cache = new LRUCache({
@@ -79,6 +80,26 @@ const MULTIPART_NAMESPACES = [
7980
];
8081
const required_obj_properties = ['obj_id', 'bucket', 'key', 'size', 'content_type', 'etag'];
8182

83+
/** _validate_account is an additional layer (to expiry_ms)
84+
* and in NC deployment it checks the stat of the config file
85+
* @param {object} data
86+
* @param {object} params
87+
* @returns Promise<{boolean>}
88+
*/
89+
// TODO: account function should be handled in account_sdk
90+
async function _validate_account(data, params) {
91+
// stat check (only in bucketspace FS)
92+
const bs = params.bucketspace;
93+
const bs_allow_stat_account = Boolean(bs.check_same_stat_account);
94+
if (bs_allow_stat_account && config.NC_ENABLE_ACCOUNT_CACHE_STAT_VALIDATION) {
95+
const same_stat = await bs.check_same_stat_account(params.access_key, data.stat);
96+
if (!same_stat) { // config file of bucket was changed
97+
return false;
98+
}
99+
}
100+
return true;
101+
}
102+
82103
class ObjectSDK {
83104

84105
/**
@@ -298,9 +319,9 @@ class ObjectSDK {
298319
const time = Date.now();
299320
// stat check (only in bucketspace FS)
300321
const bs = this._get_bucketspace();
301-
const ns_allow_stat_bucket = Boolean(bs.check_same_stat);
302-
if (ns_allow_stat_bucket && config.NC_ENABLE_BUCKET_NS_CACHE_STAT_VALIDATION) {
303-
const same_stat = await bs.check_same_stat(params.name, data.bucket.stat);
322+
const bs_allow_stat_bucket = Boolean(bs.check_same_stat_bucket);
323+
if (bs_allow_stat_bucket && config.NC_ENABLE_BUCKET_NS_CACHE_STAT_VALIDATION) {
324+
const same_stat = await bs.check_same_stat_bucket(params.name, data.bucket.stat);
304325
if (!same_stat) { // config file of bucket was changed
305326
return false;
306327
}

src/test/unit_tests/nc_coretest.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ const config_dir_name = 'nc_coretest_config_root_path';
1818
const master_key_location = `${TMP_PATH}/${config_dir_name}/master_keys.json`;
1919
const NC_CORETEST_CONFIG_DIR_PATH = `${TMP_PATH}/${config_dir_name}`;
2020
process.env.DEBUG_MODE = 'true';
21-
process.env.ACCOUNTS_CACHE_EXPIRY = '1';
21+
// process.env.ACCOUNTS_CACHE_EXPIRY = '1'; In NC we check if the config file was changed as validation
2222
process.env.NC_CORETEST = 'true';
2323

2424
require('../../util/dotenv').load();

src/test/unit_tests/test_nc_with_a_couple_of_forks.js

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,24 @@
11
/* Copyright (C) 2024 NooBaa */
2+
/* eslint-disable max-statements */
23
'use strict';
34

45
const path = require('path');
56
const _ = require('lodash');
7+
const fs = require('fs');
68
const P = require('../../util/promise');
79
const mocha = require('mocha');
810
const assert = require('assert');
911
const fs_utils = require('../../util/fs_utils');
10-
const { TMP_PATH, generate_nsfs_account, get_new_buckets_path_by_test_env, generate_s3_client, get_coretest_path } = require('../system_tests/test_utils');
12+
const { TMP_PATH, generate_nsfs_account, get_new_buckets_path_by_test_env, generate_s3_client,
13+
get_coretest_path, exec_manage_cli } = require('../system_tests/test_utils');
14+
const { TYPES, ACTIONS } = require('../../manage_nsfs/manage_nsfs_constants');
15+
const ManageCLIResponse = require('../../manage_nsfs/manage_nsfs_cli_responses').ManageCLIResponse;
1116

1217
const coretest_path = get_coretest_path();
1318
const coretest = require(coretest_path);
1419
const setup_options = { forks: 2, debug: 5 };
1520
coretest.setup(setup_options);
16-
const { rpc_client, EMAIL, get_current_setup_options, stop_nsfs_process, start_nsfs_process } = coretest;
21+
const { rpc_client, EMAIL, get_current_setup_options, stop_nsfs_process, start_nsfs_process, config_dir_name } = coretest;
1722

1823
const CORETEST_ENDPOINT = coretest.get_http_address();
1924

@@ -74,4 +79,72 @@ mocha.describe('operations with a couple of forks', async function() {
7479
// cleanup
7580
await s3_admin.deleteBucket({ Bucket: bucket_name });
7681
});
82+
83+
mocha.it('list buckets after regenerate access keys', async function() {
84+
// create additional account
85+
const account_name = 'James';
86+
const account_options_create = { account_name, uid: 5, gid: 5, config_root: config_dir_name };
87+
await fs_utils.create_fresh_path(new_bucket_path_param);
88+
await fs.promises.chown(new_bucket_path_param, account_options_create.uid, account_options_create.gid);
89+
await fs.promises.chmod(new_bucket_path_param, 0o700);
90+
const access_details = await generate_nsfs_account(rpc_client, EMAIL, new_bucket_path_param, account_options_create);
91+
// check the account status
92+
const account_options_status = { config_root: config_dir_name, name: account_name};
93+
const res_account_status = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.STATUS, account_options_status);
94+
assert.equal(JSON.parse(res_account_status).response.code, ManageCLIResponse.AccountStatus.code);
95+
// generate the s3 client
96+
const s3_uid5_before_access_keys_update = generate_s3_client(access_details.access_key,
97+
access_details.secret_key, CORETEST_ENDPOINT);
98+
// check the connection for the new account (can be any of the forks)
99+
const res_list_buckets = await s3_uid5_before_access_keys_update.listBuckets({});
100+
assert.equal(res_list_buckets.$metadata.httpStatusCode, 200);
101+
// create a bucket
102+
const bucket_name2 = 'bucket2';
103+
const res_bucket_create = await s3_uid5_before_access_keys_update.createBucket({ Bucket: bucket_name2 });
104+
assert.equal(res_bucket_create.$metadata.httpStatusCode, 200);
105+
// update the account
106+
const account_options_update = { config_root: config_dir_name, name: account_name, regenerate: true};
107+
const res_account_update = await exec_manage_cli(TYPES.ACCOUNT, ACTIONS.UPDATE, account_options_update);
108+
const access_key_id_updated = JSON.parse(res_account_update).response.reply.access_keys[0].access_key;
109+
const secret_key_updated = JSON.parse(res_account_update).response.reply.access_keys[0].secret_key;
110+
const s3_uid5_after_access_keys_update = generate_s3_client(access_key_id_updated,
111+
secret_key_updated, CORETEST_ENDPOINT);
112+
// check the connection for the updated access keys account (can be any of the forks)
113+
const res_list_buckets3 = await s3_uid5_after_access_keys_update.listBuckets({});
114+
assert.equal(res_list_buckets3.$metadata.httpStatusCode, 200);
115+
116+
// a couple of requests with the previous access keys (all should failed)
117+
// without checking the stat the expiry is OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS
118+
let failed_operations = 0;
119+
let successful_operations = 0;
120+
const number_of_requests = 5;
121+
for (let i = 0; i < number_of_requests; i++) {
122+
try {
123+
await s3_uid5_before_access_keys_update.listBuckets({});
124+
successful_operations += 1;
125+
} catch (err) {
126+
failed_operations += 1;
127+
}
128+
}
129+
assert.equal(successful_operations, 0);
130+
assert.equal(failed_operations, number_of_requests);
131+
132+
// a couple of requests with the updated access keys (all should success)
133+
let failed_operations2 = 0;
134+
let successful_operations2 = 0;
135+
const number_of_requests2 = 5;
136+
for (let i = 0; i < number_of_requests2; i++) {
137+
try {
138+
await s3_uid5_after_access_keys_update.listBuckets({});
139+
successful_operations2 += 1;
140+
} catch (err) {
141+
failed_operations2 += 1;
142+
}
143+
}
144+
assert.equal(successful_operations2, number_of_requests2);
145+
assert.equal(failed_operations2, 0);
146+
147+
// cleanup
148+
await s3_uid5_after_access_keys_update.deleteBucket({ Bucket: bucket_name2 });
149+
});
77150
});

0 commit comments

Comments
 (0)