feat(lance-io): select TLS provider to keep aws-lc-rs out of non-AWS builds - #8445
feat(lance-io): select TLS provider to keep aws-lc-rs out of non-AWS builds#8445valkum wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d33e6ba7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The dependency split is directionally sound, but there is still no functional AWS-LC-free OpenDAL configuration: the new tls-no-provider path re-enables AWS-LC through OpenDAL's compatibility alias, while the bare configurations checked by the new guard have no HTTP transport and fail at runtime.
A viable revision should attach a genuinely provider-neutral transport, test an actual non-AWS request under that feature set, and forward the TLS mode through every public facade. The existing namespace feature discussion remains applicable.
| # TLS provider for opendal stores. `tls-no-provider` keeps aws-lc-rs/cmake out but | ||
| # requires the app to install a rustls CryptoProvider (e.g. ring) before first use. | ||
| tls-aws-lc-rs = ["opendal?/reqwest-rustls-tls"] | ||
| tls-no-provider = ["opendal?/reqwest-rustls-no-provider-tls"] |
There was a problem hiding this comment.
This feature still resolves aws-lc-rs, aws-lc-sys, and cmake, so it does not provide the advertised AWS-LC-free OpenDAL transport. OpenDAL's compatibility feature also enables http-transport-reqwest, which expands to its AWS-LC Rustls transport. The new guard misses this because it checks backend features without either transport feature.
Use a provider-neutral transport path that does not also enable http-transport-reqwest-rustls—for example, construct the direct no-provider reqwest transport and attach it through OpenDAL's operation context—then make the guard exercise that working configuration.
Reproducer
Run on this head:
cargo tree --locked -p lance-io --no-default-features \
--features gcp,tls-no-provider -e features -i aws-lc-rsExpected: no matching package. Observed: lance-io/tls-no-provider -> opendal/reqwest-rustls-no-provider-tls -> opendal/http-transport-reqwest -> opendal/http-transport-reqwest-rustls -> reqwest/rustls -> aws-lc-rs.
There was a problem hiding this comment.
The AWS-LC edge in the original reproducer is fixed on this head, but the finding remains through a different verified mechanism: the two granular features link opendal-http-transport-reqwest without enabling the opendal/http-transport-reqwest feature gate that calls install_default(). As a result, both advertised modes fail before making an HTTP request. Crate presence in the new script does not prove that the transport was installed.
Please explicitly construct/install or attach the transport (preferably per operator), propagate a mode through each storage-facing consumer, and retain a request-level regression test.
Current-head reproducer
I added this disposable test at rust/lance-io/tests/opendal_transport_probe.rs:
use std::io::{Read, Write};
use std::net::TcpListener;
use std::thread;
use std::time::{Duration, Instant};
use opendal::{services::Oss, Operator};
#[tokio::test]
async fn oss_feature_installs_default_http_transport() {
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
listener.set_nonblocking(true).unwrap();
let address = listener.local_addr().unwrap();
let server = thread::spawn(move || {
let deadline = Instant::now() + Duration::from_secs(2);
loop {
match listener.accept() {
Ok((mut stream, _)) => {
let mut request = [0; 4096];
let size = stream.read(&mut request).unwrap();
stream.write_all(
b"HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n",
).unwrap();
return String::from_utf8_lossy(&request[..size]).into_owned();
}
Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => {
if Instant::now() >= deadline { return String::new(); }
thread::sleep(Duration::from_millis(10));
}
Err(error) => panic!("listener failed: {error}"),
}
}
});
let operator = Operator::new(
Oss::default()
.endpoint(&format!("http://{address}"))
.addressing_style("path")
.bucket("bucket")
.skip_signature(),
).unwrap();
let result = operator.stat("probe").await;
let request = server.join().unwrap();
assert!(
request.starts_with("HEAD /bucket/probe "),
"no OSS HTTP request reached the endpoint; OpenDAL result: {result:?}"
);
}Both of these commands exited 101, no request reached the listener, and stat returned ConfigInvalid: default HTTP transport is not installed:
cargo test --locked --manifest-path rust/lance-io/Cargo.toml --no-default-features --features oss,tls-no-provider --test opendal_transport_probe
cargo test --locked --manifest-path rust/lance-io/Cargo.toml --no-default-features --features oss,tls-aws-lc-rs --test opendal_transport_probeThere was a problem hiding this comment.
It is up to the user to select which TLS provider to use when enabling tls-no-provider. Else we end up with a big tie-in, the ecosystem is trying to solve (namely, each lib having a install_default())
There was a problem hiding this comment.
Agreed that tls-no-provider should not install or select a rustls CryptoProvider; that is separate from the missing installation reported here. The current failure is OpenDAL saying its default HTTP transporter is not installed. http-transport-reqwest-rustls-no-provider links the provider-neutral implementation, but only the separate http-transport-reqwest feature installs it as OpenDAL’s default. Installing a rustls provider in the application therefore does not make the OSS probe reach the socket.
The finding remains, but it does not require Lance to choose a crypto provider: construct the provider-neutral reqwest transport and attach it per operator (or install only that transport), while leaving CryptoProvider selection to the application.
…builds opendal defaults to reqwest's rustls TLS, which pins the aws-lc-rs crypto provider and its cmake build dependency into every build. Build opendal with default-features = false and add tls-aws-lc-rs (default) and tls-no-provider features so non-AWS backends can ride on ring or a no-provider rustls instead. These map to opendal 0.58's granular http-transport-reqwest-rustls[-no-provider] features rather than the reqwest-rustls-* aliases, which additionally re-enable the aws-lc-rs transport. tls-no-provider still yields a working reqwest transport; the application installs a rustls CryptoProvider before first use. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cargo-deny resolves the dependency graph with all features enabled, so it cannot assert that aws-lc-rs is absent from a specific feature combo. Add a script that resolves each non-AWS backend under tls-no-provider and fails if aws-lc-rs, aws-lc-sys, aws-lc-fips-sys, or cmake appears. It checks the functional config and requires a reqwest HTTP transport so a transport-less build cannot pass vacuously. Run it from the cargo-deny job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
71ee303 to
8311c44
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The original AWS-LC feature edge is fixed, but the transport contract is not: both TLS modes link the reqwest implementation without installing it, storage-facing consumers do not consistently select or forward a mode, and the non-AWS dependency promise is broader than the configurations checked by the guard. The existing namespace facade finding remains applicable.
A viable revision should explicitly construct/install or attach the transport, forward one mode through every storage-facing consumer, and prove both an actual request and every backend covered by the dependency promise.
|
|
||
| check "lance-io (gcp, tls-no-provider)" --manifest-path rust/lance-io/Cargo.toml --no-default-features --features gcp,tls-no-provider | ||
| check "lance-io (azure, tls-no-provider)" --manifest-path rust/lance-io/Cargo.toml --no-default-features --features azure,tls-no-provider | ||
| check "lance-io (oss, tls-no-provider)" --manifest-path rust/lance-io/Cargo.toml --no-default-features --features oss,tls-no-provider |
There was a problem hiding this comment.
The stated non-AWS invariant is still false for a supported backend. I ran cargo tree --locked -p lance-io --no-default-features --features huggingface,tls-no-provider -e normal,build -i aws-lc-rs; it resolves opendal-service-huggingface -> hf-xet -> xet-client/default -> reqwest/rustls -> aws-lc-rs, along with aws-lc-sys and cmake. Because this list omits Hugging Face, the guard reports success while that feature violates the compile-footprint contract. Either make the provider-neutral configuration work for Hugging Face too, or explicitly narrow and document the promise and make the checked set match it.
There was a problem hiding this comment.
There currently is no way of making the huggingface feature aws-lc-rs free. Users of the huggingface feature are exempt from this addition until opendal-service-huggingface allows setting something like tls-no-provider.
There was a problem hiding this comment.
That upstream constraint is a reasonable reason to choose the exemption path rather than removing AWS-LC from Hugging Face now. The current PR still states that this script enforces the promise for “each non-AWS backend,” though, and neither the script nor feature documentation records this exception.
The finding therefore remains only as a contract mismatch: explicitly narrow the promise and document Hugging Face as exempt until the upstream service supports a provider-neutral transport, then make the checked backend set match that documented scope.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
I filed huggingface/xet-core#957 to unblock this. Still needs work in opendal after huggingface/xet-core#957 is merged. |
opendaldefaults to reqwest'srustlsTLS, which pins theaws-lc-rscrypto provider and itscmakebuild dependency into every build.This changes opendal to use
default-features = falseand addstls-aws-lc-rs(default) andtls-no-providerfeatures so non-AWS backends can ride on ring or a no-provider rustls instead.tls-no-providerrequires the application to install a rustls CryptoProvider before first use (as is required by rustls now).This allows users of
lanceand (with another PR in LanceDB) LanceDB users to keep usingringwithout buildingaws-lc-rs.This also adds a CI check to avoid bringing
aws-lc-rsback as a hard requirement accidentally again.This PR was partially created with Claude.