Skip to content

test(server): guard dp-manager REST clients against h2 ALPN (#535, #536)#539

Open
moonming wants to merge 1 commit into
mainfrom
test/issue-535-alpn-http1-guard
Open

test(server): guard dp-manager REST clients against h2 ALPN (#535, #536)#539
moonming wants to merge 1 commit into
mainfrom
test/issue-535-alpn-http1-guard

Conversation

@moonming

@moonming moonming commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

What

Adds a regression guard for #535 — the dp-manager REST clients negotiating ALPN h2 and getting misrouted by dp-manager's single-port cmux. The fix already landed in #536 (.http1_only() on both build_client copies); this PR adds the test #536 didn't include.

Scope: this guards the REST client only — the gRPC/etcd h2 path is intentional and untouched

The DP talks to dp-manager over two protocols on the same :7944 TLS port, handled by two different client libraries:

Purpose Client Protocol h2?
etcd watch (config / model sync) etcd_client (tonic) — crates/aisix-etcd/src/etcd_provider.rs HTTP/2 ✅ intentional — gRPC requires it
heartbeat / telemetry / budget_check (REST) reqwestheartbeat.rs / telemetry.rs HTTP/1.1 ❌ always h1

dp-manager advertises ALPN ["h2","http/1.1"] on the one port to accept both clients (h2 for the tonic gRPC client, http/1.1 for the reqwest REST client); cmux then splits them by protocol. That h2 belongs to etcd-client, not reqwest — reqwest has never carried the http2 feature in this repo's history. #535 was object_store accidentally unifying reqwest/http2 onto the shared client, so the REST client also started advertising h2. This guard (and #536's .http1_only()) only pins the reqwest REST client back to its intended h1; the etcd-client gRPC watch is a separate connection and is not affected.

What it asserts

cp_rest_client_advertises_only_http1_alpn (heartbeat + telemetry): builds the real CP-REST mTLS client via build_client, fires one request at a bare TCP listener, captures the raw TLS ClientHello (sent in the clear, pre-handshake), and asserts the ALPN list advertises http/1.1 and not h2. No TLS server / cert needed, so it can't flake on handshake details — a short/failed capture makes .position() return None → loud panic, never a silent green.

Verified: fails if .http1_only() is removed from build_client (reproduces #535), passes with it.

Test plan

Tests only — no production code change. Guards #535 / #536.

#536 fixed #535 by pinning the dp-manager REST mTLS clients to HTTP/1.1
(`.http1_only()`) but shipped no test — the same blind spot that let the
regression reach production. dp-manager multiplexes kine/etcd gRPC and
the /dp/* REST API on one cmux TLS port, routing by negotiated ALPN; if
these clients ever advertise `h2` again (another dep unifies reqwest's
`http2` feature back on, or `.http1_only()` is dropped), every
budget_check / heartbeat / telemetry call fails at the transport layer.

Add `cp_rest_client_advertises_only_http1_alpn` for both build_client
copies: capture the raw TLS ClientHello and assert ALPN advertises
http/1.1 and not h2. Verified to fail if `.http1_only()` is removed.
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: 6e04a490-a277-4665-a8a2-6dbcdda37799

📥 Commits

Reviewing files that changed from the base of the PR and between af43b3c and bdd267c.

📒 Files selected for processing (2)
  • crates/aisix-server/src/heartbeat.rs
  • crates/aisix-server/src/telemetry.rs

📝 Walkthrough

Walkthrough

This PR adds two regression tests to validate HTTP/1.1-only ALPN advertisement behavior: one for the heartbeat/REST client in heartbeat.rs and one for the telemetry mTLS client in telemetry.rs. Both tests capture raw TLS ClientHello bytes and inspect ALPN fields to ensure http/1.1 is advertised without the h2 protocol, preventing cmux misrouting.

Changes

ALPN Validation Across Clients

Layer / File(s) Summary
ALPN behavior regression tests for heartbeat and telemetry clients
crates/aisix-server/src/heartbeat.rs, crates/aisix-server/src/telemetry.rs
Two async tests bind local TCP listeners and trigger HTTPS requests from heartbeat and telemetry clients, capturing raw TLS ClientHello bytes to inspect ALPN fields and verify http/1.1 is advertised without h2, ensuring dp-manager cmux routes these requests to HTTP/1.1 listeners.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above.

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

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.

1 participant