Skip to content

feat: rustls-tls and rustls-native-roots feature flags #1361

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

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

Conversation

c-thiel
Copy link
Collaborator

@c-thiel c-thiel commented May 21, 2025

What changes are included in this PR?

Add rustls-tls and rustls-tls-native-roots" feature flags for rest catalog & core.
reqwest is not directly used in other crates

Are these changes tested?

No - just pass through a well-tested rustls functionality.

@liurenjie1024
Copy link
Contributor

Hi, @c-thiel Could you elaborate what problem this pr is solving? I don't get much why we need this flag.

@c-thiel
Copy link
Collaborator Author

c-thiel commented May 22, 2025

Hi @liurenjie1024,
by default reqwests uses the openssl library of the system (vendored openssl) to connect to servers via TLS / HTTPS.
This creates a runtime dependency on the underlying system to have a compatible openssl library installed.

rustls is rust-native implementation of openssl which removes this runtime dependency. This is very helpful to build platform agnostic binaries as well as building minimal docker images. Lakekeeper for example is build on the google distroless images stack. The minimal distroless incompatible is incompatible with iceberg-rust due to the missing runtime depdendency of openssl. To remove this runtime dependency, we currently hard-coded rustls-tls in our fork: https://github.com/lakekeeper/iceberg-rust/blob/a8b6509775b139c92772a999b0cbca637e274a03/Cargo.toml#L100

@Xuanwo
Copy link
Member

Xuanwo commented May 22, 2025

I'm wondering if it would be better for us not to rely on reqwest with the default features enabled, and instead let users decide which features to include. This way, we wouldn't need to wrap those feature flags ourselves.

@c-thiel
Copy link
Collaborator Author

c-thiel commented May 22, 2025

Hm, I see in the workspace we are already disabling default features for reqwest.
We still pulled the vendored-openssl in somehow last time I checked (~4 months ago). Let me double check if we actually need this.

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

Successfully merging this pull request may close these issues.

3 participants