Nc account by id #8167

alphaprinz · 2024-06-26T03:37:30Z

Explain the changes

This PR removes references of account name from bucket json files.
The bucket_owner field is removed from bucket schema.
Principal in bucket policy is account id instead of account name.
In order to find account id by account name, a new dir root_accounts was added with symlinks from account name to account json.

Issues: Fixed #xxx / Gap #xxx

A Change to account name doesn't require updating bucket json files.

Testing Instructions:

Same tests

Doc added/updated
Tests added

shirady · 2024-06-27T06:23:56Z

src/server/system_services/schemas/nsfs_bucket_schema.js

@@ -8,7 +8,6 @@ module.exports = {
        '_id',
        'name',
        'system_owner',
-        'bucket_owner',


I think that you can rename the owner_account.
The property name is owner_account because we had duplicated information (both bucket_owner and owner_account describe the owner of the bucket) and wanted a different name.
@romayalon @guymguym @naveenpaul1 If you think otherwise, please share.

shirady · 2024-06-27T06:25:32Z

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_schema_validation.test.js

@@ -450,8 +432,7 @@ describe('schema validation NC NSFS bucket', () => {
 function get_bucket_data() {
    const bucket_name = 'bucket1';
    const id = '65a62e22ceae5e5f1a758aa8';
-    const system_owner = 'account1'; // GAP - currently account name
-    const bucket_owner = 'account1'; // account name
+    const system_owner = '65b3c68b59ab67b16f98c26e'; // GAP - currently account name


Suggested change

const system_owner = '65b3c68b59ab67b16f98c26e'; // GAP - currently account name

const system_owner = '65b3c68b59ab67b16f98c26e'; // GAP - currently bucket owner id

shirady · 2024-06-27T06:29:16Z

src/manage_nsfs/manage_nsfs_cli_utils.js

@@ -58,18 +58,18 @@ async function get_config_data(config_root_backend, config_file_path, show_secre
 * get_bucket_owner_account will return the account of the bucket_owner
 * otherwise it would throw an error
 * @param {string} config_root_backend
- * @param {string} accounts_dir_path
- * @param {string} bucket_owner
+ * @param {string} dir_path


What can be dir_path? would you please add details in the comment?

shirady · 2024-06-27T06:32:13Z

src/manage_nsfs/manage_nsfs_cli_utils.js

- * @param {string} accounts_dir_path
- * @param {string} bucket_owner
+ * @param {string} dir_path
+ * @param {string} name


I'm not sure changing the argument to just name is clearer (reading the function statement one might think it is a bucket name).

shirady · 2024-06-27T06:34:52Z

src/manage_nsfs/manage_nsfs_validations.js

@@ -247,7 +247,7 @@ function validate_flags_value_combination(type, action, input_options_with_data)
 * @param {object} data
 * @param {string} action
 */
-async function validate_bucket_args(config_root_backend, accounts_dir_path, data, action) {
+async function validate_bucket_args(config_root_backend, accounts_dir_path, root_accounts_dir_path, data, action) {


@alphaprinz @romayalon @naveenpaul1 @guymguym
Any ideas on how can pass the information of those arguments to the module that is not by every function?
config_root_backend, accounts_dir_path, root_accounts_dir_path

shirady · 2024-06-27T06:48:17Z

src/manage_nsfs/manage_nsfs_validations.js

 */
-async function verify_delete_account(config_root_backend, buckets_dir_path, account_name) {
+async function verify_delete_account(config_root_backend, buckets_dir_path, account_id) {


Do we still need this function with the current implementation that looks at ALL the buckets?
Can we add in the structure of the config to reflect the relationship: root account -> the buckets that it owns (like we would have root account -> the IAM accounts that it owns).

shirady · 2024-06-27T06:49:45Z

src/sdk/bucketspace_fs.js

+            return true;
+        } catch (err) {
+            return false;


The function name is _get_account_by_name but it returns a boolean (true/false), I'm not sure I understand.

shirady · 2024-06-27T06:53:35Z

src/sdk/bucketspace_fs.js

+            const account_config_path = this._get_account_config_path(bucket.owner_account);
+            data = (await nb_native().fs.readFile(this.fs_context, account_config_path)).data;
+            const account = JSON.parse(data.toString());
+            nsfs_schema_utils.validate_account_schema(account);
+            is_valid = await this.check_bucket_config(bucket);
+            if (!is_valid) {
+                dbg.warn('BucketSpaceFS: account linked to bucket is not valid: ', name);
+            }


What is the purpose of this addition?
Please also add in comments.

shirady · 2024-06-27T06:55:46Z

src/sdk/bucketspace_fs.js

@@ -312,8 +336,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
            name,
            tag: js_utils.default_value(tag, undefined),
            owner_account: account._id,
-            system_owner: new SensitiveString(account.name),
-            bucket_owner: new SensitiveString(account.name),
+            system_owner: account._id,


You removed the new SensitiveString it might create an issue with the containerized enviroment, make sure that you handle the places where is it unwrapped (or leave it with SensitiveString...).

shirady · 2024-06-27T07:03:22Z

src/cmd/manage_nsfs.js

+        //write_stdout_response(ManageCLIResponse.AccountUpdated, "update_name 2 = " + update_name);
+        await nb_native().fs.unlink(fs_context, cur_root_account_config_path);
+        await nb_native().fs.symlink(fs_context, new_account_relative_config_path, new_root_account_config_path);
+    }// else if (update_access_key) {


Why the "else if (update_access_key)" in a comment?

…to it Signed-off-by: Amit Prinz Setter <[email protected]>

Signed-off-by: Amit Prinz Setter <[email protected]>

… account id Signed-off-by: Amit Prinz Setter <[email protected]>

Signed-off-by: Amit Prinz Setter <[email protected]>

…) and test_bucketspace_fs.js Signed-off-by: Amit Prinz Setter <[email protected]>

Signed-off-by: Amit Prinz Setter <[email protected]>

pull-request-size bot added the size/L label Jun 26, 2024

alphaprinz requested review from guymguym, shirady, romayalon and naveenpaul1 June 26, 2024 03:37

alphaprinz changed the title ~~Nc account by~~ Nc account by id Jun 26, 2024

alphaprinz force-pushed the nc_account_by_id branch 3 times, most recently from 2a5df3c to d860c93 Compare June 27, 2024 05:28

shirady reviewed Jun 27, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch from d860c93 to eabba80 Compare June 27, 2024 15:39

alphaprinz added 9 commits June 27, 2024 09:50

NC | account by id | account by id, link root account and access key …

9e456f1

…to it Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | finish account_cli.test, changes to bucketspace_fs

15cb44c

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | remove bucket_owner from bucket schema, use only…

75ea48a

… account id Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | bucket cli and tests

9a70508

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | fix account_status and anonymous tests

703ac45

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | adapt test_nc_nsfs_cli.js

b58f133

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | small fixups

68f0217

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | remove bucket_owner from bucketspace_fs.js

f08683b

Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | allow principal to be either account id or name

f649185

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch 3 times, most recently from c9672e8 to 12e9d94 Compare June 27, 2024 18:57

NC | account by id | fix merge

f36fb47

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch from 12e9d94 to f36fb47 Compare June 27, 2024 19:23

alphaprinz added 2 commits June 27, 2024 13:55

NC | account by id | fix bucketspace_fs.has_bucket_action_permission(…

e69e7f7

…) and test_bucketspace_fs.js Signed-off-by: Amit Prinz Setter <[email protected]>

NC | account by id | lint

da61744

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch from 2f227ae to da61744 Compare June 27, 2024 22:21

pull-request-size bot added size/XL and removed size/L labels Jun 28, 2024

alphaprinz force-pushed the nc_account_by_id branch from 098d3d4 to 6e80cbc Compare June 28, 2024 01:25

NC | account by id | handle account email for bucket read and get acl

6acf688

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch 3 times, most recently from 25a75ea to 598b40a Compare June 28, 2024 21:02

NC | account by id | fixes for test_bucketspace.fs

87f7825

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch 2 times, most recently from c91bfce to 94a21c2 Compare June 28, 2024 22:58

NC | account by id | fixes for tests_s3_bucket_policy.js

665d0ff

Signed-off-by: Amit Prinz Setter <[email protected]>

alphaprinz force-pushed the nc_account_by_id branch from 94a21c2 to 665d0ff Compare June 29, 2024 03:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nc account by id #8167

Nc account by id #8167

alphaprinz commented Jun 26, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

shirady Jun 27, 2024

	const system_owner = '65b3c68b59ab67b16f98c26e'; // GAP - currently account name
	const system_owner = '65b3c68b59ab67b16f98c26e'; // GAP - currently bucket owner id

Nc account by id #8167

Are you sure you want to change the base?

Nc account by id #8167

Conversation

alphaprinz commented Jun 26, 2024

Explain the changes

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment