Skip to content

test(storage): cover GCS HTTPS OpenDAL TLS regression#6479

Closed
palkakrzysiek wants to merge 6 commits into
quickwit-oss:fix/gcs-opendal-tlsfrom
palkakrzysiek:codex/fix-gcs-opendal-tls
Closed

test(storage): cover GCS HTTPS OpenDAL TLS regression#6479
palkakrzysiek wants to merge 6 commits into
quickwit-oss:fix/gcs-opendal-tlsfrom
palkakrzysiek:codex/fix-gcs-opendal-tls

Conversation

@palkakrzysiek
Copy link
Copy Markdown

@palkakrzysiek palkakrzysiek commented May 30, 2026

Description

Draft suggestion for #6478 / #6477.

This PR is intentionally based on quickwit-oss:fix/gcs-opendal-tls and leaves the dependency fix mechanism from #6478 untouched:

opendal = { version = "0.56", default-features = false, features = ["reqwest-rustls-tls"] }

The purpose of this branch is to add regression coverage proving that Quickwit's GCS storage path can perform a verified HTTPS object read when the workspace-level OpenDAL TLS feature is enabled.

The test is self-contained and does not depend on public GCS, internet access, credentials, Docker, or the host root store:

  • it starts a local TLS server on an ephemeral 127.0.0.1 port;
  • the server certificate has SANs for localhost and 127.0.0.1 and is valid from 2020 to 3020;
  • the test client trusts only the embedded test CA, so certificate verification stays enabled;
  • the OpenDAL GCS backend is pointed at that HTTPS endpoint;
  • Quickwit builds OpendalStorage through a test-only GCS constructor that shares the production initializer and only injects the local-CA HTTP client;
  • the assertion uses Quickwit's production Storage::get_slice implementation and verifies the returned bytes;
  • the server validates that the request is the expected GCS object read path with the expected Range header.

The custom reqwest client is only there to install the local test CA. I checked whether a process-global CA file could remove that plumbing, but reqwest 0.13's rustls path uses rustls-platform-verifier: SSL_CERT_FILE is honored by the Unix/WebPKI verifier, but not by the Apple platform verifier used on macOS. Relying on a globally installed OS certificate would make the test machine-dependent, so the local CA stays explicit.

If fmassot wants to merge the regression coverage, this can be used as a direct suggestion/stacked PR. If not, the dependency fix in #6478 remains independent.

How was this PR tested?

  • Positive check: cargo test --locked -p quickwit-storage --features gcs --lib test_gcs_storage_get_slice_over_https_with_verified_tls
  • Existing GCS unit tests: cargo test --locked -p quickwit-storage --features gcs --lib opendal_storage::google_cloud_storage::tests
  • Build check: cargo check --locked -p quickwit-storage --features gcs
  • Format/check whitespace: rustfmt --edition 2024 --check quickwit-storage/src/opendal_storage/base.rs quickwit-storage/src/opendal_storage/google_cloud_storage.rs && git diff --check
  • Feature check: cargo tree --locked -e features -i reqwest@0.13.3 -p quickwit-storage --features gcs shows opendal feature "reqwest-rustls-tls" enabling reqwest feature "rustls".
  • Negative check: temporarily changed the base branch dependency back to opendal = { version = "0.56", default-features = false } and ran cargo test --offline -p quickwit-storage --features gcs --lib test_gcs_storage_get_slice_over_https_with_verified_tls; it failed because reqwest_013::Certificate and ClientBuilder::tls_certs_only are gated behind reqwest's TLS feature.

@palkakrzysiek palkakrzysiek requested review from a team as code owners May 30, 2026 12:39
@palkakrzysiek
Copy link
Copy Markdown
Author

minimal change in #6478 should be good enough for now

@palkakrzysiek palkakrzysiek reopened this May 30, 2026
@palkakrzysiek palkakrzysiek marked this pull request as draft May 30, 2026 13:15
@palkakrzysiek palkakrzysiek force-pushed the codex/fix-gcs-opendal-tls branch from be414ae to d9a02a6 Compare May 30, 2026 13:22
@palkakrzysiek palkakrzysiek changed the title fix(storage): restore TLS for GCS OpenDAL backend test(storage): cover GCS HTTPS OpenDAL TLS regression May 30, 2026
@palkakrzysiek palkakrzysiek changed the base branch from main to fix/gcs-opendal-tls May 30, 2026 13:22
@palkakrzysiek palkakrzysiek force-pushed the codex/fix-gcs-opendal-tls branch 5 times, most recently from 8160d4e to 2b4e8a8 Compare May 30, 2026 14:18
@palkakrzysiek palkakrzysiek force-pushed the codex/fix-gcs-opendal-tls branch from 2b4e8a8 to f7329bb Compare May 30, 2026 14:20
@palkakrzysiek palkakrzysiek marked this pull request as ready for review May 30, 2026 14:23
@trinity-1686a
Copy link
Copy Markdown
Contributor

it's odd, that test seems to work when run with cargo test, but not with cargo nextest

        FAIL [   0.011s] quickwit-storage opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls

--- STDOUT:              quickwit-storage opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls ---

running 1 test
test opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls ... FAILED

failures:

failures:
    opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 63 filtered out; finished in 0.00s


--- STDERR:              quickwit-storage opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls ---

thread 'opendal_storage::google_cloud_storage::tests::test_gcs_storage_get_slice_over_https_with_verified_tls' (4656668) panicked at /Users/trinity.pointard/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustls-0.23.40/src/crypto/mod.rs:249:14:

Could not automatically determine the process-level CryptoProvider from Rustls crate features.
Call CryptoProvider::install_default() before this point to select a provider manually, or make sure exactly one of the 'aws-lc-rs' and 'ring' features is enabled.
See the documentation of the CryptoProvider type for more information.
            
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@palkakrzysiek palkakrzysiek force-pushed the codex/fix-gcs-opendal-tls branch from f7329bb to fb1ac2f Compare June 1, 2026 17:35
@palkakrzysiek palkakrzysiek marked this pull request as draft June 1, 2026 17:40
@palkakrzysiek
Copy link
Copy Markdown
Author

converting to draft while investigating build issue...

@palkakrzysiek palkakrzysiek force-pushed the codex/fix-gcs-opendal-tls branch from fb1ac2f to dc9056b Compare June 1, 2026 17:41
@fmassot fmassot deleted the branch quickwit-oss:fix/gcs-opendal-tls June 1, 2026 21:16
@fmassot fmassot closed this Jun 1, 2026
@trinity-1686a
Copy link
Copy Markdown
Contributor

hum, it looks like github closed the PR when @fmassot merged his. Can you re-open one targeting main?

@palkakrzysiek
Copy link
Copy Markdown
Author

@trinity-1686a thank you for looking into this issue, I checked all the commands used by CI now (I hope there are no surprises left 😅), can you check new PR #6482?
added a fix for crypto provider when running cargo nextest

        // Nextest runs tests in separate processes, so this test must not rely
        // on another rustls user having already selected a process-wide provider.
        let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();

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.

GCS split uploads fail with invalid URL, scheme is not http since opendal 0.56 (reqwest built without TLS)pr

3 participants