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

s3fs: Added documentation for SSE-C support #3498

Merged
merged 7 commits into from
Jun 22, 2022
Merged

s3fs: Added documentation for SSE-C support #3498

merged 7 commits into from
Jun 22, 2022

Conversation

ap-kulkarni
Copy link
Contributor

@ap-kulkarni ap-kulkarni commented May 2, 2022 •
edited by shcheklein
Loading

Changes based on iterative/dvc#7671

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@ap-kulkarni ap-kulkarni mentioned this pull request May 2, 2022
Merged
2 tasks
@shcheklein shcheklein added ⌛ status: wait-core-merge Waiting for related product PR merge/release A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference labels May 2, 2022
jorgeorpinel added a commit that referenced this pull request May 9, 2022
@jorgeorpinel
ref: codeblocks
32b0e72 
complement to #3498
@jorgeorpinel jorgeorpinel mentioned this pull request May 9, 2022
Merged
jorgeorpinel
jorgeorpinel reviewed May 9, 2022
View reviewed changes
content/docs/command-reference/remote/modify.md Outdated
Comment on lines 265 to 266
- `sse_customer_key` - user specified key to encrypt data uploaded when using
SSE-C. The value should be base64 encoded version of 256 bit key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! What's the value provided to sse? Is it SSE-C, or something like aws:ssec (similar to aws:kms) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is AWS-specific right?

Copy link
Contributor Author

@ap-kulkarni ap-kulkarni May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. This is AWS-specific. Also, when SSE with client side keys for encryption is in question, libraries usually expect different parameter (SSECustomerAlgorithm as key in s3_additional_kwargs dictionary) from sse (which gets added as ServerSideEncryption as key in the s3_additional_kwargs dictionary). So, for SSE, when a user wants to use key in AWS KMS, the relevent parameters are sse and sse_kms_key_id. When a user wants to use their own key (which is SSE-C) then the relevant parameters are sse_customer_key and sse_customer_algorithm. Value of sse is irrelevant for SSE-C.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it.

@restyled-io restyled-io bot mentioned this pull request May 9, 2022
Closed
@jorgeorpinel

This comment was marked as resolved.

Sign in to view
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 11, 2022 •
edited
Loading

OK well this looks good (matches current iterative/dvc#7671) but needs reformatting, and Restyled is failing...

Anyway I can fix that but please lmk when the core PR is merged first, @ap-kulkarni. And thanks again!

@ap-kulkarni

This comment was marked as resolved.

Sign in to view
@ap-kulkarni

This comment was marked as resolved.

Sign in to view
@jorgeorpinel jorgeorpinel mentioned this pull request May 12, 2022
Closed
@jorgeorpinel

This comment was marked as resolved.

Sign in to view
@jorgeorpinel

This comment was marked as outdated.

Sign in to view
jorgeorpinel
jorgeorpinel approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Just waiting for core merge/release. Thanks again @ap-kulkarni

@julieg18 julieg18 temporarily deployed to dvc-org-master-cwkqja3dzl9go7h June 2, 2022 12:37 Inactive
@jorgeorpinel jorgeorpinel removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Jun 22, 2022
@jorgeorpinel
Copy link
Contributor

I see this was merged in core. Any relevant changes @ap-kulkarni ? Thanks

@jorgeorpinel jorgeorpinel requested a review from efiop June 22, 2022 02:43
@ap-kulkarni
Copy link
Contributor Author

Not really sure if there is a change in the way users configure the remotes after the refactoring of related files into dvc-objects project. If there is no change, then the documentation does not need any changes.

@daavoo daavoo merged commit 96becf6 into iterative:master Jun 22, 2022
jorgeorpinel added a commit that referenced this pull request Jul 12, 2022
@jorgeorpinel @restyled-io @restyled-commits
ref: code blocks around remote modify sse, etc. (#3533)
95204b1 
* ref: codeblocks

complement to #3498

* Update content/docs/command-reference/remote/modify.md

* Update content/docs/command-reference/remote/modify.md

* ref: link `sse_kms_key_id` remote config param

* Update content/docs/command-reference/remote/modify.md

* Update content/docs/command-reference/remote/modify.md

* Restyled by prettier (#3547)

Co-authored-by: Restyled.io <commits@restyled.io>

* ref: remote modify sse aws:kms doesn't require explicit key

per #3533 (review)

* Restyled by prettier (#3581)

Co-authored-by: Restyled.io <commits@restyled.io>

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel approved these changes

@efiop efiop Awaiting requested review from efiop

Assignees

@ap-kulkarni ap-kulkarni

Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ap-kulkarni @jorgeorpinel @shcheklein @daavoo @julieg18