Remove SSL consideration in S3 migration guide #24772
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
I don't understand this one too. Is the client-side referring to Trino doing encryption or else?
There was a problem hiding this comment.
Maybe things got mixed up in writing this .. spooling protocol vs native file system ..
There was a problem hiding this comment.
yeah, it seems so. SSL is required for S3 spooling as it generates presigned URIs.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
.withProtocol(sslEnabled ? Protocol.HTTPS : Protocol.HTTP)
That was setting protocol on AWS SDK v1 which is equivalent to setting an endpoint with http uri
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool.. so you can remove the whole section of warnings and the opening sentence then @Joelg96 ..
|
please squash into one commit and actually delete the section .. I think you must have mixed something up on your git fu |
|
Yeah, I messed it up on my side merging master and what not... Just created a new PR... #24772 |
Description
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: