Skip to content

reqwest: re-apply 0.13 bump at 0.13.4 with explicit TLS backend pin - #37469

Merged
jasonhernandez merged 2 commits into
mainfrom
jason/reqwest-013-retry
Jul 7, 2026
Merged

jasonhernandez merged 2 commits into
mainfrom
jason/reqwest-013-retry

Conversation

@jasonhernandez

@jasonhernandez jasonhernandez commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Re-applies the reqwest 0.12 → 0.13 bump (originally #36215, reverted in #36241 when the Kafka auth schema-registry TLS tests started flaking on main). Bumps reqwest to 0.13.4, reqwest-middleware to 0.5.2, and reqwest-retry to 0.9.1. This is only the version bump — the TLS backend stays native-tls; the switch to rustls is owned separately by the crypto migration (#35947).

Why it should stick this time

The April flake was an OpenSSL PEM routines:get_name:no start line error at connection time on the ccsr (schema registry) client, on requests whose TLS handshake should have succeeded. Three things are different now:

  1. reqwest 0.13.4, not 0.13.2. Upstream has since fixed several bugs in the 0.13 TLS config layer: PEM parsing of CRL values plus a rustls-platform-verifier upgrade (0.13.3), native-tls ALPN combinations (late 0.13.2), and TLS 1.3 for native-tls (0.13.4). Note the tree already ships reqwest 0.13.3 transitively via sentry 0.48, so reqwest 0.13 is not itself new to production images.
  2. ccsr pins tls_backend_native() explicitly. In reqwest 0.13, rustls becomes the default backend whenever any transitive dependency compiles the rustls feature in (ours do — the lockfile carries rustls-platform-verifier/quinn), while ccsr's certificates and identities are prepared for native-tls. Silent backend/verifier drift on this exact client is the most plausible mechanism for the April flake, and the explicit pin removes that degree of freedom regardless of which theory is correct.
  3. Cleaner handling of the reqwest-0.12-only ecosystem pins. The Azure transport client now comes from the reqwest_0_12 workspace alias, keeping the BlobKnobs timeout plumbing intact on both the emulator and production paths (azure_core 0.21 implements HttpClient only for reqwest 0.12; April dropped the emulator-path timeouts to work around this). The alias also covers iceberg's RequestAuthenticator, which gained a second impl (Sigv4Authenticator) since April.

Validation plan

The Kafka auth suite is the regression signal that matters here (it exercises the exact schema-registry TLS path that flaked). It was disabled in CI when this PR was first opened (SS-115), but #37470 re-enabled it and merged, and this branch is now rebased on top — so Kafka auth 1–3 run on this PR's CI and directly exercise the April failure mode.

Because that failure was intermittent, a single green run isn't sufficient. The mz-test skill's flake-handling guidance applies: re-trigger the build 10–20 times (via https://ci.dev.materialize.com/trigger, clearing the target branch for parallel runs) and confirm Kafka auth stays green before merging.

  • cargo check --workspace --all-targets
  • cargo --locked deny check licenses bans sources, bin/lint-cargo, bin/fmt
  • Kafka auth suite green locally (8/8 consecutive runs) on this stack
  • Kafka auth green across 10–20 CI re-triggers (now runs here post-rebase, per the mz-test skill)

🤖 Generated with Claude Code

@jasonhernandez
jasonhernandez marked this pull request as ready for review July 7, 2026 11:56
@jasonhernandez
jasonhernandez requested review from a team as code owners July 7, 2026 11:56
@jasonhernandez
jasonhernandez requested a review from def- July 7, 2026 12:07

@def- def- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope it's green now. Where is this flake playbook btw?

jasonhernandez added a commit that referenced this pull request Jul 7, 2026
… 8.x) (#37470)

## Summary

Re-enables the Kafka auth test suite, disabled since #36405 (SS-115),
and fixes the two breakages that accumulated while it was dark.

## What broke while the suite was disabled

The Confluent Platform 7.9.4 → 8.2.0 bump (#36683) landed three weeks
after the suite was skipped, and broke its setup in two ways nothing
could catch:

1. **Schema registry basic auth returned 401 for everyone.** CP 8.x
ships Jetty 12, which moved the JAAS login modules from
`org.eclipse.jetty.jaas.spi` to `org.eclipse.jetty.security.jaas.spi`.
Our `schema-registry.jaas.config` referenced the old class, so the login
module never loaded and every request, correct credentials included, got
401. Verified directly: `curl -u materialize:sekurity` against the
container → 401 before, 200 after the one-line class-path fix
(wrong/missing creds still 401).
2. **Kafka mssl negative tests asserted TLS 1.2 alert text.** CP 8.x
brokers negotiate TLS 1.3, where a missing/untrusted client certificate
fails with `tlsv13 alert certificate required` (alert 116) instead of
`ssl/tls alert bad certificate` (alert 42). The assertions now check
only that the broker rejected the connection with a TLS alert. The exact
wording is a function of TLS version and OpenSSL build that we do not
control. This is the third exact-text loosening in this suite's history
(#30501, #36232), so the loosening deliberately goes all the way rather
than chasing the new strings.

## On the original SS-115 flake

The `PEM routines:get_name:no start line` flake that got the suite
disabled did not reproduce in local full-suite runs against current main
images (reqwest 0.12) or against the reqwest 0.13.4 branch (#37469).
Timeline evidence from SS-115 itself shows the flake continued after the
reqwest 0.13 revert (#36241), so reqwest was likely misattributed. If it
resurfaces, the `ci-regexp` in SS-115 will link it; the suite being
enabled is what gives us the data to root-cause it.

## Why now

Two in-flight workstreams change exactly the surface this suite covers:
the reqwest 0.13 re-application (#37469) and the rdkafka → AWS-LC switch
(#35941). Landing those with this suite dark is how auth/TLS regressions
reach production unobserved.

## Test plan

- [x] Full suite green locally against main images (all fixes applied)
- [x] Repeated local runs to probe for the SS-115 flake: 8 consecutive
local full-suite passes (5-run loop + 3 individual), plus 2 clean runs
on the reqwest-0.12 main baseline. The PEM flake did not reproduce.
- [ ] Kafka auth 1–3 green in this PR's CI (the un-skip makes them run
here)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Jason Hernandez <[email protected]>
Co-authored-by: Claude Fable 5 <[email protected]>
jasonhernandez and others added 2 commits July 7, 2026 06:24
One of several sequenced PRs in the crypto migration toward rustls +
aws-lc-rs for everything.
Specifically:

- **This PR** — mechanical bump from reqwest 0.12 → 0.13. Keeps the
existing `native-tls` backend for now to scope the change to just the
version bump. Does not flip the TLS backend.
- **Next in the reqwest chain** — the HTTP-clients-to-rustls PR (#35947
in the plan) swaps `native-tls` for `rustls` with `aws-lc-rs` across all
reqwest consumers. That's where the real TLS backend change happens.
- **In parallel** — the Azure SDK migration (#36216, stacked on this)
adopts the new `azure_core` 0.35 series, which defaults to
`reqwest_rustls` + aws-lc-rs on its own; that's a concrete step forward
on the Azure path even before the workspace-wide flip.

- Bump `reqwest` 0.12.28 → 0.13.2, `reqwest-middleware` 0.4.2 → 0.5.1,
`reqwest-retry` 0.8.0 → 0.9.1.
- `default-features = false` on reqwest so the 0.13 default changes (now
`default-tls = rustls`, new `system-proxy` feature) don't ambush us
mid-bump.
- Explicitly list the feature set we had in 0.12 (including `native-tls`
/ `native-tls-vendored` for now) plus the two newly-opt-in features we
need: `query` (used by `cloud-api` and `frontegg-client`) and
`system-proxy` (was implicit in 0.12).

The `native-tls` pin here is a temporary consequence of keeping this PR
scoped to the bump — the crypto migration's HTTP-clients PR is where the
switch to rustls actually lands.

Two places still need reqwest 0.12 because their trait ecosystem is
pinned there. Both are narrow and clearly labeled; both go away as the
related migrations land.

1. **`src/persist/src/azure.rs` — Azurite test path only.** The old code
plumbed a custom `reqwest::Client` through `TransportOptions` inside the
`if account == EMULATOR_ACCOUNT` branch to apply short `BlobKnobs`
per-attempt / read / connect timeouts (5s–10s) to the Azurite test
client. `azure_core` 0.21 pins reqwest 0.12 internally, so our 0.13
`Client` no longer implements its `HttpClient` trait.
- **Production impact: zero.** The `else` branch (real Azure Blob
Storage) never set a custom transport — it uses
`BlobServiceClient::new(...)` which gets the SDK's default reqwest
client. That behavior is unchanged.
- **Test impact: minimal.** Azurite runs on localhost, so falling back
to the SDK's default timeouts has no observable effect on test
reliability.
- The outer `knobs.operation_timeout()` is preserved via the retry
policy's `max_total_elapsed` in both branches.
- The Azure SDK 0.35 migration (#36216, stacked) restores the
emulator-path timeout plumbing against the new SDK.
2. **`src/storage-types/src/connections.rs` — `AwsCredentialLoad`
impl.** The `reqsign::AwsCredentialLoad` trait (re-exported from
iceberg) is defined against reqwest 0.12. Added a versioned
`reqwest_0_12` alias dep in `[workspace.dependencies]` and use it only
for the single trait-impl parameter. Removable once iceberg upstream
bumps to a reqsign that supports reqwest 0.13 — tracked as a multi-hop
ecosystem blocker (reqsign 0.17 was a breaking API rewrite; neither
iceberg nor opendal have migrated).

~27 new packages (quinn for HTTP/3, rustls-platform-verifier,
wasm-streams, newer windows-sys). Both reqwest 0.12 and 0.13 coexist in
the tree — the 0.12 copy comes in via azure_core 0.21 and reqsign 0.16
and is needed only for the two workarounds above. Exempted in
`deny.toml`'s skip list.

- [x] `cargo check --workspace --all-targets` — clean
- [x] `cargo --locked deny check bans sources` — `bans ok`
- [x] `bin/lint-cargo` — exit 0
- [x] Full `cargo test` run / CI green
- [ ] Smoke-test at least one TLS client path (`ccsr`,
`frontegg-client`, or similar) against a real endpoint to confirm
native-tls still works at 0.13 before the crypto migration's rustls
switch lands

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Jason Hernandez <[email protected]>
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Re-application of #36215 (reverted in #36241 for flaky Kafka auth
schema-registry TLS tests) with three changes:

- reqwest 0.13.4 instead of 0.13.2. Upstream has since fixed several
  bugs in the TLS config layer, notably PEM parsing of CRL values and
  native-tls ALPN/TLS 1.3 handling. sentry 0.47 already ships reqwest
  0.13.3 in our tree, so 0.13 itself is not new here.
- ccsr pins its client to tls_backend_native(). reqwest 0.13 defaults
  to rustls whenever transitive deps compile the rustls backend in, and
  ccsr's certificates and identities are prepared for native-tls. The
  April flake surfaced OpenSSL PEM errors at connect time on exactly
  this client, consistent with TLS backend/verifier drift.
- The Azure transport client comes from the reqwest_0_12 alias, keeping
  the BlobKnobs timeout plumbing on both the emulator and production
  paths. azure_core 0.21 implements HttpClient only for reqwest 0.12.
  The alias also covers iceberg's RequestAuthenticator, which gained a
  second impl (Sigv4Authenticator) since April.

Co-Authored-By: Claude Fable 5 <[email protected]>
@jasonhernandez
jasonhernandez force-pushed the jason/reqwest-013-retry branch from 9d33029 to a8106d9 Compare July 7, 2026 13:25
@jasonhernandez

Copy link
Copy Markdown
Contributor Author

Let's hope it's green now. Where is this flake playbook btw?

The flake playbook is a reference to the mz-test skill.

I'm going to rebase this on the kafka-auth tests quickly.

@jasonhernandez
jasonhernandez enabled auto-merge (squash) July 7, 2026 13:30
@jasonhernandez
jasonhernandez merged commit 11b283f into main Jul 7, 2026
124 checks passed
@jasonhernandez
jasonhernandez deleted the jason/reqwest-013-retry branch July 7, 2026 13:58
@def-

def- commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

I'm quite concerned that this was merged. It failed multiple times even in the PR, now main is unusable: https://buildkite.com/materialize/test/builds/127623
Screenshot 2026-07-07 at 16 31 16
Why did we retry the runs if the goal is to verify that this test is not flaky anymore?
The validation plan also seems to not have been implemented at all.
Revert here: #37491

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.

2 participants