Repository navigation
Revert "Bump reqwest to 0.13 (#36215)" - #36241
Merged
Merged
Conversation
This reverts commit 6d7c3bb. Causing a lot of flakiness in CI: https://buildkite.com/materialize/test/builds/121629
jasonhernandez
approved these changes
Apr 24, 2026
Contributor
|
Thanks, I'll try and look into this later. |
This was referenced Jul 7, 2026
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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit 6d7c3bb.
Causing a lot of flakiness in CI: https://buildkite.com/materialize/test/builds/121629 and https://buildkite.com/materialize/test/builds/121626
@jasonhernandez Alternatively you could also fix the test failures, or we could disable them.