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

[bugfix]: Optional check for encryption in s3op response #1460

Merged
merged 2 commits into from
Jun 20, 2023
Merged

[bugfix]: Optional check for encryption in s3op response #1460

merged 2 commits into from
Jun 20, 2023

Conversation

valayDave
Copy link
Collaborator

@valayDave valayDave commented Jun 19, 2023 •
edited
Loading

Introduction of s3 encryption #1436 broke support for MiniO since MiniO doesn't have a ServerSideEncryption key in its response payload. This PR will only set encryption from the response if ServerSideEncryption is present in the payload.

@valayDave valayDave requested a review from saikonen June 19, 2023 22:29
savingoyal
savingoyal reviewed Jun 20, 2023
View reviewed changes
metaflow/plugins/datatools/s3/s3.py Outdated
@@ -759,7 +759,9 @@ def _info(s3, tmp):
"metadata": resp["Metadata"],
"size": resp["ContentLength"],
"last_modified": get_timestamp(resp["LastModified"]),
"encryption": resp["ServerSideEncryption"],
"encryption": resp["ServerSideEncryption"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you do resp.get("ServerSideEncryption") instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a comment here re: why ServerSideEncryption might be missing and under what circumstances?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@saikonen
Copy link
Collaborator

Complete oversight on my part while testing the original feature. Now verified that this fixes the issue with MinIO.
I second the suggestion to use resp.get("ServerSideEncryption") for a bit of a cleaner setup. Otherwise this is good to go👌

@savingoyal savingoyal merged commit 5571e2d into Netflix:master Jun 20, 2023
@ashtuchkin
Copy link

Hey, I'm getting an exception here when working with moto S3 mocking server, seems like this should be changed to .get as well?
https://github.com/Netflix/metaflow/blob/master/metaflow/plugins/datatools/s3/s3op.py#L282

@DonIvanCorleone DonIvanCorleone mentioned this pull request Jul 10, 2023
Closed
@saikonen
Copy link
Collaborator

Hey, I'm getting an exception here when working with moto S3 mocking server, seems like this should be changed to .get as well? https://github.com/Netflix/metaflow/blob/master/metaflow/plugins/datatools/s3/s3op.py#L282

Thank you for the report, this should now be fixed with the latest 2.9.9 release.

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

@savingoyal savingoyal savingoyal left review comments

@saikonen saikonen Awaiting requested review from saikonen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@valayDave @saikonen @ashtuchkin @savingoyal