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

Allow optional SSL for native S3 file system #24771

Closed
rileymcdowell opened this issue Jan 22, 2025 · 6 comments
Closed

Allow optional SSL for native S3 file system #24771

rileymcdowell opened this issue Jan 22, 2025 · 6 comments

Comments

@rileymcdowell
Copy link

This issue arises because of incompatibilities between legacy and new native S3 file system support.

Our team uses MinIO as an api-compatible S3 emulation layer when doing local testing with Trino. This local testing happens in docker containers and is used to double check that configuration for connectors/catalogs will work prior to a deployed environment. In addition to running Trino and MinIO, we also run other applications that interact with Trino such as an orchestrator, all within docker, all to test for interoperability.

During our latest round of software upgrades, we came across the new native s3 filesystem migration instructions here:

https://trino.io/docs/current/object-storage/legacy-s3.html#migration-to-s3-file-system

These documents state that

SSL is always enabled and cannot be disabled.

The legacy S3 support included the ability to target an http (not https) s3 endpoint, and we use that for testing. This is ideal for local testing with MinIO because there's no need to set up self signed SSL certificates or share them among containers just to do basic interoperability testing in docker. With the introduction of S3 file system support, this is no longer possible due to mandatory SSL on connections to the S3 endpoint. Obviously we can stay on the legacy S3 support path for a while longer, but that's not sustainable long term for us or for Trino.

I understand this might seem like a silly request - it's 2025, shouldn't everyone be using SSL everywhere? For deployed environments, I agree completely. However, for local testing and adoption of technologies like Trino, having an easy path to confirm integration between components is useful not only to allow existing users like us to test, but to help interested adoptees have a good first experience when running a technology evaluation. That's precisely what our team did before we deployed Trino in our organization 2 years ago.

It would be ideal if there was a setting like s3.endpoint_requires_https that had a default value of true but that could be set to false to ease the setup requirements for testing trino against a local copy of MinIO.

Thank you for considering this request.

@wendigo
Copy link
Contributor

wendigo commented Jan 22, 2025

This is not true, we test in the CI with dockerized Minio that is not using TLS for both legacy and native file systems. Our configuration usually looks like this:

s3.endpoint=http://minio:9080/

@wendigo wendigo closed this as completed Jan 22, 2025
@rileymcdowell
Copy link
Author

This is what the current version of the docs on the Trino website have to say:

Image

I'm glad to hear that what we were doing is still possible. Do the docs need an update?

@wendigo
Copy link
Contributor

wendigo commented Jan 22, 2025

@mosabua can you chime in? SSL is required for generating pre-signed URIs in the spooled protocol. Rather than that, http endpoint will work with insecure Minio.

@mosabua
Copy link
Member

mosabua commented Jan 22, 2025

Hm .. we should update that in the docs .. @Joelg96 wrote that initially.. maybe he can chime in about the reasoning and/or send a PR to update. Also happy to review a PR from @rileymcdowell alternatively

@rileymcdowell
Copy link
Author

rileymcdowell commented Jan 22, 2025

The principal of Chesterton's Fence says we probably shouldn't change this until we know why it was put there in the first place. If we can get some clarity on what should be there (or if it turns out to have been not literally true and should go away) I would be happy to submit a PR.

@mosabua
Copy link
Member

mosabua commented Jan 22, 2025

Hahah .. sure. Personally I trust @wendigo to know this better than most..

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

No branches or pull requests

3 participants