noobaa-core
noobaa-core copied to clipboard
Remove `allowed_buckets`
Explain the changes
This PR attempts to remove allowed_buckets
construct from Noobaa core:
- The construct of
allowed_buckets.permission_list
is replaced with Bucket Policy. - All the files that belonged to lambdas and were touched in the PR are deleted.
- Tests were modified to use Bucket Policies wherever either
allowed_buckets.full_permission
orallowed_buckets.permission_list
was being used. - Upgrade script has been added for version 5.12 which removes
allowed_buckets
field from the accounts in the database.
Issues: Fixed BZ1929108
- https://bugzilla.redhat.com/show_bug.cgi?id=1929108
Testing Instructions:
- Run
make test
- Run
make test-postgres
- Run
docker run --user root --rm --name test1 noobaa-tester ./src/test/unit_tests/run_npm_test_on_test_container.sh -s sudo_index.js
- [ ] Doc added/updated
- [ ] Tests added
I think they can
Streamlining discussion (TLDR of above comments) for reviewers
All of the "resolved" comments have been addressed
There are 2 items still needs to be discussed in this PR:
-
has_bucket_permission
has become fairly complicated because- Earlier it used to rely on
allowed_buckets
which now no longer exists. It has been now replaced with bucket policy. - Regardless of the intended action,
has_bucket_permission
verifies every action because this function is directly and indirectly called in multiple other portions where the intended action may or may not be accessible.
- Earlier it used to rely on
- Removal of
allowed_buckets.permission_list
(NooBaa Operator uses the above field to grant access to accounts to the buckets created via OBC) causes the following issues :- NooBaa operator will fail to give access to accounts associated with the OBC.
- Proposed Solution: Replace
allowed_buckets.permission_list
with bucket policy. - Issue with proposed solution: Creating a bucket policy without user consent.
- Proposed Solution: Replace
- Users will not be able to access their previous buckets:
- Proposed Solution: Translate all of the
allowed_buckets.permission_list
to bucket policy in the 5.12 upgrade script. - Issue with proposed solution: Creating a bucket policy without user consent.
- Proposed Solution: Translate all of the
- NooBaa operator will fail to give access to accounts associated with the OBC.
After discussions over the items mentioned in the previous comment:
-
has_bucket_permission
rigorous permission checks if can be avoided or reduced must be reduced. This may involve removing redundant permission checks. - Removal
allowed_buckets.full_permission
may cause regression but the goal is to ensure that at least 90% of the users should be able to upgrade without any hiccups.- 2 flows that needs to be fully functional are:
- Creation of OBC and verifying if the bucket is accessible and the user can perform operations on the bucket.
- Creation of S3 bucket by the user and verifying that the user can perform operations on the bucket.
- 2 flows that needs to be fully functional are:
After discussion with @jackyalbo, load_bucket
and find_bucket
in the bucket_server and object_server no longer rely on has_bucket_permission
which essentially means, that if an RPC is performed and the user is authenticated then they will be able to access buckets/objects without further authorization. The exception to this is list_bucket
which will list only the buckets to which the requesting account has access. Hence, authz is now only limited to s3_rest
sts_rest
.
PS: For some reason the test is failing here although passing on the local system.
Rebased the branch ✔️. Thank you @jackyalbo for reviewing this PR 😄!
Merging the PR...