fix: fail closed on uncharacterized HTTP/1 Upgrade - #29
seonghobae merged 48 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
seonghobae
added
bug
ChatGPT Codex Connector
|
Parent-first succession handoff after #25/#27 repair. #27 is now current exact Fresh compare from this PR's historical #27 base |
|
Parent exact moved once more after a valid independent review finding. CodeRabbit correctly found that #27 documented mapped/native authority equivalence without a direct IPv4-mapped upstream regression. #27 was returned to Draft, repaired without widening production policy, and is now Ready at |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-exact owner technical sweep after ordinary/non-force parent repair. The live base is terminal-clean #27 d351f1e5cffdadf31e696d637e9a76b382351e5c; the effective child range is exactly nine paths and excludes #61-owned baseline/TRACEABILITY plus the repaired #25 post-commit-reset fixture. The adoption retained historical #29 as first parent while using current #27 truth and only the seven valid protocol-transition source/test blobs; CHANGELOG/TEST_STRATEGY were then re-synthesized from current-parent text, so the header-commit RST repair is preserved rather than replayed from stale child history. Both composition roots still reject HTTP/1 Upgrade before admission/route/upstream work, real-listener acceptance requires bounded /readyz HTTP/1.1 200, exact 501 and a finite 500 ms no-origin-contact observation, and the docs correctly keep WebSocket/H2 Extended CONNECT/H3 outside this version. No unresolved review thread or additional writer-safe defect found in this exact range. COMMENT only; this is not self-approval, and fresh exact-head CI/Supply Chain are required.
|
@coderabbitai review Please review current exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="523d825a8678976a3e678cd244918df4da9f691b"
head="3158895c7463be30c5de86497712315998e77efd"
historical="604d8d6c3eec6cd4536575e11faf290bf96e479e"
printf '%s\n' '--- exact objects, ancestry, and merge shape ---'
for rev in "$base" "$head" "$historical"; do
printf '%s ' "$rev"
git cat-file -t "$rev"
done
printf 'merge-base(base, head): '; git merge-base "$base" "$head"
printf 'parents(head): '; git show -s --format='%P' "$head"
printf 'first-parent(head): '; git show -s --format='%P' "$head" | awk '{print $1}'
printf 'second-parent(head): '; git show -s --format='%P' "$head" | awk '{print $2}'
git merge-base --is-ancestor "$base" "$head" && echo 'base_is_ancestor=yes'
git diff --check "${base}..${head}"
git diff --stat "${base}..${head}"
git diff --name-status "${base}..${head}"
git log --format='%H %P %s' "${base}..${head}"
printf '%s\n' '--- exact effective-path comparison ---'
expected=$'CHANGELOG.md\nTEST_STRATEGY.md\nsrc/gateway_proxy.rs\nsrc/lib.rs\nsrc/migration_proxy.rs\nsrc/pingora_delivery.rs\nsrc/protocol_transition_policy.rs\ntests/protocol_transition_policy.rs\ntests/protocol_transition_traffic.rs'
actual="$(git diff --name-only "${base}..${head}" | sort)"
printf '%s\n' 'expected:'
printf '%s\n' "$expected" | sort
printf '%s\n' 'actual:'
printf '%s\n' "$actual"
if diff -u <(printf '%s\n' "$expected" | sort) <(printf '%s\n' "$actual"); then
echo 'effective_path_set_matches=yes'
else
echo 'effective_path_set_matches=no'
fi
printf '%s\n' '--- parent-owned paths must be unchanged ---'
for path in \
API_CONFIG_CONTRACT.md \
src/edge_contract.rs \
tests/network_authority_self_loop_contract.rs \
tests/pg_erd_post_commit_reset_traffic.rs \
tests/production_path.rs \
tests/tls_sni_hostname_contract.rs \
tests/upstream_unicast_authority_contract.rs
do
if git diff --quiet "$base" "$head" -- "$path"; then
printf 'unchanged %s\n' "$path"
else
printf 'CHANGED %s\n' "$path"
git diff --unified=20 "$base" "$head" -- "$path"
fi
done
printf '%s\n' '--- changed Rust outlines ---'
git diff --name-only "${base}..${head}" -- '*.rs' | while IFS= read -r path; do
printf '\n### %s\n' "$path"
ast-grep outline "$path" --items all || true
done
printf '%s\n' '--- focused full diff ---'
git diff --find-renames --find-copies --unified=50 "${base}..${head}" -- \
CHANGELOG.md TEST_STRATEGY.md \
src/gateway_proxy.rs src/lib.rs src/migration_proxy.rs \
src/pingora_delivery.rs src/protocol_transition_policy.rs \
tests/protocol_transition_policy.rs tests/protocol_transition_traffic.rs
printf '%s\n' '--- current production policy, mapping, and request-filter ordering ---'
for path in \
src/protocol_transition_policy.rs \
src/pingora_delivery.rs \
src/gateway_proxy.rs \
src/migration_proxy.rs \
src/lib.rs
do
printf '\n### %s\n' "$path"
nl -ba "$path" | sed -n '1,400p'
done
printf '%s\n' '--- current transition tests ---'
for path in tests/protocol_transition_policy.rs tests/protocol_transition_traffic.rs; do
printf '\n### %s\n' "$path"
nl -ba "$path" | sed -n '1,430p'
done
printf '%s\n' '--- all transition-related current call sites ---'
rg -n -C 7 --glob '*.rs' \
'(reject_uncharacterized_http1_protocol_transition\s*\(|requests_http1_protocol_transition\s*\(|ErrorType::HTTPStatus\(501\)|async fn request_filter|async fn upstream_peer|Upgrade|Connection|readyz|livez|assert_origin_untouched|wait_until_ready|response_headers|http1_status_code)' \
src tests
printf '%s\n' '--- current documentation claims ---'
for path in CHANGELOG.md TEST_STRATEGY.md; do
printf '\n### %s\n' "$path"
rg -n -i -C 8 \
'(upgrade|websocket|protocol.transition|501|origin|traffic|500 ?ms|observation|callback|ordering|readyz|http/1\.1|64.?ki?b|sni|unicast|post.commit|reset)' \
"$path" || true
doneLength of output: 204368 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="523d825a8678976a3e678cd244918df4da9f691b"
head="3158895c7463be30c5de86497712315998e77efd"
historical="604d8d6c3eec6cd4536575e11faf290bf96e479e"
printf '%s\n' '--- ancestry ---'
printf 'merge-base(base,head)='; git merge-base "$base" "$head"
printf 'parents(head)='; git show -s --format='%P' "$head"
printf 'first-parent-is-historical='
test "$(git show -s --format='%P' "$head" | awk '{print $1}')" = "$historical" && echo yes || echo no
printf 'second-parent-is-base='
test "$(git show -s --format='%P' "$head" | awk '{print $2}')" = "$base" && echo yes || echo no
printf 'base-ancestor-of-head='
git merge-base --is-ancestor "$base" "$head" && echo yes || echo no
printf '%s\n' '--- effective child paths ---'
git diff --name-only "$base" "$head" | sort
printf '%s\n' '--- effective path set result ---'
expected=$'CHANGELOG.md\nTEST_STRATEGY.md\nsrc/gateway_proxy.rs\nsrc/lib.rs\nsrc/migration_proxy.rs\nsrc/pingora_delivery.rs\nsrc/protocol_transition_policy.rs\ntests/protocol_transition_policy.rs\ntests/protocol_transition_traffic.rs'
if diff -u <(printf '%s\n' "$expected" | sort) <(git diff --name-only "$base" "$head" | sort); then
echo MATCH
else
echo MISMATCH
fi
printf '%s\n' '--- parent-owned equality ---'
for path in API_CONFIG_CONTRACT.md src/edge_contract.rs tests/network_authority_self_loop_contract.rs tests/pg_erd_post_commit_reset_traffic.rs tests/production_path.rs tests/tls_sni_hostname_contract.rs tests/upstream_unicast_authority_contract.rs; do
git diff --quiet "$base" "$head" -- "$path" && printf 'UNCHANGED %s\n' "$path" || printf 'CHANGED %s\n' "$path"
done
printf '%s\n' '--- transition readiness implementation ---'
nl -ba tests/protocol_transition_traffic.rs | sed -n '90,165p'
printf '%s\n' '--- exact transition documentation ---'
nl -ba CHANGELOG.md | sed -n '12,20p'
nl -ba TEST_STRATEGY.md | sed -n '11,16p'
printf '%s\n' '--- diff check ---'
git diff --check "$base" "$head"
echo CLEANLength of output: 12568
The reconciliation otherwise matches the requested scope.
You are interacting with an AI system. |
|
@coderabbitai review Please re-review current exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="523d825a8678976a3e678cd244918df4da9f691b"
reconciled="3158895c7463be30c5de86497712315998e77efd"
head="976a0c5c90f9a6a106867e0af732bc63b47acecd"
printf '%s\n' '--- object and ancestry integrity ---'
for rev in "$base" "$reconciled" "$head"; do
printf '%s ' "$rev"
git cat-file -t "$rev"
done
printf 'merge-base(base,head): '; git merge-base "$base" "$head"
printf 'merge-base(reconciled,head): '; git merge-base "$reconciled" "$head"
printf 'reconciled ancestor of head: '
git merge-base --is-ancestor "$reconciled" "$head" && echo yes || echo no
printf '%s\n' '--- commits after reconciled head ---'
git log --format='%H %P %s' "${reconciled}..${head}"
printf '%s\n' '--- range hygiene ---'
git diff --check "${base}..${head}"
git diff --stat "${base}..${head}"
git diff --name-status "${base}..${head}"
printf '%s\n' '--- exact declared child-path comparison ---'
expected=$'CHANGELOG.md\nTEST_STRATEGY.md\nsrc/gateway_proxy.rs\nsrc/lib.rs\nsrc/migration_proxy.rs\nsrc/pingora_delivery.rs\nsrc/protocol_transition_policy.rs\ntests/protocol_transition_policy.rs\ntests/protocol_transition_traffic.rs'
actual="$(git diff --name-only "${base}..${head}" | sort)"
diff -u <(printf '%s\n' "$expected" | sort) <(printf '%s\n' "$actual") && echo 'effective_path_set_matches=yes'
printf '%s\n' '--- repair delta only ---'
git diff --check "${reconciled}..${head}"
git diff --unified=70 "${reconciled}..${head}" -- tests/protocol_transition_traffic.rs
printf '%s\n' '--- traffic fixture structure ---'
ast-grep outline tests/protocol_transition_traffic.rs --items all
nl -ba tests/protocol_transition_traffic.rs | sed -n '1,360p'
printf '%s\n' '--- source policy, mapping, and ordering ---'
for file in \
src/protocol_transition_policy.rs \
src/pingora_delivery.rs \
src/gateway_proxy.rs \
src/migration_proxy.rs \
tests/protocol_transition_policy.rs
do
printf '\n### %s\n' "$file"
rg -n -C 9 \
'(reject_uncharacterized_http1_protocol_transition\s*\(|requests_http1_protocol_transition\s*\(|ErrorType::HTTPStatus\(501\)|async fn request_filter|async fn upstream_peer|readyz|livez|Upgrade|Connection)' \
"$file" || true
done
printf '%s\n' '--- current in-scope documentation claims ---'
for file in CHANGELOG.md TEST_STRATEGY.md; do
printf '\n### %s\n' "$file"
rg -n -i -C 7 \
'(upgrade|protocol.transition|501|origin|500 ?ms|observation|callback|ordering|readyz|http/1\.1|64.?ki?b)' \
"$file" || true
doneLength of output: 79439
I found one remaining readiness-oracle issue.
The remaining reviewed behavior matches the requested scope:
You are interacting with an AI system. |
|
@coderabbitai review Please re-review current exact |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="523d825a8678976a3e678cd244918df4da9f691b"
previous="976a0c5c90f9a6a106867e0af732bc63b47acecd"
head="ac83bd788ecda687e9f5717f8d66fce67b177fd0"
printf '%s\n' '--- objects and ancestry ---'
for rev in "$base" "$previous" "$head"; do
printf '%s ' "$rev"
git cat-file -t "$rev"
done
printf 'merge-base(base,head): '; git merge-base "$base" "$head"
printf 'merge-base(previous,head): '; git merge-base "$previous" "$head"
printf 'previous-is-ancestor: '
git merge-base --is-ancestor "$previous" "$head" && echo yes || echo no
printf '%s\n' '--- commits after previous ---'
git log --format='%H %P %s' "${previous}..${head}"
printf '%s\n' '--- range hygiene and effective paths ---'
git diff --check "${base}..${head}"
git diff --stat "${base}..${head}"
git diff --name-status "${base}..${head}"
expected=$'CHANGELOG.md\nTEST_STRATEGY.md\nsrc/gateway_proxy.rs\nsrc/lib.rs\nsrc/migration_proxy.rs\nsrc/pingora_delivery.rs\nsrc/protocol_transition_policy.rs\ntests/protocol_transition_policy.rs\ntests/protocol_transition_traffic.rs'
if diff -u <(printf '%s\n' "$expected" | sort) <(git diff --name-only "${base}..${head}" | sort); then
echo 'effective_path_set_matches=yes'
else
echo 'effective_path_set_matches=no'
fi
printf '%s\n' '--- repair delta only ---'
git diff --check "${previous}..${head}"
git diff --unified=100 "${previous}..${head}" -- tests/protocol_transition_traffic.rs
printf '%s\n' '--- current traffic fixture map and implementation ---'
ast-grep outline tests/protocol_transition_traffic.rs --items all
nl -ba tests/protocol_transition_traffic.rs | sed -n '1,380p'
printf '%s\n' '--- relevant production ordering and mapping ---'
for file in src/protocol_transition_policy.rs src/pingora_delivery.rs src/gateway_proxy.rs src/migration_proxy.rs; do
printf '\n### %s\n' "$file"
rg -n -C 10 \
'(reject_uncharacterized_http1_protocol_transition\s*\(|requests_http1_protocol_transition\s*\(|ErrorType::HTTPStatus\(501\)|async fn request_filter|async fn upstream_peer|readyz|livez)' \
"$file" || true
done
printf '%s\n' '--- policy tests and traffic-test deadline references ---'
nl -ba tests/protocol_transition_policy.rs | sed -n '1,280p'
rg -n -C 7 --glob '*.rs' \
'(STARTUP_DEADLINE|set_(read|write)_timeout|read\s*\(|write_all|response_headers|http1_status_code|assert_origin_untouched|WouldBlock|Duration::from_millis)' \
tests src
printf '%s\n' '--- relevant documentation claims ---'
for file in CHANGELOG.md TEST_STRATEGY.md; do
printf '\n### %s\n' "$file"
rg -n -i -C 6 \
'(upgrade|protocol.transition|501|origin|500 ?ms|observation|callback|ordering|readyz|http/1\.1|64.?ki?b)' \
"$file" || true
doneLength of output: 251425
In
Therefore, a slow-drip The effective range contains the declared nine paths and no additional paths. The range is also whitespace-clean. The full range continues to match the transition boundary:
You are interacting with an AI system. |
aee4815
into
fix/tls-sni-hostname-admission-v1
1 participant
Valid finding
The current product contract keeps WebSocket and other protocol-transition semantics outside generic v1 and the bounded pg-erd candidate. Both Pingora composition roots therefore need an explicit edge-owned decision before any HTTP/1 Upgrade can reach admission, route selection, or an origin. This remains an Ingress / HTTP Policy boundary only; it does not implement WebSocket, CONNECT tunneling, product auth/business logic, Keyverse identity, Wardnet/EgressWeave policy, or retry authority.
The transport-neutral policy treats either an
Upgradefield or a case-insensitive comma-delimitedupgradetoken in anyConnectionfield value as an uncharacterized transition attempt. Lookalike tokens such asx-upgraderemain ordinary values. The Pingora delivery adapter maps that policy to HTTP 501.Parent-first reconciliation
#107 exact
523d825a8678976a3e678cd244918df4da9f691bcompleted current-head CI/Supply and independent review, then normally merged asa2dbb0b52547fd89736252b8a7bd2dcdb2d3e1beinto its non-default parent.Historical #29 head
604d8d6c3eec6cd4536575e11faf290bf96e479ehad diverged from current parent523d825...at merge-based351f1e5...(ahead 45 / behind 46). Reconciliation commit3158895c7463be30c5de86497712315998e77efdis an ordinary two-parent/non-force merge: first parent is the complete historical child head604d8d6...; second parent is current parent523d825.... No force-push, destructive rebase, selective parent copy, child-only fake merge, self-approval, gate bypass, or predecessor-GREEN transfer was used.Fresh compare after reconciliation has merge-base
523d825..., behind 0, and exactly the nine intended child paths:CHANGELOG.mdTEST_STRATEGY.mdsrc/gateway_proxy.rssrc/lib.rssrc/migration_proxy.rssrc/pingora_delivery.rssrc/protocol_transition_policy.rstests/protocol_transition_policy.rstests/protocol_transition_traffic.rsParent-only Admin Config/SNI/network-authority/post-commit evidence was adopted. The two genuine documentation overlaps were semantically synthesized so neither the current parent evidence nor the child HTTP/1 transition contract was discarded.
Current review RED → causal repairs
Current-range CodeRabbit review on reconciled exact
3158895...confirmed the merge shape, exact nine-path authority, parent evidence preservation, transition-guard ordering, 501 mapping and token matching, then identified a valid bounded-oracle defect intests/protocol_transition_traffic.rs:wait_until_readydiscarded socket timeout-configuration failures. Repair976a0c5c90f9a6a106867e0af732bc63b47acecdmade timeout setup fail closed before readiness reads.The current-range re-review of
976a0c5...found one further valid boundedness defect: a fixed 250 ms read timeout was renewed after every successful partial read, so slow-drip response progress could extend the nominal ten-second startup budget indefinitely. The PR returned to Draft again.Current exact
ac83bd788ecda687e9f5717f8d66fce67b177fd0is the minimal causal repair.wait_until_readynow uses the original absolutedeadlineinside the response-read loop. Before every read it stops the attempt if that deadline has elapsed and sets the socket read timeout tomin(deadline - now, 250 ms); a timeout-configuration failure ends that attempt. Thus partial progress cannot renew the startup budget. The preceding write-timeout setup must also succeed before the HTTP readiness exchange proceeds. Production policy, HTTP 501 mapping, route/admission ordering, origin-contact acceptance and the 500 ms observation window remain unchanged.Fresh compare from
976a0c5...toac83bd7...is one commit and one file (tests/protocol_transition_traffic.rs), +15/-16. Current-head CodeRabbit re-review reply5704986753reports that this repair addresses the prior finding and found no remaining actionable finding in the requested scope; it also reconfirmed ordinary ancestry and the exact nine-path effective range. Reviews/evidence on predecessor heads remain predecessor evidence only.Preserved transition contract
Both request filters call the shared transition rejection after gateway-local health handling but before admission and route/upstream work. Real-listener acceptance requires bounded
/readyz, exact HTTP/1.1 501 for WebSocket-shaped Upgrade attempts, no relevant origin connection in the fixed observation window, and preserved/readyz. The observation window is bounded evidence, not a claim that arbitrarily delayed origin contact is impossible. WebSocket enablement, HTTP/2 Extended CONNECT, and HTTP/3/QUIC remain separate versioned work.docs/product-technical-gap-baseline.md, repository-wide TRACEABILITY, parent Admin Config sources/fixtures, product auth/business logic, Keyverse identity and Wardnet/EgressWeave authority remain outside this lane.Current exact gate
Current exact review is clean, but Ready is not merge authorization. CI
35153593189and Supply Chain35153592634must still reach terminal same-SHA success. Before normal merge, the unchanged exact head must pass Rust formatting,cargo test --all-targets --locked, strict Clippy, warning-denied rustdoc, 100% owned-production coverage, real listener/traffic and applicable load acceptance, dual-profile OCI, Supply Chain/source binding, security/required workflows, and current-head independent review. Any deterministic RED returns the PR to repair/Draft before a causal fix and fresh evidence.#31 and #33 remain dependent Drafts and must consume this repaired exact by ordinary/non-force ancestry only after normal #29 integration. No protected merge, immutable release, parity/shadow/canary, rollback, cutover, or legacy-removal credit is claimed.