s3fs: Adds support for SSE client keys #7671
Conversation
|
I will raise a separate PR for documentation and link it here once the code changes are agreed by reviewers. |
…ES256 if sse_customer_key is specified and no algorithm is specified
|
Created PR at dvc.org#3498 for documentations changes |
dvc/fs/s3.py
Outdated
| sse_params = SSEParams( | ||
| server_side_encryption=config.get("sse"), | ||
| sse_customer_algorithm=sse_customer_algorithm, | ||
| sse_customer_key=sse_customer_key, | ||
| sse_kms_key_id=config.get("sse_kms_key_id"), | ||
| ) |
There was a problem hiding this comment.
I wonder if these belong in s3fs?
There was a problem hiding this comment.
Not sure exactly what the question meant. However, I would go ahead explain the snippet tagged. This SSEParams is just a helper class which accepts these parameters and provides a method to_kwargs() which returns a dictionary with appropriate key names required by aws libraries. Before these changes, the key names were hardcoded like below
additional["ServerSideEncryption"] = config.get("sse")
additional["SSEKMSKeyId"] = config.get("sse_kms_key_id")
Now, this is achieved by appropriately constructing the SSEParams object, and then updating the additional dictionary with the dictionary obtained from to_kwargs() like below
additional.update(sse_params.to_kwargs())
|
Let me know if this PR requires any more work. |
|
Sorry, @ap-kulkarni, we haven't had a chance to take a close look. We will try to do that soon and let you know if anything is needed 🙏 . |
There was a problem hiding this comment.
Hi @ap-kulkarni ! Unfortunately, during the period of the review process, some of the files modified here (dvc/fs/s3.py) were refactored out to a separate repo (https://github.com/iterative/dvc-objects/blob/main/src/dvc_objects/fs/implementations/s3.py) . You would need to move part of the changes to dvc-objects and open a P.R. there 🙏
|
Thank you @daavoo. What would be the best course of action then? Should this PR be closed and fresh sets of PRs be raised? or should I just raise PR for changes in |
I would just remove |
|
Opened PR #68 in |
|
@ap-kulkarni Thank you for your patience! Regarding tests, please do add them to this PR. You can run them locally to ensure that they work using |
Fixes #6141
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