feat: add versioned downstream TLS/H2 edge listener - #75
seonghobae wants to merge 64 commits into
Conversation
📝 WalkthroughWalkthrough공유 게이트웨이와 PG-ERD 마이그레이션에 다운스트림 TLS/H2 설정 계약을 추가했습니다. 인증서와 개인 키를 Pingora TLS 설정으로 변환합니다. 두 실행 파일은 TLS 설정이 있으면 TLS 리스너를 사용하고, 없으면 TCP 리스너를 사용합니다. 실환경 테스트는 H2와 HTTP/1.1 폴백을 검증합니다. Changes다운스트림 TLS/H2 지원
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant TLSDelivery
participant Pingora
participant Upstream
Client->>Gateway: TLS 연결 및 ALPN 제안
Gateway->>TLSDelivery: build_downstream_tls_settings
TLSDelivery-->>Gateway: Pingora TLS 설정
Gateway->>Pingora: TLS 리스너 등록
Client->>Pingora: H2 또는 HTTP/1.1 요청
Pingora->>Upstream: cleartext HTTP 요청
Upstream-->>Pingora: HTTP 응답
Pingora-->>Client: TLS 응답
Merge Risk: 🟡 Moderate · up to Handshake or startup regressions can leave test gateways running or hang the test suite indefinitely. These process-lifecycle issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
@coderabbitai review |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current writer review on 96ce09c03fde641e91e8802cb9590dc56a9f1913: fresh review of the complete-header origin helper found a second evidence-integrity defect in the prior 071293... implementation. TcpStream::set_read_timeout(5s) is an inactivity timeout for each individual read, not a five-second end-to-end header deadline; a continuously progressing but pathological stream could therefore keep the fixture alive beyond its stated bound. The current forward repair establishes one Instant deadline and resets each socket read timeout to only the remaining budget while retaining the 64 KiB header cap. Production Rust, TLS/ALPN behavior, read_ms: 5000, routing/retry authority and response oracles are unchanged. #76 and #77 are being non-force restacked on this parent. COMMENT technical evidence only; new exact CI/Supply Chain/capacity are nonterminal and this is not independent APPROVED, merge, release or cutover credit.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current writer review on dfb2d9b012e1c3c881796775cdff08067dfd53a1: the only movement from 96ce09... is docs/doctoring/DOWNSTREAM_TLS_HTTP2_TRACEABILITY.md. The durable trace now records why complete HTTP/1 header evidence must be read through the empty-line delimiter (RFC 9112), why Rust TcpStream::set_read_timeout is a per-read call bound rather than an end-to-end HTTP-header lifetime primitive, and why the fixture therefore fixes one Instant deadline and supplies only the remaining budget on later reads. It explicitly separates this test-evidence repair from production read_ms and the independent #45/#447 whole-header lifetime gap. Production Rust, TLS/ALPN behavior, routes/retries, traffic oracles and thresholds are unchanged. COMMENT technical evidence only; fresh exact CI/Supply Chain/capacity are queued and no predecessor GREEN or independent approval transfers.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head technical review after CI RCA. The prior exact head failed only the 100% owned-production coverage gate: the configured ALPN callback lived inside TlsSettings, but the real-wire handshake exercised it in a spawned gateway process whose forced test cleanup did not flush LLVM coverage. This exact adds an in-process TLS handshake test that exercises the configured callback without widening production visibility or changing TLS/ALPN/runtime policy. Existing CodeRabbit threads remain resolved. Fresh exact-head CI/Supply Chain/capacity evidence is required before Ready or merge; this COMMENT is not independent approval.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head follow-up after exact CI 34546454816 exposed a formatting-only failure in the newly added in-process ALPN coverage receipt. Job 103100030726 stopped at cargo fmt --check; the emitted diff only wrapped acceptor.accept(stream).expect(...). Exact df70de9... applies that rustfmt output and changes no production behavior or test semantics. Fresh exact CI/Supply Chain/capacity evidence is required before Ready or merge; this COMMENT is not independent approval.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current terminal-gate review on df70de9cc0c77cfc1dabc51de039ac46af47df08: CI 34547405414, Supply Chain 34547405418, and PgErd bounded-origin capacity 34547405428 have all completed successfully. All three prior CodeRabbit inline findings remain resolved. The last delta was rustfmt-only around the in-process ALPN callback coverage receipt and did not change production TLS/ALPN, routing/retry, certificate custody, product auth/business logic, Keyverse, Wardnet, or EgressWeave authority. I found no additional writer-safe source/test/DDD repair on this exact head. This is COMMENT technical evidence only, not independent APPROVED; Ready-for-review is justified, protected merge/release/cutover credit is not.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai approve Current exact |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='df70de9cc0c77cfc1dabc51de039ac46af47df08'
actual="$(git rev-parse HEAD)"
printf 'checked_out_head=%s\nexpected_head=%s\n' "$actual" "$expected"
echo '--- commit summary ---'
git show --no-ext-diff --format=fuller --no-renames --stat HEAD
echo '--- diff from merge base ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --no-ext-diff --check "$base"...HEAD
git diff --no-ext-diff --name-status "$base"...HEAD
echo '--- PR metadata and checks ---'
gh pr view 75 --repo ContextualWisdomLab/pingora-gateway \
--json headRefOid,isDraft,mergeStateStatus,reviewDecision,statusCheckRollup,reviews \
--jq '{headRefOid,isDraft,mergeStateStatus,reviewDecision,statusCheckRollup:[.statusCheckRollup[]|{name,conclusion,status}],reviews:[.reviews[]|{author:.author.login,state,commitId}]}'
echo '--- unresolved review threads ---'
gh api graphql \
-F owner='ContextualWisdomLab' \
-F name='pingora-gateway' \
-F number=75 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
comments(first: 20) {
nodes {
author { login }
commit { oid }
body
path
line
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false))
| map({isOutdated, comments: [.comments.nodes[] | {author: .author.login, commit: .commit.oid, path, line, body}]})'Length of output: 8586 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Rust module outlines ---'
ast-grep outline src/downstream_tls.rs --items all --view expanded
ast-grep outline src/tls_delivery.rs --items all --view expanded
ast-grep outline src/edge_contract.rs --items all --view expanded
ast-grep outline src/migration_admin.rs --items all --view expanded
ast-grep outline src/bin/cwl-pingora-gateway.rs --items all --view expanded
ast-grep outline src/bin/cwl-pingora-pg-erd-migration.rs --items all --view expanded
echo '--- TLS implementation and composition roots ---'
cat -n src/downstream_tls.rs
cat -n src/tls_delivery.rs
cat -n src/edge_contract.rs
cat -n src/migration_admin.rs
cat -n src/bin/cwl-pingora-gateway.rs
cat -n src/bin/cwl-pingora-pg-erd-migration.rs
echo '--- TLS-focused test outlines ---'
ast-grep outline tests/downstream_tls_h2_config_red.rs --items all --view expanded
ast-grep outline tests/downstream_tls_h2_wire.rs --items all --view expanded
ast-grep outline tests/downstream_tls_http1_fallback_wire.rs --items all --view expanded
ast-grep outline tests/downstream_tls_lifecycle.rs --items all --view expanded
ast-grep outline tests/downstream_tls_pg_erd_wire.rs --items all --view expanded
echo '--- Pingora dependency lock entries ---'
rg -n -A8 -B2 '^name = "pingora' Cargo.lockLength of output: 92666
The current head matches the requested commit. The terminal checks succeeded. No unresolved review threads remain. I found no blocking issue in the downstream TLS/H2 configuration, ALPN policy, or listener activation paths. ✅ Action performedComments resolved. Approval is disabled; enable |
|
Documentation/implementation mismatch found while reviewing the current stack head (
Suggested minimal repair when this lane is next restacked: move Evidence: |
|
H2 owner handoff from process-health Runtime Isolation PR #102: current #102 exact #102 cannot honestly claim the review-requested real H2 listener fixture because its historical contract still excludes downstream H2. This PR is the versioned downstream TLS/H2 owner and already has exact-current GREEN at |
|
Lifecycle correction: this PR is Draft because its exact parent #73 is itself Draft on historical #70 ancestry. Existing exact GREEN ( |
|
Dependency handoff from #102 current exact Current No source/ref/PR-state mutation is performed here; this is a dependency/evidence handoff only. |
Refs #51. Historical branch-local exact
df70de9cc0c77cfc1dabc51de039ac46af47df08was built as a child of #73 exact625cae4f156366bc39d6782161a4a5f336d58624; #74 remains the separate NUMA evidence lane.This PR owns the versioned downstream TLS/H2 listener increment for generic and pg-erd composition roots. Generic v1 remains cleartext and v2 requires
downstream_tls; pg-erd v1/v2 remain cleartext and v3 adds downstream TLS while retaining the v2 response-lifetime contract. Certificate/key references are materialized only at listener construction, key/cert consistency is checked, ALPN prefersh2, acceptshttp/1.1when offered, fails closed on malformed/unsupported advertised ALPN, and preserves verified no-ALPN HTTP/1.1. Certificate lifecycle/private-key custody, product auth/business logic, Keyverse/Wardnet/EgressWeave authority, deployment authority, h2c and H3/QUIC remain outside this gateway increment.The branch repaired three earlier test-harness defects without weakening production gates: the specialized post-commit observer was separated from unchanged production
read_ms: 5000; HTTP/1 origin fixtures read through\r\n\r\nwith a 64 KiB cap; and that helper uses one five-secondInstantdeadline rather than a resettable per-read timeout.dfb2d9b012e1c3c881796775cdff08067dfd53a1made TRACEABILITY current.Exact
dfb2d9...completed Supply Chain and bounded-origin capacity but CI failed only the 100% owned-production coverage gate because the configured ALPN callback executed in spawned gateway processes whose forced cleanup did not flush LLVM coverage.b0df584b7ec558fc482dd60ec55e22a8a21db5bfadded an in-process TLS handshake that exercises the exact configured callback through Pingora's dereferencedSslAcceptorBuilderwithout widening production visibility or changing TLS/ALPN/runtime behavior. Its CI34546454816then failed earlier atcargo fmt --checkonly; job103100030726emitted one formatter diff wrappingacceptor.accept(stream).expect(...). Historical exactdf70de9...applies exactly that rustfmt output with no semantic change.Historical exact CI
34547405414, Supply Chain34547405418, and PgErd bounded-origin capacity34547405428are terminal GREEN. All prior CodeRabbit inline threads remain resolved. Technical COMMENT5174068417records the historical exact-head review and is not independent approval. Those receipts remain regression/characterization evidence for this historical tree only.#76 is non-force stacked on this exact parent and owns explicit TLS 1.2–1.3/cipher policy; #77 is stacked through #76 and owns concurrent-stream H2 multiplexing. Full H2→H1 parity remains gated by release-qualified supplier #901 Cookie disposition plus a maintainer-integrated/release-qualified zero-length-body repair equivalent to open #936/#976. #1000 remains mutable parser evidence; upstream issues #889 and #447 remain separate roots.
Parent-first lifecycle repair — 2026-09-19
Live PR metadata is correctly Draft. Earlier “Ready for review” wording is superseded by current ancestry state. Direct parent #73 is Draft at historical
625cae4f156366bc39d6782161a4a5f336d58624and is stale against current #70df5d2f05fc5fbdd94bbfb487283bf2a6d73a55bf; #70 itself must first inherit current foundation listener-readiness through ordinary #44/#47 release-line reconciliation and reacquire exact evidence. #73 must then ordinary/non-force adopt current #70 and reacquire its own current-head evidence before #75 can move.The terminal
df70de9...receipts therefore do not authorize Ready state or merge into stale #73. Current order isfoundation promotion prerequisites -> ordinary #44/#47 release-line reconciliation -> #70 adoption + fresh exact evidence/review -> #73 ordinary/non-force reconciliation + fresh exact evidence/review -> #75 ordinary/non-force reconciliation preserving only the valid downstream TLS/H2 listener delta -> fresh exact CI/Supply/capacity/current-range review -> independent governance -> normal integration -> #76/#77 and later H2 descendants reconcile in dependency order.Process-health H2 acceptance dependency — 2026-09-20
#102's process-health source repair is complete at
b9447d58ce90e58c76a2af4acd2ef99c4648b9b9: both adapters useSession::is_body_done()and transport-incomplete health requests no longer receive privileged localProbetreatment. Its only unresolved substantive review thread is now the executable transport requirement for real HTTP/2/livezand/readyztraffic usingHEADERSwithoutEND_STREAMfollowed by DATA. The reviewer explicitly reconfirmed that the source repair is valid and that this real-listener evidence belongs to #75 or its reconciliation successor.#102 was therefore correctly returned to Draft rather than duplicating this listener authority. Its current CI/Supply executions were cancelled by that lifecycle transition and later Draft synchronizations skipped; those outcomes are not source RED. When this lane reaches current ancestry, preserve the existing TLS/H2 listener contract and add the process-health H2 acceptance here, consuming #102's source contract rather than copying its implementation. Only after that owner evidence exists should #102's review-thread disposition and exact-head gates be reconsidered.
No predecessor GREEN transfers across ancestry movement. No force-push, destructive rebase, self-approval, gate weakening, protected merge, immutable release, canary/shadow, rollback, cutover or Nginx/OpenResty removal credit is claimed.