(PPS-411): Add bucket selection support for multi part upload #1112

BinamB · 2023-08-15T15:20:12Z

Jira Ticket: PXP-xxxx

Remove this line if you've changed the title to (PXP-xxxx): <title>

cdis-data-client PR (uc-cdis/cdis-data-client#123)

New Features

Added upload bucket selection support for multipart upload
Add support for non-aws s3 buckets for multipart upload

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Avantol13 · 2023-08-23T15:41:33Z

fence/blueprints/data/blueprint.py

@@ -176,17 +177,9 @@ def upload_data_file():
    )

    protocol = params["protocol"] if "protocol" in params else None
-    bucket = params.get("bucket")
+    bucket = params.get("bucket", None)


If should already default to None, you don't need this

Avantol13 · 2023-08-23T15:43:02Z

fence/blueprints/data/blueprint.py

-            logger.debug(f"Bucket '{bucket}' not in ALLOWED_DATA_UPLOAD_BUCKETS config")
-            raise Forbidden(f"Uploading to bucket '{bucket}' is not allowed")
-
+        _is_allowed_data_upload_bucket_configured(bucket)


I know this function already exists, but the name is a bit confusing. The prefix "is" usually implies a boolean return. Since this is a check I would change this to something like "verify_data_upload_bucket_configuration"

Avantol13 · 2023-08-23T15:44:17Z

fence/blueprints/data/blueprint.py

@@ -257,12 +256,18 @@ def generate_multipart_upload_presigned_url():
        default=default_expires_in,
    )

+    bucket = params.get("bucket", None)


.get() already returns None if it's not found, so the , None is redundant. You could just .get("bucket")

Avantol13 · 2023-08-23T15:45:20Z

fence/blueprints/data/blueprint.py

+    if bucket:
+        _is_allowed_data_upload_bucket_configured(bucket)
+
+    # TODO add bucket param here


Is this a real TODO? like we need to do something in the future, can we do it now?

If this is just a note for the future in case we need to add things, don't use TODO, just say note: or something

Avantol13 · 2023-08-23T15:45:39Z

fence/blueprints/data/blueprint.py

@@ -285,6 +290,7 @@ def complete_multipart_upload():
        raise UserError("missing required arguments: {}".format(list(missing)))

    default_expires_in = flask.current_app.config.get("MAX_PRESIGNED_URL_TTL", 3600)
+    bucket = params.get("bucket", None)


