Skip to content

Revert "Bump reqwest to 0.13 (#36215)" - #36241

Merged
def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-tls-reqwest-unflake
Apr 24, 2026
Merged

def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-tls-reqwest-unflake

Conversation

@def-

@def- def- commented Apr 24, 2026 •

Copy link
Copy Markdown
Contributor

@def-
def- requested a review from jasonhernandez April 24, 2026 04:40
@def-
def- requested review from a team as code owners April 24, 2026 04:40
@jasonhernandez

Copy link
Copy Markdown
Contributor

Thanks, I'll try and look into this later.

@def-
def- merged commit cc8d06d into MaterializeInc:main Apr 24, 2026
120 checks passed
@def-
def- deleted the pr-tls-reqwest-unflake branch April 24, 2026 06:52
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 added a commit that referenced this pull request Jul 7, 2026
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 added a commit that referenced this pull request Jul 7, 2026
…37469)

## 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.

- [x] `cargo check --workspace --all-targets`
- [x] `cargo --locked deny check licenses bans sources`,
`bin/lint-cargo`, `bin/fmt`
- [x] 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](https://claude.com/claude-code)

---------

Co-authored-by: Jason Hernandez <[email protected]>
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
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