Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NC | NSFS | Config Dir Restructure - Add users/ Dir #8312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Aug 28, 2024

Explain the changes

  1. Update IAM API Users, Access Keys and additional changes in accountspace_fs:
  • Move the config creation from the function _copy_data_from_requesting_account_to_account_config to the create_user.
  • Fix the ARN account ID for root accounts that were operated by the roots accounts manager (before we copied the requesting_account._id which was true only for root accounts on IAM users).
  • Fix _check_root_account as it has a redundant line that was not relevant (it was there when we thought of additional case, but we never get to it).
  • Add 2 helper functions: _get_account_owner_id_for_arn, _get_owner_account_argument.
  • Improve performance in the function _check_if_root_account_does_not_have_IAM_users_before_deletion after we have the new structure.
  1. Update the ConfigFS module to support the new structure and operate on users configs.
  2. Update docs:
  • With the config dire restructure (identities/, accounts_by_name/, users/directories).
  • IAM docs - regarding the naming scope (that we have with the new structure) and about the new structure with users/ directory.
  1. Update the IAM API tests:
  • Mainly reading the config file in the new structure.
  • Add account validation to accounts created hardcoded (to avoid schema changes without them updated).
  • Refactor it names to multiple lines.
  1. In rest_s3 change the 'is_owner` part (the gap mentioned in NC | Bucket Owner Removal #8289), where it checks the name, to make sure the account is not a user with the same name.

Issues:

Open questions to answer in the CR:

  1. Should we delete the directory users/ if there are no users in the account? I decided not to delete it.
  2. What should the ARN of the account (not a user, the identity that the root accounts manager operates on) look like? I decided it will be arn:aws:iam::${account_id}:user/${username} the account_id is his _id and the username is the account name.

List of GAPS:

  1. Add JSDoc in accountspace_fs methods.

Testing Instructions:

Unit Tests:

Please run: sudo npx jest test_accountspace_fs.test.js

Manual Tests:

Operate any IAM actions on users and access keys on the NSFS server as described in Non Containerized NSFS IAM (Developers Documentation)

  • Doc added/updated
  • Tests added

@shirady shirady self-assigned this Aug 28, 2024
Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a small gap from bucket_owner removal we had to add when adding users/ - #8289

Gap - we need to remove the following line when having IAM users in users/ directory (this will mean that a root account can have the same name as an IAM user) @shirady

-   if (account_identifier === bucket_owner.unwrap()) return true;
+   if (owner_account && account._id === owner_account.id) return true;
+   //name check - only for root accounts
+   if (!account.owner && account_identifier_name === bucket_owner.unwrap()) return true;

docs/NooBaaNonContainerized/Configuration.md Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
src/sdk/accountspace_fs.js Show resolved Hide resolved
src/sdk/config_fs.js Outdated Show resolved Hide resolved
@nimrod-becker nimrod-becker mentioned this pull request Sep 4, 2024
2 tasks
@shirady shirady force-pushed the nsfs-nc-config-dir-restructure-users-dir branch 3 times, most recently from 66c7a7d to 08b70a8 Compare September 5, 2024 14:39
@shirady shirady force-pushed the nsfs-nc-config-dir-restructure-users-dir branch from 08b70a8 to 4036567 Compare September 8, 2024 05:16
1. Update IAM API Users, Access Keys and additional changes in accountspace_fs:
 - Move the config creation from the function _copy_data_from_requesting_account_to_account_config to the create_user.
 - Fix the ARN account ID for root accounts that were operated by the roots accounts manager (before we copied the requesting_account._id which was true only for root accounts on IAM users).
 - Fix _check_root_account as it has a redundant line that was not relevant (it was there when we thought of additional case, but we never get to it).
 - Add 2 helper functions: _get_account_owner_id_for_arn, _get_owner_account_argument.
 - Improve performance in the function _check_if_root_account_does_not_have_IAM_users_before_deletion after we have the new structure.
2. Update the ConfigFS module to support the new structure and operate on users configs.
3. Update docs:
 - With the config dire restructure (identities/, accounts_by_name/, users/directories).
 - IAM docs - regarding the naming scope (that we have with the new structure) and about the new structure with users/ directory.
4. Update the IAM API tests:
 - Mainly reading the config file in the new structure.
 - Add account validation to accounts created hardcoded (to avoid schema changes without them updated).
 - Refactor `it` names to multiple lines.
5. In rest_s3 change the 'is_owner` part (the gap mentioned in NC | Bucket Owner Removal noobaa#8289), where it checks the name, to make sure the account is not a user with the same name.

Signed-off-by: shirady <[email protected]>
@shirady shirady force-pushed the nsfs-nc-config-dir-restructure-users-dir branch from 4036567 to 2b3f42d Compare September 8, 2024 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants