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

[BEAM-3813] Support encryption for S3FileSystem (SSE-S3, SSE-C and SSE-KMS) #5244

Merged
merged 6 commits into from May 14, 2018

Conversation

iemejia
Copy link
Member

@iemejia iemejia commented Apr 28, 2018

No description provided.

@iemejia
Copy link
Member Author

iemejia commented Apr 28, 2018

R: @jbonofre @echauchot
CC: @jacobmarble

@iemejia
Copy link
Member Author

iemejia commented May 3, 2018

Have not noticed this needed a rebase. @jbonofre PTAL

@jbonofre
Copy link
Member

jbonofre commented May 3, 2018

Thanks for the update. I'm taking a look.

@jacobmarble
Copy link

This PR looks great @iemejia . I see a few nice cleanups, too; thanks!

@iemejia
Copy link
Member Author

iemejia commented May 4, 2018

Thanks @jacobmarble. Hoping this gets into the next release.

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

LGTM ! I'm merging. Great job !

@jbonofre jbonofre merged commit 7a3f688 into apache:master May 14, 2018
@iemejia iemejia deleted the BEAM-3813-s3-sse branch May 14, 2018 15:13

@Description("SSE key for SSE-C encryption, e.g. a base64 encoded key and the algorithm.")
@Nullable
SSECustomerKey getSSECustomerKey();
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a custom object type, how does it get serialized/deserialized with Jackson?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, thanks you have a really valid argument. I hesitated to put the arguments as Strings and build the API objects internally but I thought letting the AWS SDK exposed would move this maintenance to the user. Do you have any suggestion on how to do this properly ?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest following the same pattern that was done for the AWSCredentialsProvider. You can see how the class is being converted to and from JSON within AwsModule.java. You effectively define the schema of the object as JSON.

AWSCredentialsProvider to/from JSON had some complexity since it had to deal with inheritance and that it wasn't trivial to convert some credentials providers since they didn't expose all the necessary attributes.


@Description("KMS key id for SSE-KMS encryption, e.g. \"arn:aws:kms:...\".")
@Nullable
SSEAwsKeyManagementParams getSSEAwsKeyManagementParams();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on SSEAwsKeyManagementParams and Jackson serialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants