Skip to content

feat: add versioned downstream TLS/H2 edge listener - #75

Draft
seonghobae wants to merge 64 commits into
feat/runtime-service-threads-v1from
test/downstream-tls-h2-config-red-v1
Draft

seonghobae wants to merge 64 commits into
feat/runtime-service-threads-v1from
test/downstream-tls-h2-config-red-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Refs #51. Historical branch-local exact df70de9cc0c77cfc1dabc51de039ac46af47df08 was built as a child of #73 exact 625cae4f156366bc39d6782161a4a5f336d58624; #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 prefers h2, accepts http/1.1 when 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\n with a 64 KiB cap; and that helper uses one five-second Instant deadline rather than a resettable per-read timeout. dfb2d9b012e1c3c881796775cdff08067dfd53a1 made 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. b0df584b7ec558fc482dd60ec55e22a8a21db5bf added an in-process TLS handshake that exercises the exact configured callback through Pingora's dereferenced SslAcceptorBuilder without widening production visibility or changing TLS/ALPN/runtime behavior. Its CI 34546454816 then failed earlier at cargo fmt --check only; job 103100030726 emitted one formatter diff wrapping acceptor.accept(stream).expect(...). Historical exact df70de9... applies exactly that rustfmt output with no semantic change.

Historical exact CI 34547405414, Supply Chain 34547405418, and PgErd bounded-origin capacity 34547405428 are terminal GREEN. All prior CodeRabbit inline threads remain resolved. Technical COMMENT 5174068417 records 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 625cae4f156366bc39d6782161a4a5f336d58624 and is stale against current #70 df5d2f05fc5fbdd94bbfb487283bf2a6d73a55bf; #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 is foundation 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 use Session::is_body_done() and transport-incomplete health requests no longer receive privileged local Probe treatment. Its only unresolved substantive review thread is now the executable transport requirement for real HTTP/2 /livez and /readyz traffic using HEADERS without END_STREAM followed 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.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

📝 Walkthrough

Walkthrough

공유 게이트웨이와 PG-ERD 마이그레이션에 다운스트림 TLS/H2 설정 계약을 추가했습니다. 인증서와 개인 키를 Pingora TLS 설정으로 변환합니다. 두 실행 파일은 TLS 설정이 있으면 TLS 리스너를 사용하고, 없으면 TCP 리스너를 사용합니다. 실환경 테스트는 H2와 HTTP/1.1 폴백을 검증합니다.

Changes

다운스트림 TLS/H2 지원

Layer / File(s) Summary
TLS 구성 계약과 버전 검증
src/downstream_tls.rs, src/edge_contract.rs, src/migration_admin.rs, src/lib.rs, tests/config_contract.rs, tests/downstream_tls_h2_config_red.rs
공유 게이트웨이는 버전 2, PG-ERD는 버전 3의 downstream_tls 설정을 검증합니다. 인증서와 개인 키 경로는 비어 있지 않은 절대 경로여야 합니다.
Pingora TLS 설정 생성
src/tls_delivery.rs, src/downstream_tls.rs
인증서와 개인 키 경로를 검증하고 Pingora TLS 설정을 생성합니다. 설정 오류, 비UTF-8 경로, TLS materialization 오류를 별도 오류로 반환합니다.
조건부 TLS 리스너 바인딩
src/bin/cwl-pingora-gateway.rs, src/bin/cwl-pingora-pg-erd-migration.rs
두 바이너리는 downstream_tls가 있으면 add_tls_with_settings를 호출합니다. 설정이 없으면 add_tcp를 호출합니다.
공유 게이트웨이 실환경 검증
tests/downstream_tls_h2_wire.rs, tests/downstream_tls_http1_fallback_wire.rs
공유 게이트웨이에서 검증된 TLS 연결, H2 프록시, HTTP/1.1 ALPN 폴백과 실제 업스트림 응답을 검증합니다.
PG-ERD 경로와 수명 주기 검증
tests/downstream_tls_pg_erd_wire.rs, tests/downstream_tls_lifecycle.rs, tests/gateway_proxy.rs, tests/pg_erd_admin_config_contract.rs
PG-ERD v3의 HTTP/1.1 프록시, 정상 종료, 인증서 누락 시 실패, 기존 버전 검증을 검증합니다.

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 응답
Loading

Merge Risk: 🟡 Moderate · up to 8de1f

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 버전이 적용된 다운스트림 TLS/H2 엣지 리스너 추가라는 변경의 핵심을 정확하고 간결하게 설명합니다.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/downstream-tls-h2-config-red-v1

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae changed the title test: encode versioned downstream TLS/H2 config RED feat: add versioned downstream TLS/H2 edge listener Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 10, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added a commit that referenced this pull request Sep 11, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seonghobae
seonghobae marked this pull request as ready for review September 11, 2026 01:26
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

