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.

Sign up for GitHub

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

Jump to bottom

Rework S3 object version ID and marker check #8014

Merged
merged 14 commits into from
May 8, 2024
Merged

Rework S3 object version ID and marker check #8014

merged 14 commits into from
May 8, 2024

Conversation

Neon-White
Copy link
Contributor

@Neon-White Neon-White commented May 6, 2024 •
edited
Loading

Explain the changes

  1. Move the version ID check to happen before read_object_md is called, which takes care of both namespacestore and backingstore queries - while also potentially eliminating the case in which the store would enter a Rejected state
  2. Remove the max-line linter error in test_s3_ops
  3. Check for empty strings in all API calls containing a version ID
  4. Check for empty strings in all API calls containing a version ID marker

Issues: Fixed #xxx / Gap #xxx

  1. 2265562

Testing Instructions:

  1. Upload an object to a bucket
  2. Use the AWS CLI (s3api with --version-id "") to try and access an existing object's version ID by providing an empty string
  3. Verify that NooBaa throws the appropriate error (An error occurred (InvalidArgument) when calling the GetObject operation: Version id cannot be the empty string)
  • Tests added

@Neon-White Neon-White requested review from liranmauda and shirady May 6, 2024 16:50
@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from 0ec8e3a to 267a53c Compare May 6, 2024 16:52
guymguym
guymguym requested changes May 6, 2024
View reviewed changes
src/endpoint/s3/ops/s3_get_object.js Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/S and removed size/XS labels May 6, 2024
@Neon-White Neon-White requested a review from guymguym May 6, 2024 20:27
@Neon-White Neon-White changed the title Move S3 get_object version ID check Rework S3 object version ID check May 6, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 6, 2024
@Neon-White Neon-White requested a review from guymguym May 7, 2024 00:58
shirady
shirady reviewed May 7, 2024
View reviewed changes
src/endpoint/s3/ops/s3_get_object.js Show resolved Hide resolved
src/endpoint/s3/s3_utils.js Outdated Show resolved Hide resolved
shirady
shirady reviewed May 7, 2024
View reviewed changes
src/endpoint/s3/s3_rest.js Outdated Show resolved Hide resolved
guymguym
guymguym reviewed May 7, 2024
View reviewed changes
src/endpoint/s3/s3_rest.js Outdated Show resolved Hide resolved
guymguym
guymguym reviewed May 7, 2024
View reviewed changes
src/endpoint/s3/s3_rest.js Outdated Show resolved Hide resolved
@Neon-White Neon-White requested a review from guymguym May 7, 2024 10:01
@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from 2e993d4 to 5ec49fc Compare May 7, 2024 13:53
@Neon-White Neon-White changed the title Rework S3 object version ID check Rework S3 object version ID and marker check May 8, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 8, 2024
@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from 90fd733 to 1e4444b Compare May 8, 2024 09:43
@Neon-White Neon-White requested a review from shirady May 8, 2024 09:43
shirady
shirady approved these changes May 8, 2024
View reviewed changes
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

@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from 082923b to c979e8d Compare May 8, 2024 15:18
@Neon-White Neon-White requested a review from guymguym May 8, 2024 19:37
Neon-White added 12 commits May 8, 2024 21:45
@Neon-White
Move S3 get_object version ID check
57235da 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
- Move the obj version ID check to S3 utils
098230a 
- Move the `get_object` check closer to the parsing checks
- Add the version ID check to `head_object`

Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Fix check logic
f215786 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Add unittest
6a4d950 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Fix logic to actually make sense
a6a2d6a 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
- Change parse_version_id logic
417ac0d 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Utilize parse_version_id.... everywhere.
6cd2a4d 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Please DeepScan
4d503f8 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
- Improve parse_version_id docs
f99c8a3 
- Revert `s3_rest.js` changes

Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
- Revert s3_post_bucket_delete changes
46cdd2e 
- Add empty version ID marker error
- Add empty version ID marker checks

Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Add empty version ID marker test
ff82577 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White
Fix test print
e6781c6 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from 9ad4d56 to 3cfa021 Compare May 8, 2024 19:45
@Neon-White
Rework error flow
f3c3f65 
Signed-off-by: Ben <belimele@redhat.com>
@Neon-White Neon-White force-pushed the move-s3-req-versionid-check branch from f844763 to f3c3f65 Compare May 8, 2024 20:06
@Neon-White
Mark empty_err as optional
edbfc3b 
Signed-off-by: Ben <belimele@redhat.com>
guymguym
guymguym approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@guymguym guymguym 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!

@guymguym guymguym merged commit 105625c into noobaa:master May 8, 2024
10 checks passed
@Neon-White Neon-White deleted the move-s3-req-versionid-check branch May 8, 2024 22:58
@nimrod-becker
Copy link
Contributor

@guymguym 14 commits! not squashed :(

@guymguym
Copy link
Member

guymguym commented May 9, 2024

didnt notice. will be squashed by backports if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shirady shirady shirady approved these changes

@guymguym guymguym guymguym approved these changes

@liranmauda liranmauda Awaiting requested review from liranmauda

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Neon-White @nimrod-becker @guymguym @shirady