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

Remove SSL consideration in S3 migration guide #24772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Joelg96
Copy link
Contributor

@Joelg96 Joelg96 commented Jan 22, 2025

Description

  • Removes SSL consideration in the S3 file system migration guide.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@Joelg96 Joelg96 requested a review from mosabua January 22, 2025 19:55
@cla-bot cla-bot bot added the cla-signed label Jan 22, 2025
@Joelg96 Joelg96 requested a review from wendigo January 22, 2025 19:55
@github-actions github-actions bot added the docs label Jan 22, 2025
@mosabua
Copy link
Member

mosabua commented Jan 22, 2025

Would be good to know from @jhlodin why this was there in the first place and get confirmation from @wendigo or @electrum that this is valid as a change.

@@ -346,7 +346,6 @@ implementation is recommended. Legacy support will be deprecated and removed.
The following considerations apply to legacy configurations moving to the native
implementation:

* SSL is always enabled and cannot be disabled.
* Client-side encryption is not supported. Server-side encryption (SSE) is
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this one too. Is the client-side referring to Trino doing encryption or else?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe things got mixed up in writing this .. spooling protocol vs native file system ..

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it seems so. SSL is required for S3 spooling as it generates presigned URIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still worth mentioning: "Multipart (non-streaming) writes and upload are not supported."? Does it really add value or should the entire "considerations" section be removed?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what that means in the context eithere.. lets wait for @jhlodin to chime in about where that info came from, but I think we can remove it all.

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

Successfully merging this pull request may close these issues.

3 participants