Copy link
Copy Markdown
Contributor Author

@coderabbitai approve

Current exact df70de9cc0c77cfc1dabc51de039ac46af47df08 is terminal GREEN across CI/Supply Chain/capacity and all returned inline review threads are resolved. Please review/approve the exact current head only; do not transfer any predecessor review.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown
🧩 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.lock

Length of output: 92666


@seonghobae I reviewed pull request #75 at exact commit df70de9cc0c77cfc1dabc51de039ac46af47df08.

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 performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Documentation/implementation mismatch found while reviewing the current stack head (docs/pingora-0-9-traceability-v1@e23ed24), owned by this lane via commit 8e78d1c (docs: make architecture current for downstream TLS H2):

ARCHITECTURE.md line 76 lists forwarding_policy among the "transport-neutral anti-corruption boundaries around Pingora delivery semantics" together with edge_contract, downstream_tls, edge_routing, http_policy, migration_plan, migration_admin, runtime_isolation.

src/forwarding_policy.rs is not Pingora-free: it imports pingora::prelude::{Error, ErrorType, RequestHeader} and pingora::protocols::l4::socket::SocketAddr, and its public API returns pingora::Result (ForwardingContext::from_downstream_transport, apply). That coupling has existed since 192f697 on the lower stack; only the sentence is wrong. AGENTS.md only mandates a Pingora-free edge_contract/downstream_tls/edge_routing/http_policy, so this is a documentation defect, not a source regression.

Suggested minimal repair when this lane is next restacked: move forwarding_policy out of the transport-neutral list and describe it as a delivery-adjacent adapter that consumes accepted Pingora session/request types (or, if the transport-neutral claim is intended, split the Pingora-typed apply/from_downstream_transport into pingora_delivery — a larger change that needs its own RED). No source change is proposed here; the writer of this lane keeps ownership.

Evidence: grep -n pingora src/forwarding_policy.rs → lines 9, 10, 68, 109, 145 at e23ed24.

Copy link
Copy Markdown
Contributor Author

H2 owner handoff from process-health Runtime Isolation PR #102: current #102 exact 1faa68a6cc25dedbe8c11140c4e62509855fc433 repairs the header-only health-bypass classifier by passing Pingora Session::is_body_done() into the shared policy. A GET/HEAD /livez or /readyz is privileged only when header framing is body-free and the downstream body is already complete; body_done=false enters ordinary admission before 413. This specifically closes the semantic gap where H2 HEADERS without END_STREAM can be followed by DATA despite no Content-Length.

#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 df70de9cc0c77cfc1dabc51de039ac46af47df08. When the #102 process-health delta is ordinarily reconciled into this H2 line, please add executable acceptance for both /livez and /readyz: HEADERS without END_STREAM followed by DATA must not receive privileged 200, while a completed HEADERS+END_STREAM GET/HEAD probe remains observable under saturation. Preserve this PR's TLS/H2 authority; do not copy protocol parsing into #102. No #75 source/ref/state was changed by this handoff.

Copy link
Copy Markdown
Contributor Author

Lifecycle correction: this PR is Draft because its exact parent #73 is itself Draft on historical #70 ancestry. Existing exact GREEN (df70de9cc0c77cfc1dabc51de039ac46af47df08) remains characterization of the TLS/H2 listener increment, not current-parent promotion evidence. Re-promote only after #70 current exact is terminal/current-reviewed, #73 ordinary/non-force adopts current #70 and reacquires its gates/review, then #75 reconciles current #73 and reacquires applicable exact evidence. Downstream #76–#93 plus #96/#98 have likewise been returned to Draft so no stale-parent child can advance first. The body’s older “Ready” wording is superseded by live Draft state and canonical #58; preserve the valid child delta, do not source-copy parent repairs or transfer predecessor receipts.

Copy link
Copy Markdown
Contributor Author

Dependency handoff from #102 current exact c8fa48aeadf49ef657990e5bd70668cbf9057870: the shared process-health classifier now consumes Session::is_body_done() so header-only body inference cannot grant /livez or /readyz privilege. The remaining executable review evidence belongs here because #75 owns the real downstream TLS/H2 listener.

Current tests/downstream_tls_h2_wire.rs proves ordinary H2 HEADERS with END_STREAM (flags 0x5) through the generic listener, but it does not yet characterize a process-health stream sent as HEADERS without END_STREAM followed by DATA. When this lane next advances, please preserve its existing TLS/ALPN/source contracts and add owner-local acceptance that /livez and /readyz do not return privileged 200 for HEADERS(no END_STREAM) -> DATA, while completed payload-free GET/HEAD probes still return 200 (including under the applicable admission condition). That evidence can close #102's remaining CodeRabbit transport-level thread without moving H2 authority into the process-health child.

No source/ref/PR-state mutation is performed here; this is a dependency/evidence handoff only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant