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 | Bucket Owner Removal #8289

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Aug 18, 2024

Disclaimers -

  1. This PR is based on the bucket owner removal part of Nc account by id #8167 written by @alphaprinz.
  2. This PR is dependent on merging - NC | Config Dir Restructure #8279 and currently, cherry-picking its only commit, which will be removed after its merge.

Explain the changes

  1. Schema - Removal of bucket_owner property requirement from nsfs_bucket_schema.js. Didn't remove it completely from the schema for backward compatibility.
  2. CLI - Create/Update flows - removal of bucket_owner property usage from the schema, but left it on the CLI API which means that on create/update bucket --owner flag is still the name of the account, and it'll be mapped to an id internally - owner_account.
  3. S3 API (BucketspaceFS) -
  • read_bucket_sdk_info() - since we don't keep the bucket owner name we now read the account config file by its id (owner account) and set the bucket_owner run time object.
  • create_bucket() - removed bucket_owner from new_bucket_defaults.
  1. IAM API (AccountspaceFS) -
  • _check_if_root_account_does_not_have_buckets_before_deletion() - Updated usage by id instead of by name.
  1. Tests/Docs - Removed and updated bucket_owner related changes.

Issues: Fixed #xxx / Gap #xxx

  1. Fixed #Account update with account name is not updating bucket_name.json file automatically #8080
  2. 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;

Testing Instructions:

  • Doc added/updated
  • Tests added

src/test/unit_tests/jest_tests/test_config_fs.test.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Show resolved Hide resolved
src/test/unit_tests/test_bucketspace.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_cli_utils.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Show resolved Hide resolved
@romayalon romayalon requested a review from shirady August 20, 2024 15:32
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Show resolved Hide resolved
src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Show resolved Hide resolved
src/test/unit_tests/test_nc_nsfs_cli.js Outdated Show resolved Hide resolved
@shirady
Copy link
Contributor

shirady commented Aug 21, 2024

@romayalon could you please update the the PR description (and attach the issue)?

config.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Outdated Show resolved Hide resolved
src/cmd/manage_nsfs.js Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_cli_utils.js Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Outdated Show resolved Hide resolved
src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

@romayalon romayalon force-pushed the romy-bucket-owner-removal branch 2 times, most recently from e287e6f to 8a352d9 Compare August 22, 2024 09:32
@romayalon romayalon merged commit 8c93a87 into noobaa:master Aug 22, 2024
10 checks passed
@nimrod-becker nimrod-becker mentioned this pull request Sep 4, 2024
2 tasks
shirady added a commit to shirady/noobaa-core that referenced this pull request Sep 8, 2024
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 added a commit to shirady/noobaa-core that referenced this pull request Sep 8, 2024
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]>
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