Skip to content

kafka auth test: Handle different error - #30501

Merged
def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-mssl
Nov 15, 2024
Merged

def- merged 1 commit into
MaterializeInc:mainfrom
def-:pr-mssl

Conversation

@def-

@def- def- commented Nov 15, 2024 •

Copy link
Copy Markdown
Contributor

Failed 16 times in CI: https://ci-failures.dev.materialize.com/?key=test-failures&tfFilters=%5B%7B%22id%22%3A%22build_date%22%2C%22value%22%3A%5Bnull%2Cnull%5D%7D%2C%7B%22id%22%3A%22content%22%2C%22value%22%3A%22files%20involved%3A%20test-schema-registry-mssl.td%22%7D%5D

I'm not sure if this indicates something worse or is just an inconsistent error message.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@def-
def- requested a review from petrosagg November 15, 2024 14:17
@def-
def- marked this pull request as ready for review November 15, 2024 15:01
@def-
def- requested a review from benesch November 15, 2024 15:01
@benesch

benesch commented Nov 15, 2024

Copy link
Copy Markdown
Contributor

I'm not sure if this indicates something worse or is just an inconsistent error message.

Almost surely just an inconsistency. Hyper has been flaky about producing error messages reliably. "Channel closed" usually indicates a shutdown race in Hyper internals.

@def-
def- merged commit a60af9c into MaterializeInc:main Nov 15, 2024
@def-
def- deleted the pr-mssl branch November 15, 2024 16:08
jasonhernandez added a commit that referenced this pull request Apr 24, 2026
## Summary

Two `!` assertions in
`test/kafka-auth/test-schema-registry-mssl{-basic,}.td` have been flaky
for over a year because the exact TLS error text varies with the TLS
backend, reqwest version, and OpenSSL build in use. The tests only
verify that a disallowed-client-cert connection fails — the wording is
"not under our control" (as the comment in the tests already says).

Switch both `contains:alert certificate unknown` to `contains:error
sending request for url` — the outer reqwest wrapper that every failure
mode we've observed shares.

## Precedent

- [#30501](#30501)
(Nov 2024) loosened the same test family for the same reason, citing 16
CI failures over an inconsistent error string. Dennis's commit message:
*"I'm not sure if this indicates something worse or is just an
inconsistent error message."*

## Variants observed

- `ssl/tls alert certificate unknown` (native-tls + OpenSSL, current
main)
- `alert bad certificate` (pre-2024 wording, fixed by #30501)
- `CertificateUnknown` (rustls; from the crypto migration branch)
- `PEM routines:get_name:no start
line:crypto/pem/pem_lib.c:773:Expecting: CERTIFICATE` (reqwest 0.13 on
Linux, intermittent)

## Why separate from the reqwest bump

The reqwest 0.13 bump
([#36215](#36215))
surfaces the OpenSSL PEM variant more often, which is how I noticed this
— but it's not the root cause. The flakiness is latent on `main` and
will bite any subsequent TLS-stack change (rustls migration, OpenSSL
updates). Hardening the assertion is independently justifiable.

## Test plan

- [ ] `bin/mzcompose --find kafka-auth run default` passes.
- [ ] No other `alert certificate unknown` assertions exist against
these endpoints (verified via `grep`).

🤖 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]>
jasonhernandez added a commit that referenced this pull request Jul 7, 2026
The suite was disabled entirely in #36405 after unexplained PEM flakes
(SS-115). While it was dark, the Confluent Platform 7.9.4 -> 8.2.0 bump
(#36683) broke its setup in two ways that nothing caught:

- 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.
  The schema registry basic-auth realm silently failed to load its
  login module and returned 401 for every request, correct credentials
  included. Point the jaas.config at the new class path.
- CP 8.x brokers negotiate TLS 1.3, where a missing or untrusted
  client certificate fails with different alert text (certificate
  required, alert 116) than TLS 1.2 (bad certificate, alert 42). The
  kafka mssl tests asserted the exact TLS 1.2 wording. Assert only
  that the broker rejected the connection with a TLS alert, since the
  exact text varies with TLS version and OpenSSL build. This is the
  third exact-text loosening in this suite (#30501, #36232).

With both fixes the full suite passes locally against current main
images. The original SS-115 PEM flake did not reproduce in local runs
on either reqwest 0.12 (main) or the 0.13.4 branch.

Co-Authored-By: Claude Fable 5 <[email protected]>
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]>
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