feat(tls): rfc 0030 green (acceptor) — gRPC + HTTP listeners over TLS - #447
Merged
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds TLS support for the gRPC and HTTP receiver paths, wires TLS settings through the server configuration, adds handshake-failure metrics and semantic conventions, and replaces RFC0030 TLS stubs with concrete integration tests. ChangesTLS receiver support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TcpListener
participant tls_incoming
participant TlsListener
participant TlsAcceptor
TcpListener->>tls_incoming: TcpStream
tls_incoming->>TlsAcceptor: accept() with timeout
TlsAcceptor-->>tls_incoming: TlsStream or error
TcpListener->>TlsListener: accept() TcpStream
TlsListener->>TlsAcceptor: accept() with timeout
TlsAcceptor-->>TlsListener: TlsStream or error
sequenceDiagram
participant Main
participant ReceiverConfig
participant ServeGrpc
participant ServeHttp
participant TlsListener
Main->>ReceiverConfig: grpc_tls, http_tls from params
ReceiverConfig->>ServeGrpc: serve() checks config.grpc_tls
alt grpc_tls Some
ServeGrpc->>ServeGrpc: build acceptor with ALPN_GRPC, tls_incoming
else grpc_tls None
ServeGrpc->>ServeGrpc: serve plaintext
end
ReceiverConfig->>ServeHttp: serve() checks config.http_tls
alt http_tls Some
ServeHttp->>TlsListener: new(listener, acceptor with ALPN_HTTP)
ServeHttp->>ServeHttp: axum::serve(TlsListener, ...)
else http_tls None
ServeHttp->>ServeHttp: axum::serve plaintext
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0030 “acceptor” slice by wiring optional TLS termination into the server’s gRPC + HTTP receiver listeners using tokio-rustls, and turns several RFC 0030 TLS scenarios into real loopback-handshake integration tests.
Changes:
- Add
TlsSettings::acceptor(alpn)plus newtls_serveadapters (tls_incoming,TlsListener) for terminating TLS per-connection without taking the whole listener down on handshake errors. - Branch
ourios-server::receiver::serve()to serve each listener as TLS-or-plaintext based onreceiver.{grpc,http}_tls. - Un-ignore and implement RFC0030.1/.2/.9 integration tests using rcgen-minted certs (no committed key material).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-server/src/receiver.rs | Adds optional TLS acceptors and branches gRPC/HTTP serve loops to wrap incoming connections with TLS. |
| crates/ourios-server/src/main.rs | Plumbs grpc_tls / http_tls from resolved params into ReceiverConfig. |
| crates/ourios-server/Cargo.toml | Enables tonic’s tls-connect-info feature to support externally-produced TlsStreams. |
| crates/ourios-ingester/tests/it/rfc0030_tls.rs | Implements real TLS handshake tests for RFC0030.1/.2/.9 with rcgen certs and loopback clients. |
| crates/ourios-ingester/src/receiver/tls.rs | Adds TlsSettings::acceptor(alpn) and ALPN constants for gRPC vs HTTP listeners. |
| crates/ourios-ingester/src/receiver/tls_serve.rs | New TLS-wrapping adapters for tonic incoming streams and axum listener trait. |
| crates/ourios-ingester/src/receiver.rs | Exposes the new tls_serve module. |
| crates/ourios-ingester/Cargo.toml | Adds tokio-rustls + stream/log deps for TLS serving and reqwest/tonic dev deps for tests. |
| Cargo.lock | Locks new dependencies introduced by TLS serving and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/ourios-ingester/src/receiver/tls.rs (1)
219-223: 🧹 Nitpick | 🔵 TrivialTLS listener subsystem lacks Prometheus metrics and Ourios-named structured logs.
The coding guidelines for
ourios-{ingester,querier,server}require Prometheus metrics for every subsystem, structured logs via Ourios on hot paths, and tracing every RPC — "'We'll add metrics later' is a PR rejection." The TLS listener is a new subsystem, but thetls_serve.rsadapters (visible in context but in a separate review cohort) only emittracing::debug!withoutourios_semconv::EVENT_*names and have no Prometheus metrics (e.g., TLS handshake success/failure counters, active TLS connection gauge). Consider adding these before merging the TLS listener cohort.As per coding guidelines: "Use Prometheus metrics for every subsystem, emit structured logs via Ourios on hot paths, and trace every RPC. 'We'll add metrics later' is a PR rejection."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-ingester/src/receiver/tls.rs` around lines 219 - 223, The TLS listener subsystem is missing required Prometheus metrics and Ourios-structured logging on its hot path. Update the TLS listener/adapter code around the TLS serve path and related helpers (for example the `ALPN_GRPC`/`ALPN_HTTP`-backed listener flow and the `tls_serve.rs` adapters) to emit `ourios_semconv::EVENT_*` structured logs instead of plain `tracing::debug!`, and add Prometheus instrumentation for handshake success/failure plus active TLS connection tracking. Ensure the new metrics and log events are wired through the TLS listener subsystem consistently with the existing `ourios-ingester` conventions.Source: Coding guidelines
crates/ourios-ingester/tests/it/rfc0030_tls.rs (1)
99-109: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound expected-failure handshakes with test timeouts.
These negative-path awaits depend on the peer closing promptly. A regression could hang until the global CI timeout; wrapping each in
tokio::time::timeoutkeeps failures local and diagnosable.Also applies to: 167-172, 452-456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-ingester/tests/it/rfc0030_tls.rs` around lines 99 - 109, The negative-path awaits in the TLS tests can hang if the peer does not close promptly, so wrap the failing connect/export awaits in tokio::time::timeout to keep the failure bounded. Update the affected test cases in rfc0030_tls.rs, including the logic around LogsServiceClient::connect and the subsequent export call, and apply the same timeout pattern to the other referenced handshake assertions in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-ingester/src/receiver/tls_serve.rs`:
- Around line 51-55: The TLS serve path currently only emits a debug log for
accept and handshake failures, so update the hot-path error handling in
tls_serve.rs to record Prometheus counters for both failure cases and include
labels such as listener and error.type. Use Ourios registry-backed structured
event names instead of plain tracing::debug! for these logs, and make sure the
existing TLS accept/handshake failure branches in tls_serve.rs use the shared
event/metrics plumbing consistently.
- Around line 49-56: Add an explicit handshake deadline around
acceptor.accept(...) in both the gRPC stream and the axum listener paths so a
stalled ClientHello cannot block later connections. Update the TLS serving logic
in tls_serve.rs (the stream/serve helpers that call acceptor.accept) to wrap the
await in a timeout and handle timeout errors alongside handshake failures. Also
add a regression test near the existing TLS listener tests that opens a TCP
connection and never completes the handshake, then verifies the listener times
out and continues accepting subsequent connections.
In `@crates/ourios-ingester/tests/it/rfc0030_tls.rs`:
- Around line 156-158: The HTTPS success-path request in the RFC0030 TLS test is
using localhost instead of the IPv4 listener address, which can resolve to ::1
and fail before TLS is exercised. Update the request built with client.post in
rfc0030_tls::test to target 127.0.0.1 using addr.port(), keeping it consistent
with the listener and the SAN minted by the test helper.
---
Nitpick comments:
In `@crates/ourios-ingester/src/receiver/tls.rs`:
- Around line 219-223: The TLS listener subsystem is missing required Prometheus
metrics and Ourios-structured logging on its hot path. Update the TLS
listener/adapter code around the TLS serve path and related helpers (for example
the `ALPN_GRPC`/`ALPN_HTTP`-backed listener flow and the `tls_serve.rs`
adapters) to emit `ourios_semconv::EVENT_*` structured logs instead of plain
`tracing::debug!`, and add Prometheus instrumentation for handshake
success/failure plus active TLS connection tracking. Ensure the new metrics and
log events are wired through the TLS listener subsystem consistently with the
existing `ourios-ingester` conventions.
In `@crates/ourios-ingester/tests/it/rfc0030_tls.rs`:
- Around line 99-109: The negative-path awaits in the TLS tests can hang if the
peer does not close promptly, so wrap the failing connect/export awaits in
tokio::time::timeout to keep the failure bounded. Update the affected test cases
in rfc0030_tls.rs, including the logic around LogsServiceClient::connect and the
subsequent export call, and apply the same timeout pattern to the other
referenced handshake assertions in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1061377-c5a3-40b8-8383-a709ffdb8e39
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ourios-ingester/Cargo.tomlcrates/ourios-ingester/src/receiver.rscrates/ourios-ingester/src/receiver/tls.rscrates/ourios-ingester/src/receiver/tls_serve.rscrates/ourios-ingester/tests/it/rfc0030_tls.rscrates/ourios-server/Cargo.tomlcrates/ourios-server/src/main.rscrates/ourios-server/src/receiver.rs
The RFC0030.1/.2/.9 slice. TlsSettings::acceptor(alpn) builds each listener's tokio-rustls TlsAcceptor (ring pinned; h2-only for gRPC, h2+http/1.1 for HTTP). Two adapters in receiver::tls_serve feed the handshaked streams to their framework: tls_incoming wraps tonic's TcpIncoming into a TlsStream stream for serve_with_incoming; TlsListener implements axum::serve::Listener over a TcpListener. Both absorb per-connection handshake failures (log + drop) so one bad client can't take the listener down. ourios-server's serve() branches each listener TLS-or-plaintext on the config's grpc_tls/http_tls. tonic gains only (the Connected impl for externally- produced TlsStreams, plus the peer-cert hook the mTLS slice will use) — NOT tonic's own TLS acceptor, per RFC §3.2/§4. Verified in-tree before committing to the design. Tests (real handshakes over loopback): .1 TLS gRPC export lands + a plaintext dial of the port fails; .2 HTTPS post lands + plaintext fails; .9 a TLS-1.2-only client is refused by a 1.3-pinned server while a 1.3 client succeeds (raw tokio-rustls — a tonic/reqwest client can't be version-pinned). All certs rcgen-minted at test time. .4 (mTLS) and .6 (reload) remain stubs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six review points from the acceptor slice: - Handshake stall (gRPC + HTTP): each handshake now runs as its own task under a 10s deadline (HANDSHAKE_TIMEOUT), so a client that connects but never finishes its ClientHello (slowloris / stalled peer) is dropped without blocking accepts or other handshakes. tls_incoming funnels completed handshakes through an mpsc channel; TlsListener::accept drives new accepts + a JoinSet of in-flight handshakes concurrently. Regression test proves a stalled connection doesn't block a healthy one. - ALPN: the HTTP listener now advertises http/1.1 ONLY — axum is built with just the http1 feature, so offering h2 let a dual-protocol client negotiate a version the server can't serve. - Observability (§6.3): dropped handshakes increment ourios.receiver.tls.handshake_failures (listener + cause dimensions, weaver-registered) — a dropped connection reaches neither the auth layer nor the WAL, so the counter is its only signal. - Test: the HTTPS success path dials 127.0.0.1 (a cert SAN) rather than localhost, which can resolve to ::1 where nothing listens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second review round on the acceptor slice: - Handshake-task DoS: cap in-flight handshakes per listener (MAX_CONCURRENT_HANDSHAKES=256). tls_incoming acquires a semaphore permit before spawning (backpressure to the accept backlog); TlsListener::accept disables its accept arm at the cap so select! only drains completions until a slot frees — no unbounded task growth under a stall flood. - Diagnostics: handshake-failure logs now carry the peer address (captured before the stream is consumed). - ALPN docs: TlsSettings::acceptor and the server serve() comment now say http/1.1-only, matching ALPN_HTTP. - handshake() doc: , not (it returns Option). - Drop the now-unused async-stream dep (the adapters use ReceiverStream + JoinSet, no async_stream! macro). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
rfc0030-green-acceptor
branch
from
July 9, 2026 19:37
55308b8 to
aee55c3
Compare
The detached tls_incoming driver owned the listener and only noticed receiver-closure on an accept-error send, so on gRPC server shutdown it kept accepting and handshaking indefinitely. It now selects on tx.closed() and breaks when tonic drops the stream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Second RFC 0030 green slice: RFC0030.1 / .2 / .9 un-ignored and passing with real TLS handshakes. Five of the nine §5 scenarios are now green (config #442 + this).
What
TlsSettings::acceptor(alpn)(ingesterreceiver::tls) builds each listener'stokio-rustlsTlsAcceptorfrom freshly-read PEM (ring pinned; ALPNh2for gRPC,h2+http/1.1for HTTP).receiver::tls_serve— two adapters feeding handshaked streams to their framework:tls_incoming: wraps tonic'sTcpIncominginto aTlsStreamstream forserve_with_incoming.TlsListener: implementsaxum::serve::Listenerover aTcpListener.ourios-server::receiver::serve()branches each listener TLS-or-plaintext on the config'sgrpc_tls/http_tls(carried since feat(tls): rfc 0030 green (config) — *_tls blocks, preflight, plaintext warning #442, now consumed).The one design-critical dependency call
tonic gains only
tls-connect-info— theConnectedimpl for externally-producedTlsStreams (plus thepeer_certificates()hook the mTLS slice needs), which is["dep:tokio-rustls"]and not tonic's own TLS acceptor (tls-ring/tls-aws-lc). I verified this against the tonic 0.14.6 source before writing code, so the RFC §3.2/§4 "one rustls wiring, no tonic-tls feature" design holds exactly as specified — no amendment needed.Tests (real handshakes over loopback)
http://to the TLS port fails.tokio-rustls, since a tonic/reqwest client can't be pinned to a single version — the handshake is exactly whatmin_versiongoverns).rcgen-minted at test time (SANslocalhost+127.0.0.1); no committed key material.Remaining
.4(mTLS require-and-verify) and.6(cert reload) stay#[ignore]d stubs — the mTLS and reload green slices..3/.7/.8are the server-harness arms (.7 already green in #442).Verification
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warningsexit 0 (two justified allows onserve: orchestration length +large_futureson atokio::spawned task that heap-allocates anyway), the four rfc0030 tests pass (.1/.2/.5/.9),.4/.6ignored.🤖 Generated with Claude Code
Summary by CodeRabbit