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

Remove SSL consideration in S3 migration guide #24772

Closed
wants to merge 2 commits into from
Closed

Remove SSL consideration in S3 migration guide #24772

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17c9a36
Remove SSL consideration
Joelg96 Jan 22, 2025
5771142
Remove consideration sections
Joelg96 Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/src/main/sphinx/object-storage/legacy-s3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Contributor

@jhlodin jhlodin Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When Starburst was planning to roll out support for the native S3 filesystem integration, we had found almost 30 deployments that had set the legacy hive.s3.ssl catalog config property to false. The property was removed for the native file system implementation. So we felt that this explicit callout was necessary to make it more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

.withProtocol(sslEnabled ? Protocol.HTTPS : Protocol.HTTP)

That was setting protocol on AWS SDK v1 which is equivalent to setting an endpoint with http uri

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the withProtocol javadoc:

Individual clients can also override this setting by explicitly including the protocol as part of the endpoint URL when calling AmazonWebServiceClient. setEndpoint(String).

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough! Well that was the reasoning for why it was added in the first place, makes sense to me to remove it then. Maybe consider adding a callout to the migration guide if you feel that forcing HTTP is a realistic use case.

Copy link
Member

Choose a reason for hiding this comment

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

Cool.. so you can remove the whole section of warnings and the opening sentence then @Joelg96 ..

the recommended alternative.
* Multipart (non-streaming) writes and upload are not supported.
Expand Down
Loading