same as prev comments, can just use .get(

Avantol13 · 2023-08-23T15:57:02Z

openapis/swagger.yaml

+        bucket:
+          type: string
+          required: false
+          description: bucket to upload


Suggested change

description: bucket to upload

description: bucket to upload to

Avantol13 · 2023-08-23T15:57:08Z

openapis/swagger.yaml

+        bucket:
+          type: string
+          required: false
+          description: bucket to upload


Suggested change

description: bucket to upload

description: bucket to upload to

tests/data/test_data.py

Avantol13 · 2023-08-23T15:58:36Z

fence/blueprints/data/indexd.py

-            raise InternalError(
-                "fence not configured with data upload bucket; can't create signed URL"
+        if bucket:
+            s3_buckets = get_value(


is this logic exactly what _is_allowed_data_upload_bucket_configured(bucket) is doing? Can you reuse that here?

I think you still left a chunk of code you don't need. If you're calling verify you don't need to get the s3_buckets here

Avantol13 · 2023-08-23T15:59:47Z

fence/blueprints/data/blueprint.py

@@ -9,6 +9,7 @@
    BlankIndex,
    IndexedFile,
    get_signed_url_for_file,
+    _is_allowed_data_upload_bucket_configured,


You can't/shouldn't import private functions or variables. The prefixed _ implies private in Python naming conventions so if you need the function to be used here, you need to rename it to remove the _ prefix and add a docstring

fence/blueprints/data/indexd.py

Avantol13 · 2023-11-21T15:14:07Z

fence/blueprints/data/multipart_upload.py

 from fence.errors import InternalError

 MAX_TRIES = 5

 logger = get_logger(__name__)


-def initilize_multipart_upload(bucket, key, credentials):
+def initilize_multipart_upload(bucket_name, key, credentials):


refactor name for typo: initialize, not initilize

fence/blueprints/data/blueprint.py

Avantol13 · 2023-11-21T15:18:29Z

fence/blueprints/data/indexd.py

+        if not bucket:
+            try:
+                bucket = flask.current_app.config["DATA_UPLOAD_BUCKET"]
+            except KeyError:


this error will never happen b/c DATA_UPLOAD_BUCKET is part of the default config, so it will always be set, it might just be ''. And this isn't catching the case where the bucket is actually empty ''.

So you can simplify/fix this whole block with:

bucket = bucket or flask.current_app.config["DATA_UPLOAD_BUCKET"] if not bucket: raise InternalError("fence not configured with data upload bucket; can't create signed URL")

There's a test that deletes that configuration so this will fail. https://github.com/uc-cdis/fence/blob/master/tests/data/test_blank_index.py#L351
I don't think it gets another default value.
Should I just set the test to empty the field rather than delete the config since that would be more realistic?

Avantol13 · 2023-11-21T15:21:08Z

fence/blueprints/data/indexd.py

+
+def verify_data_upload_bucket_configuration(bucket):
+    s3_buckets = get_value(
+        flask.current_app.config,


you don't need to use get_value b/c ALLOWED_DATA_UPLOAD_BUCKETS is part of the default configuration, which guarantees its existence in the config dictionary. If you're trying to check if it's an empty list, then this isn't catching that either.

s3_buckets = flask.current_app.config["ALLOWED_DATA_UPLOAD_BUCKETS"] if not s3_buckets: raise InternalError("ALLOWED_DATA_UPLOAD_BUCKETS not configured")

Avantol13 · 2023-11-21T15:22:33Z

fence/blueprints/data/indexd.py

-            raise InternalError(
-                "fence not configured with data upload bucket; can't create signed URL"
+        if bucket:
+            s3_buckets = get_value(


I think you still left a chunk of code you don't need. If you're calling verify you don't need to get the s3_buckets here

Avantol13 · 2023-11-21T15:22:56Z

fence/blueprints/data/indexd.py

+            verify_data_upload_bucket_configuration(bucket)
+        else:
+            try:
+                bucket = flask.current_app.config["DATA_UPLOAD_BUCKET"]


see previous comment about default configs. this will never KeyError

Avantol13 · 2023-11-21T15:23:13Z

fence/blueprints/data/indexd.py

+        else:
+            try:
+                bucket = flask.current_app.config["DATA_UPLOAD_BUCKET"]
+            except KeyError:


see previous comment

Avantol13 · 2023-11-21T15:25:58Z

fence/blueprints/data/multipart_upload.py

@@ -95,7 +125,7 @@ def complete_multipart_upload(bucket, key, credentials, uploadId, parts):


 def generate_presigned_url_for_uploading_part(


these are all positive tests. I know they were here before, but can you add some for the error cases like when DATA_UPLOAD and/or ALLOWED_DATA_UPLOAD_BUCKETS buckets aren't specified / are empty?

Sounds like my previous comment might resolve this test case? 🤔

lbeckman314 · 2024-01-22T18:43:41Z

Thanks @BinamB for your patience, working on the unit tests issues blocking #1127 which targets this branch (let us know if we should specify a different branch to merge into!).

I will update here and in Slack when the tests are running successfully and it's ready for review!

BinamB added 5 commits July 12, 2023 14:07

Multi Part Init

d5f92b4

change multipart_upload

dd8f0ff

add unit tests

065e77c

add complete bucket name

5136bad

add params to api

6adbc62

github-actions bot added the @requires-fence label Aug 15, 2023

BinamB mentioned this pull request Aug 15, 2023

(PPS-411): Add bucket selection support for multi part upload uc-cdis/cdis-data-client#123

Merged

1 task

PlanXCyborg added test-apis-centralizedAuth test-apis-dataUploadTest test-apis-dbgapTest test-portal-dataUploadTest and removed @requires-fence labels Aug 15, 2023

lbeckman314 mentioned this pull request Aug 15, 2023

Test with wasabi S3 ACED-IDP/gen3-helm#25

Closed

BinamB and others added 2 commits August 16, 2023 11:39

Merge branch 'master' into feat/multi_part_bucket_param

a5fb557

Add logging + cleanup

dda86e1

BinamB marked this pull request as ready for review August 16, 2023 19:58

BinamB removed test-apis-dataUploadTest test-apis-dbgapTest test-portal-dataUploadTest test-apis-centralizedAuth labels Aug 16, 2023

Add test + documentation

fa3f969

Avantol13 requested changes Aug 23, 2023

View reviewed changes

BinamB and others added 4 commits August 24, 2023 13:39

Resolve Comments

3c8788d

Support non aws s3 for multipart

3a46245

typo

5359915

Update support for multipart uploads to non-AWS S3 endpoints

0e2c5ec

BinamB changed the title ~~(feat): Add bucket selection support for multi part upload~~ (PPS-411): Add bucket selection support for multi part upload Oct 18, 2023

Avantol13 requested changes Nov 21, 2023

View reviewed changes

lbeckman314 mentioned this pull request Jan 25, 2024

Feat/s3 delete files #1127

Merged

MaribelleHGomez previously approved these changes Apr 2, 2024

View reviewed changes

reviewed comments

5f710c8

BinamB dismissed MaribelleHGomez’s stale review via 5f710c8 April 3, 2024 20:57

BinamB force-pushed the feat/multi_part_bucket_param branch from 689d11e to 5f710c8 Compare April 3, 2024 20:57

resolve comments

517566d

BinamB force-pushed the feat/multi_part_bucket_param branch from bbaa9bb to 517566d Compare April 4, 2024 15:32

BinamB added 2 commits April 4, 2024 10:39

Merge branch 'master' into feat/multi_part_bucket_param

22a4ff9

Merge branch 'master' into feat/multi_part_bucket_param

5cc14c1

Avantol13 approved these changes Apr 4, 2024

View reviewed changes

BinamB merged commit 558ac4e into master Apr 5, 2024
9 checks passed

BinamB deleted the feat/multi_part_bucket_param branch April 5, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PPS-411): Add bucket selection support for multi part upload #1112

(PPS-411): Add bucket selection support for multi part upload #1112

BinamB commented Aug 15, 2023 •

edited

Loading

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Nov 21, 2023

Avantol13 Aug 23, 2023

Avantol13 Nov 21, 2023

Avantol13 Nov 21, 2023

BinamB Dec 1, 2023

Avantol13 Nov 21, 2023

Avantol13 Nov 21, 2023

Avantol13 Nov 21, 2023

Avantol13 Nov 21, 2023

Avantol13 Nov 21, 2023

BinamB Dec 1, 2023

lbeckman314 commented Jan 22, 2024 •

edited

Loading

	description: bucket to upload
	description: bucket to upload to

		@@ -95,7 +125,7 @@ def complete_multipart_upload(bucket, key, credentials, uploadId, parts):


		def generate_presigned_url_for_uploading_part(

(PPS-411): Add bucket selection support for multi part upload #1112

(PPS-411): Add bucket selection support for multi part upload #1112

Conversation

BinamB commented Aug 15, 2023 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

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

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

lbeckman314 commented Jan 22, 2024 • edited Loading

BinamB commented Aug 15, 2023 •

edited

Loading

lbeckman314 commented Jan 22, 2024 •

edited

Loading