Align config architecture with nginx-style vhosts#62
Conversation
Move site-owned listen, TLS, upstreams, and routes into virtual host configs while keeping the main config focused on runtime/global defaults. Fix redundant Host forwarding for h2/h3 authority pseudo-header compatibility and extend validation, compile, routing, and check coverage.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 Walkthrough📋 Walkthrough该PR重构了虚拟主机和监听器配置模型，引入了按虚拟主机定义监听器的能力，支持虚拟主机局域上游编译和作用域限定，并改进了代理请求中主机头的处理逻辑以正确处理HTTP/2和HTTP/3的权限伪头。 📊 Changes
🔄 Sequence Diagram(s)sequenceDiagram
actor Client
participant VhostConfig as Vhost Config<br/>(listen, upstreams)
participant Listener as Vhost Listener<br/>Compiler
participant Router as Route<br/>Compiler
participant Upstream as Scoped<br/>Upstream Map
participant Proxy as Proxy<br/>Handler
Client->>VhostConfig: provide listen entries<br/>and local upstreams
VhostConfig->>Listener: extract listen directives
Listener->>Listener: parse and deduplicate<br/>by SocketAddr
Listener-->>Listener: generate vhost-listen:*<br/>listener IDs
VhostConfig->>Upstream: extract local upstreams
Upstream->>Upstream: compile with scope<br/>prefix (e.g., servers[0]::)
Upstream-->>Router: return scoped names map
VhostConfig->>Router: compile routes with<br/>local upstream mapping
Router->>Router: resolve upstream via<br/>local mapping first
Router-->>Proxy: emit routes targeting<br/>scoped upstreams
Client->>Proxy: send request
Proxy->>Upstream: lookup upstream by<br/>scoped name
Upstream-->>Proxy: return compiled upstream
Proxy->>Proxy: sanitize Host header<br/>for H2/H3 authority
Proxy-->>Client: return proxied response
🎯 Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 📌 Possibly related PRs
🐰 Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the configuration model toward an nginx-style “vhost owns site specifics” layout (listen/TLS/upstreams/routes), while also addressing an upstream HTTP/2/HTTP/3 compatibility issue by removing redundant Host forwarding in situations where :authority is used.
Changes:
- Fix upstream request/header construction to avoid sending redundant
Hostfor h2/h3-style requests (especially withUpstreamProtocol::Autoover HTTPS), and update tests accordingly. - Introduce
servers[].listenparsing/validation plus compilation of deduplicated listeners from vhostlistenentries; add vhost-local upstream scoping and route compilation support. - Update validation, “check” reporting, and sample configs/docs to reflect the new vhost-centric config architecture.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rginx-http/src/transition/tests.rs | Updates reload boundary stability expectations for new vhost-scoped fields. |
| crates/rginx-http/src/transition.rs | Expands reloadable/restart-required field lists to include vhost listen + vhost upstream TLS fields. |
| crates/rginx-http/src/proxy/tests/request_headers.rs | Adds tests covering redundant Host removal behavior across protocols. |
| crates/rginx-http/src/proxy/tests/mod.rs | Exposes new header helper to proxy test modules. |
| crates/rginx-http/src/proxy/tests/grpc.rs | Adjusts gRPC probe expectations to match new Host handling. |
| crates/rginx-http/src/proxy/request_body/prepare.rs | Applies redundant Host removal after request header sanitization. |
| crates/rginx-http/src/proxy/health/request.rs | Ensures health-check requests also apply redundant Host removal (and forces HTTP/2 for gRPC checks). |
| crates/rginx-http/src/proxy/common.rs | Adds remove_redundant_host_header_for_authority_pseudo_header() and helper gating logic. |
| crates/rginx-http/src/proxy/clients/http_client.rs | Enables Hyper host synthesis (set_host(true)) to keep HTTP/1 correctness when Host is intentionally absent. |
| crates/rginx-config/src/validate/vhost.rs | Adds vhost listen validation and vhost-local upstream visibility for vhost routes. |
| crates/rginx-config/src/validate/tests/vhosts.rs | Extends validation tests for vhost listen model and vhost-local upstream scoping. |
| crates/rginx-config/src/validate/tests.rs | Updates sample vhost helper to include new vhost fields. |
| crates/rginx-config/src/validate/server/listener/listeners.rs | Adds validation path for servers[].listen listener model and binding consistency checks. |
| crates/rginx-config/src/validate/server.rs | Exposes validate_http3_config() for reuse by vhost validation. |
| crates/rginx-config/src/validate.rs | Updates top-level validation rules for “locations vs vhosts required” and listener model selection. |
| crates/rginx-config/src/model/vhost.rs | Extends vhost schema with listen, vhost-local upstreams, and http3; supports single-string-or-list for list fields. |
| crates/rginx-config/src/model/listener.rs | Adds Default derive for Http3Config to simplify vhost-listen compilation. |
| crates/rginx-config/src/model.rs | Makes locations defaultable (optional in config files). |
| crates/rginx-config/src/listen.rs | Implements nginx-style parsing for vhost listen strings and options. |
| crates/rginx-config/src/lib.rs | Wires in the new listen parser module. |
| crates/rginx-config/src/compile/vhost.rs | Compiles vhosts with vhost-local upstreams and returns both vhost + local upstream map. |
| crates/rginx-config/src/compile/upstream.rs | Adds support for compiling “scoped” upstream names (e.g., servers[0]::name). |
| crates/rginx-config/src/compile/tests/vhosts.rs | Adds compile-time tests for vhost listener generation and vhost-local upstream precedence. |
| crates/rginx-config/src/compile/tests/route.rs | Updates route compilation tests for new vhost fields. |
| crates/rginx-config/src/compile/tests.rs | Registers new vhost compile test module. |
| crates/rginx-config/src/compile/server/listener.rs | Adds compilation of deduplicated listeners derived from vhost listen entries. |
| crates/rginx-config/src/compile/server.rs | Exposes compile_vhost_listeners() as part of server compilation API. |
| crates/rginx-config/src/compile/route.rs | Adds local-upstream-aware route compilation for vhost scope resolution. |
| crates/rginx-config/src/compile/mod.rs | Selects vhost-listen listener model when any vhost has listen, and merges vhost-local upstreams into snapshot. |
| crates/rginx-app/tests/upstream_mtls.rs | Extends mTLS integration test to assert Host presence/shape for HTTP/1 requests. |
| crates/rginx-app/tests/upstream_http2.rs | Extends HTTP/2 integration tests to assert Host is absent and to simulate upstream rejection when present. |
| crates/rginx-app/tests/check/tls.rs | Updates check output expectations for new reload boundary fields (vhost upstream TLS fields). |
| crates/rginx-app/tests/check/summary.rs | Updates restart boundary fields output expectations to include servers[].listen. |
| crates/rginx-app/tests/check/basic.rs | Updates check output expectations for vhost listener model and route counts. |
| crates/rginx-app/src/check/summary.rs | Detects and reports new listener_model=vhost based on generated listener IDs. |
| configs/rginx.ron | Updates default config to keep only global defaults and defer site config to conf.d/*.ron. |
| configs/conf.d/default.ron | Updates example vhost to own listen and default routes (/, /healthz). |
| README.md | Documents the new nginx-aligned config layout in the README. |
💡 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rginx-app/src/check/summary.rs (1)
90-99:⚠️ Potential issue | 🟡 Minor分类仅依据首个 listener，混合部署下结果取决于顺序
当配置中既存在
vhost-listen:前缀的 vhost 派生 listener，又存在显式 listener 时，listener_model完全由config.listeners的迭代顺序决定，可能给出误导性的诊断输出（例如混合场景被报告为vhost或explicit）。建议遍历全部 listener 后给出更确定的分类，例如全部以vhost-listen:开头时才报vhost，存在混合时报mixed。♻️ 建议改为基于全集合的判断
fn listener_model( listener_count: usize, first_listener_id: Option<&str>, first_listener_name: Option<&str>, + all_listener_ids: impl IntoIterator<Item = &str>, ) -> &'static str { if listener_count == 1 && first_listener_id == Some("default") && first_listener_name == Some("default") { "legacy" - } else if first_listener_id.is_some_and(|id| id.starts_with("vhost-listen:")) { - "vhost" } else { - "explicit" + let mut any_vhost = false; + let mut any_explicit = false; + for id in all_listener_ids { + if id.starts_with("vhost-listen:") { + any_vhost = true; + } else { + any_explicit = true; + } + } + match (any_vhost, any_explicit) { + (true, false) => "vhost", + (true, true) => "mixed", + _ => "explicit", + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/check/summary.rs` around lines 90 - 99, Current logic classifies listener_model using only the first listener (vars listener_count, first_listener_id, first_listener_name), which yields order-dependent results; change it to scan all listeners in config.listeners: keep the existing legacy branch (when listener_count == 1 and first_listener_id/name == "default"), otherwise compute counts/flags over the full set to decide: if every listener id starts_with("vhost-listen:") -> return "vhost"; if none start with that prefix -> return "explicit"; otherwise return "mixed". Ensure you reference the "vhost-listen:" prefix and replace the single-item-first-listener checks with the aggregate checks to produce deterministic output.crates/rginx-config/src/compile/upstream.rs (1)
190-192: 🧹 Nitpick | 🔵 Trivial
HashMap::collect会静默覆盖映射后同名条目,建议显式失败。若多个
UpstreamConfig在 scope 后映射到同一名称(同 scope + 同 name 的副本),collect::<HashMap<_,_>>()会丢失所有除最后一个之外的条目,而不会报错。虽然此前validate_upstreams应已拒绝同 vhost 内重名 upstream,但建议在编译层加一道防御,例如在插入前显式检测冲突并返回Error::Config,以避免日后改动验证顺序时出现静默丢失。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/upstream.rs` around lines 190 - 192, The current use of collect() on the iterator of Ok((name, compiled)) silently overwrites duplicate keys; change the logic to build the HashMap by iterating the results and inserting each (name, compiled) with an explicit collision check: if a name already exists, return Error::Config (matching the crate's error type) instead of overwriting. Update the code path around the Ok((name, compiled)) iterator/collect() in upstream.rs to perform a checked insert (using the existing validate_upstreams reference for context) so duplicate scope+name entries fail loudly during compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-app/tests/upstream_mtls.rs`:
- Around line 75-76: Add a short comment immediately above the assertion that
constructs expected_host and checks observed.host (the
assert_eq!(observed.host.as_deref(), Some(expected_host.as_str()))) explaining
that because the upstream is running HTTP/1.1 (http1::Builder::new()), hyper
will rebuild the Host header from the request URI authority even if the proxy
removed Host on the Auto+https path, so this assertion implicitly verifies the
invariant that h1 fallback reconstructs Host rather than the proxy directly
forwarding it; apply the same explanatory comment to the analogous assertions in
the 120-136 range.
In `@crates/rginx-config/src/compile/server/listener.rs`:
- Around line 151-162: The merge silently drops conflicting per-vhost flags
(ssl, proxy_protocol, http3) when inserting into bindings via
bindings.entry(parsed.addr).and_modify(...).or_insert(...); change this to use
the Entry API (match bindings.entry(parsed.addr)) and on Occupied(entry)
explicitly validate: if parsed.ssl != entry.get().ssl or parsed.proxy_protocol
!= entry.get().proxy_protocol or (parsed.http3 != entry.get().http3 && !(both
true with identical Http3Config)), return Error::Config describing the conflict
(use Error::Config and include addr and vhost names); only allow non-conflicting
merges (e.g., OR-ing proxy_protocol/ssl only if you intentionally want that
policy) or require identical http3 configs before merging; update code around
VhostListenerBinding, parsed, validate_vhost_listens to locate and apply this
check.
- Around line 168-188: 当前在 map 闭包中调用 compile_server_fields 时强制把
ServerFieldConfig 的 default_certificate 和 tls 设为 None，导致当 binding.ssl == true
且未显式提供默认证书时会生成一个默认为空的 SniCertificateResolver（acceptor.rs 中默认只在单 vhost
时隐式选首证书），从而使非 SNI 客户端在多 vhost 场景下无证书连接失败；请在生成 ServerFieldConfig 或在 map
前增加校验：对于所有 binding.ssl == true 的 binding，确保要么为该 binding 提供 explicit
default_certificate（将其传入 compile_server_fields 的
ServerFieldConfig.default_certificate），要么保证该 binding 仅包含单个 vhost（可隐式使用该 vhost
证书），否则返回编译错误/验证失败；如果业务上希望自动回退，可改为当只有一个 vhost 时将该 vhost 证书填充到 default_certificate
字段以供 SniCertificateResolver 使用（参考 SniCertificateResolver 和 acceptor.rs 的
by_name/默认选择逻辑）。
In `@crates/rginx-config/src/compile/tests/vhosts.rs`:
- Around line 4-66: Add tests that exercise conflicting vhost listen semantics
for compile_vhost_listeners: extend or add new test(s) (similar to
compile_generates_deduplicated_listeners_from_vhost_listen) that create two
VirtualHostConfig entries both listening on "127.0.0.1:8443" but with (1) one
having tls (tls: Some(...)) and the other tls: None to verify which listener/tls
flag wins, (2) both with "ssl" but one enabling proxy_protocol and the other not
to assert proxy_protocol merging behavior, and (3) both with "ssl" and different
Http3Config values to assert that only the first Http3Config is applied; use
compile_with_base to produce snapshot and add assertions on snapshot.listeners
(ids, tls_enabled), snapshot.total_listener_binding_count(), proxy_protocol
flags and snapshot.listeners[n].http3 fields to lock down the merge semantics
implemented by compile_vhost_listeners and related listener handling.
In `@crates/rginx-config/src/listen.rs`:
- Line 41: The match arm in listen.rs that currently swallows "default_server"
and "reuseport" (the branch `"default_server" | "reuseport" => {}`) should emit
a warning instead of being silent: locate the match handling in listen.rs (the
branch matching those literal tokens) and replace the empty arm with a warning
log (e.g., using the crate's logging facility like log::warn! or tracing::warn!)
that includes the directive name and, if available, the source location or vhost
context; ensure the message clearly states the directive is
unimplemented/ignored so users migrating from nginx see the behavior
(alternatively add a visible compile-time/doc note if logging is not
appropriate).
- Around line 57-71: Add unit tests for parse_listen_addr covering IPv6 and the
star-prefix cases: in the existing listeners test module add tests that assert
parse_listen_addr returns Ok for IPv6 with port (e.g., "[::]:8080" and
"[::1]:8443"), assert Err for IPv6 without port (e.g., "[::1]" or "::1"), assert
that "*:8080" is normalized to SocketAddr::from((Ipv4Addr::UNSPECIFIED, 8080))
(i.e., 0.0.0.0:8080), and assert numeric-only input returns an IPv4 unspecified
address with that port; use the parse_listen_addr function and check both
successful SocketAddr values and the error message shape for failures.
In `@crates/rginx-config/src/validate.rs`:
- Around line 30-35: 当前实现中 require_vhost_listen/validate_vhost_listens 会在 “任一
vhost 配置了非空 listen 就要求所有 vhost 必须有 listen” 的情况下对未声明 listen 的 vhost 报错；请在 vhost
模块的报错文案中增加明确提示，修改 validate_vhost_listens（或与之配套的错误构造处）将原错误消息扩展为类似 “注意：一旦任一 vhost
使用 servers[].listen，所有 vhost 必须显式声明 listen” 的说明，确保在
vhost::validate_virtual_hosts 的错误路径返回包含该额外上下文以便运维快速定位。
In `@crates/rginx-config/src/validate/vhost.rs`:
- Around line 79-85: The condition guarding the TLS/listen checks is redundant:
when require_vhost_listen is false, has_ssl_listen cannot be true so the
require_vhost_listen && ... guard on the first if is unnecessary and makes the
logic confusing; remove the require_vhost_listen part from both checks and
simply validate based on has_ssl_listen and vhost.tls (i.e., replace the two ifs
that reference require_vhost_listen with checks that return
Err(Error::Config(format!("{vhost_label} ssl listen requires tls"))) when
has_ssl_listen && vhost.tls.is_none() and return
Err(Error::Config(format!("{vhost_label} TLS requires an ssl listen"))) when
vhost.tls.is_some() && !has_ssl_listen), keeping vhost_label, has_ssl_listen,
and vhost.tls identifiers to locate the code in validate::vhost.rs.
In `@crates/rginx-http/src/proxy/common.rs`:
- Around line 125-131: Add an explicit comment in or immediately above the
function may_use_authority_pseudo_header (or at its call sites) stating that
when UpstreamProtocol::Auto and peer.scheme == "https" the code allows removing
the Host header because hyper's HTTP/1.1 Client will reconstruct the Host from
the request URI authority; reference this dependency on hyper's behavior so
future maintainers know the h1 fallback relies on hyper auto-deriving Host from
URI authority and should not be changed without verifying hyper semantics.
In `@crates/rginx-http/src/proxy/tests/request_headers.rs`:
- Around line 150-193: Add two tests to cover the remaining branches of
may_use_authority_pseudo_header: (1) a test that constructs a peer with an
arbitrary scheme and calls
remove_redundant_host_header_for_authority_pseudo_header with
UpstreamProtocol::Http2 (or Http3) and asserts the HOST header matching the
authority is removed; (2) a test that constructs a peer with peer.scheme ==
"http" and calls the function with UpstreamProtocol::Auto and asserts the HOST
header is preserved (not removed). Reference the existing tests for patterns and
use the same HeaderMap/HOST/HeaderValue helpers and resolved_peer_from_url to
create peers.
---
Outside diff comments:
In `@crates/rginx-app/src/check/summary.rs`:
- Around line 90-99: Current logic classifies listener_model using only the
first listener (vars listener_count, first_listener_id, first_listener_name),
which yields order-dependent results; change it to scan all listeners in
config.listeners: keep the existing legacy branch (when listener_count == 1 and
first_listener_id/name == "default"), otherwise compute counts/flags over the
full set to decide: if every listener id starts_with("vhost-listen:") -> return
"vhost"; if none start with that prefix -> return "explicit"; otherwise return
"mixed". Ensure you reference the "vhost-listen:" prefix and replace the
single-item-first-listener checks with the aggregate checks to produce
deterministic output.
In `@crates/rginx-config/src/compile/upstream.rs`:
- Around line 190-192: The current use of collect() on the iterator of Ok((name,
compiled)) silently overwrites duplicate keys; change the logic to build the
HashMap by iterating the results and inserting each (name, compiled) with an
explicit collision check: if a name already exists, return Error::Config
(matching the crate's error type) instead of overwriting. Update the code path
around the Ok((name, compiled)) iterator/collect() in upstream.rs to perform a
checked insert (using the existing validate_upstreams reference for context) so
duplicate scope+name entries fail loudly during compilation.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7440944-48f4-4590-b842-bef1ecf34e5e
📒 Files selected for processing (38)
README.mdconfigs/conf.d/default.ronconfigs/rginx.roncrates/rginx-app/src/check/summary.rscrates/rginx-app/tests/check/basic.rscrates/rginx-app/tests/check/summary.rscrates/rginx-app/tests/check/tls.rscrates/rginx-app/tests/upstream_http2.rscrates/rginx-app/tests/upstream_mtls.rscrates/rginx-config/src/compile/mod.rscrates/rginx-config/src/compile/route.rscrates/rginx-config/src/compile/server.rscrates/rginx-config/src/compile/server/listener.rscrates/rginx-config/src/compile/tests.rscrates/rginx-config/src/compile/tests/route.rscrates/rginx-config/src/compile/tests/vhosts.rscrates/rginx-config/src/compile/upstream.rscrates/rginx-config/src/compile/vhost.rscrates/rginx-config/src/lib.rscrates/rginx-config/src/listen.rscrates/rginx-config/src/model.rscrates/rginx-config/src/model/listener.rscrates/rginx-config/src/model/vhost.rscrates/rginx-config/src/validate.rscrates/rginx-config/src/validate/server.rscrates/rginx-config/src/validate/server/listener/listeners.rscrates/rginx-config/src/validate/tests.rscrates/rginx-config/src/validate/tests/vhosts.rscrates/rginx-config/src/validate/vhost.rscrates/rginx-http/src/proxy/clients/http_client.rscrates/rginx-http/src/proxy/common.rscrates/rginx-http/src/proxy/health/request.rscrates/rginx-http/src/proxy/request_body/prepare.rscrates/rginx-http/src/proxy/tests/grpc.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/proxy/tests/request_headers.rscrates/rginx-http/src/transition.rscrates/rginx-http/src/transition/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Test Fast
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-13T10:53:29.903Z
Learnt from: vansour
Repo: vansour/rginx PR: 43
File: crates/rginx-config/src/validate/server.rs:421-439
Timestamp: 2026-04-13T10:53:29.903Z
Learning: In `crates/rginx-config/src/validate/server.rs` (`validate_http3`), the check for `tls.session_tickets` when `http3.early_data = true` intentionally only rejects `Some(true)` and allows `None`. Requiring `Some(false)` breaks 0-RTT resumption and the `routes_http3_early_data_by_replay_safety` test and the CI `http3` gate in the current rustls/quinn integration. This is a deliberate design decision and should not be flagged as a bug until the runtime/session policy is updated to support that configuration.
Applied to files:
crates/rginx-config/src/model/listener.rscrates/rginx-app/tests/check/tls.rscrates/rginx-config/src/validate/tests.rscrates/rginx-http/src/transition/tests.rscrates/rginx-config/src/compile/tests/route.rscrates/rginx-config/src/validate/server.rscrates/rginx-app/tests/check/basic.rscrates/rginx-config/src/validate/server/listener/listeners.rscrates/rginx-config/src/validate.rscrates/rginx-http/src/transition.rscrates/rginx-app/tests/check/summary.rscrates/rginx-app/tests/upstream_mtls.rscrates/rginx-config/src/validate/vhost.rscrates/rginx-config/src/validate/tests/vhosts.rs
🔇 Additional comments (40)
README.md (1)
58-60: LGTM!文档说明与本 PR 的架构对齐目标一致：
rginx.ron仅保留全局 runtime/server 默认值，站点级listen/TLS/upstreams/locations下沉至conf.d/*.ron中的VirtualHostConfig。crates/rginx-config/src/model.rs (1)
37-38: LGTM!为
locations增加#[serde(default)]与 PR 中“顶层不再强制要求locations、改由VirtualHostConfig提供站点路由”的方向一致，且与validate.rs现已放宽的校验规则保持自洽。crates/rginx-config/src/compile/tests.rs (1)
64-64: LGTM!仅是模块声明，新增的
vhosts子模块为本 PR 的 vhost listener/upstream 编译测试提供入口。crates/rginx-config/src/lib.rs (1)
6-6: LGTM!
mod listen;内部模块声明与本 PR 引入的parse_vhost_listen/ParsedVhostListen实现匹配。crates/rginx-config/src/compile/tests/route.rs (1)
255-269: LGTM!
VirtualHostConfig新增的listen/upstreams/http3字段在该测试中被显式置空，保持现有 vhost id/route id 断言（servers[0]、servers[0]/routes[0]|exact:/）的有效性，符合本 PR 的字段扩展。crates/rginx-http/src/proxy/tests/grpc.rs (1)
224-232: LGTM —— 与 issue#61的修复方向一致请求被强制为 HTTP/2（line 225），此时
:authority已等价于Host，再发送Host反而会引发部分上游（如 issue#61中的 mirrors.ocf.berkeley.edu）回 400。本断言正确钉住了“gRPC 主动健康探测请求不应携带冗余Host”的新行为。建议同时在 request_headers 测试里覆盖 HTTP/1.1 仍保留Host的对照用例（若尚未覆盖），以防回归。crates/rginx-http/src/proxy/tests/mod.rs (1)
46-50: LGTM!仅是为
request_headers/grpc测试模块暴露新增的remove_redundant_host_header_for_authority_pseudo_header辅助函数，与 PR 中的 Host 头规范化逻辑保持一致。crates/rginx-http/src/proxy/request_body/prepare.rs (1)
87-91: LGTM!在
sanitize_request_headers之后调用remove_redundant_host_header_for_authority_pseudo_header的位置正确：先由 sanitize 步骤把Host规范化为upstream_authority（或保留preserve_host的原值），再针对支持:authority伪首部的协议（HTTP/2、HTTP/3，以及 HTTPS 下的Auto）剔除冗余的Host，从而避免某些 h2 源站对Host与:authority重复时返回 400，与 issue#61的目标一致。crates/rginx-config/src/validate/tests.rs (1)
127-156: LGTM!
sample_vhost现在正确初始化listen、upstreams、http3字段，与VirtualHostConfig的新字段及validate_vhost_listens在require_vhost_listen=false时的约束一致（空listen+tls=None+http3=None不会触发任一校验分支）。crates/rginx-config/src/model/listener.rs (1)
5-27: LGTM!为
Http3Config派生Default是合理的：所有字段均为Option<T>，默认即全部None，与compile/server/listener.rs中vhost.http3.clone().unwrap_or_default()的合并语义一致——当某个 vhost 启用了 http3 监听但未提供显式http3配置块时，会回落到全空配置而不影响反序列化路径。crates/rginx-http/src/proxy/clients/http_client.rs (1)
155-157: LGTM ——set_host(true)与新增的冗余Host移除逻辑互补。在
Auto + https路径下，remove_redundant_host_header_for_authority_pseudo_header会主动剔除等于upstream_authority的Host，以避免 h2 源站拒绝Host与:authority重复（issue#61根因）。一旦 ALPN 实际协商到 h1，Hyper 才能通过set_host(true)从 URI authority 自动补回Host。其他情况（h2/h3 帧不依赖Host、纯 h1 路径下sanitize_request_headers已写入Host）此开关均为 no-op，行为是安全的。crates/rginx-app/tests/check/tls.rs (1)
43-45: LGTM!
reload_tls_updates期望串新增的servers[].upstreams[].{tls,server_name,server_name_override}路径与crates/rginx-http/src/transition/tests.rs中reloadable_fields的扩展保持一致，覆盖 vhost 局部 upstream 的 TLS 热重载边界。crates/rginx-http/src/transition/tests.rs (1)
55-84: LGTM!
reloadable_fields扩展的servers[].upstreams[].{tls,server_name,server_name_override}与顶层upstreams[].*的语义对称；restart_required_fields加入servers[].listen也符合监听套接字变更需重启绑定的语义。这与crates/rginx-app/tests/check/{tls,summary}.rs中的字符串断言保持一致。crates/rginx-app/tests/check/summary.rs (1)
112-117: LGTM!
reload_requires_restart_for与tls_restart_required_fields期望串中插入的servers[].listen与transition/tests.rs的restart_required_fields顺序保持一致，正确反映 vhost 派生监听器变更需触发重启。crates/rginx-config/src/validate/server.rs (1)
49-55: LGTM!新的
validate_http3_config是一个简洁的 facade，签名与底层http3::validate_http3保持一致，并将 TLS 上下文按可选引用传入，这一接入点便于 vhost 校验路径在拥有vhost.tls时复用同一 HTTP/3 校验逻辑。crates/rginx-http/src/proxy/health/request.rs (1)
19-48: LGTM!
request_protocol选择逻辑（gRPC 强制Http2，否则跟随upstream.protocol）与请求版本设置和后续的remove_redundant_host_header_for_authority_pseudo_header调用保持一致——HOST在第 19 行被显式设置为peer.upstream_authority，post-build 清理只在“可走 :authority”时移除等价的HOST，这与request_body/prepare.rs的处理时机也是对齐的。crates/rginx-http/src/proxy/common.rs (1)
105-123: LGTM!按
peer.upstream_authority进行 ASCII 大小写无关字节比较的策略合理：在sanitize_request_headers的写入顺序下，普通转发场景HOST必然等于authority、会被清除；而proxy_set_headers或preserve_host提供的差异化HOST不会匹配，因此被保留——这与函数注释的 “explicit Host overrides intact” 语义一致。crates/rginx-config/src/validate.rs (1)
18-22: LGTM!校验放宽为
locations或servers至少一项非空的语义与新的 nginx 风格配置布局一致；request_buffering=On的错误文案也同步覆盖了server.listen与servers[].listen两种监听派生路径，与configs/rginx.ron和configs/conf.d/default.ron的拆分保持一致。Also applies to: 55-55
configs/conf.d/default.ron (1)
1-22: LGTM!默认 vhost 与 nginx 默认页风格对齐：
listen 0.0.0.0:80+localhost/127.0.0.1server_names、欢迎信息以及/healthz探针端点能让安装即可访问验证。这套默认配合configs/rginx.ron中清空的locations/server形成清晰的“全局默认 vs 站点配置”分层。configs/rginx.ron (1)
5-13: LGTM!把站点级绑定/证书/upstream/路由全部下放到
conf.d/*.ron、顶级rginx.ron仅保留runtime与全局默认值的拆分，与 PR 目标和 README 中描述的 nginx 对齐布局一致。crates/rginx-config/src/compile/server.rs (1)
49-56: LGTM!
compile_vhost_listeners的对外接口与compile_legacy_server、compile_listeners风格一致，所有去重逻辑内聚在listener子模块，便于测试和后续维护。crates/rginx-http/src/transition.rs (1)
3-27: LGTM!
RELOADABLE_FIELDS与RESTART_REQUIRED_FIELDS的扩展与transition/tests.rs::boundary_lists_are_stable的预期一致,数组长度也已同步更新为 13/7。新增字段路径与 PR 中 vhost 化的 listener/upstream 编译结果对齐。crates/rginx-app/tests/check/basic.rs (2)
29-34: LGTM!
reload_requires_restart_for与reload_tls_updates的断言字符串严格对应crates/rginx-http/src/transition.rs中RESTART_REQUIRED_FIELDS与RELOADABLE_FIELDS的最新顺序,字段顺序、命名一致。
117-138: LGTM!仓库默认配置断言由
legacy/routes=4切换为vhost/routes=2,与 PR 将默认configs/rginx.ron改为 vhost 风格架构一致。crates/rginx-config/src/validate/server/listener/listeners.rs (2)
15-27: LGTM!vhost-listen 模式下对 top-level
listeners的互斥校验、server字段空值校验以及绑定一致性校验三步走的早返回结构清晰,错误信息也明确指向冲突来源。
140-171: 确认 http3 跨 vhost 绑定一致性校验的设计意图当前
validate_vhost_listener_bindings仅对 ssl 与 proxy_protocol 进行跨 vhost 一致性校验,而 http3 被排除在外。同时compile_vhost_listeners中通过and_modify进行隐式合并:当多个 vhost 绑定到同一SocketAddr时,仅第一个启用 http3 的 vhost 生效,后续 vhost 的 http3 listen 被静默忽略。这与 ssl/proxy_protocol 的校验策略不一致(冲突会抛错)。需确认:
- 这种隐式合并是否为预期设计(允许 http3 跨 vhost 无声合并)
- 若是预期设计,应在代码中添加注释说明；若非,应在 validate 阶段补充校验
crates/rginx-app/tests/upstream_http2.rs (1)
137-180: LGTM —— 这是与 issue#61直接对应的关键回归测试。让上游在收到
Host头时返回 400(并附带可识别的响应体"unexpected h2 host header\n"),配合observed.host.is_none()双重断言,可以同时覆盖"代理仍向 H2 上游转发 Host"与"上游因此返回 4xx"两种回归路径。HOST在写入ObservedRequest之前先 clone 给响应分支用,顺序正确,无锁后操作风险。crates/rginx-config/src/validate/vhost.rs (2)
103-120: LGTM —— 但请确认空策略字段不会绕过 http3 校验。
server_tls_policy_from_vhost_tls把所有策略类字段(versions/cipher_suites/.../session_tickets/session_resumption)都置为None并复用现有validate_http3_config。结合已记录的设计(session_tickets = None在early_data = true时是允许的),此处不会回归 0-RTT 用例。仅作提醒:若未来validate_http3对其他策略字段加严,这里可能默默通过——届时需要把VirtualHostTlsConfig的相应字段透传,而不是再保持None。
87-98: 当前代码设计正确，不需要添加require_vhost_listen门控。
vhost.http3不应该在非 vhost-listen 模式下被使用，这是有意的架构限制。Http3Config 的listen字段（用于指定独立的 HTTP/3 绑定地址）需要 vhost 拥有自己的 listen 配置才有意义。与 TLS 不同（TLS 证书路径是简单参数，可以继承使用），HTTP/3 的 listen 字段需要在 vhost 级别上绑定。因此，当
vhost.listen为空时（top-level listeners 模式），无条件拒绝vhost.http3的设计是正确的，而非遗漏的门控。这不是回归风险。crates/rginx-config/src/compile/mod.rs (2)
56-69: LGTM!三分支的 listener 选择顺序合理:vhost-listen 优先,其次 legacy server 兜底,再次顶层 listeners 列表。
server.server_names.clone()作为 vhost 模式下的默认 server names 兜底是合理的(vhost 自身在后续validate_virtual_hosts中已校验必填)。
71-94: LGTM —— upstream 可见性顺序正确。关键时序:
default_vhost.routes在 line 74 用尚未合并 vhost-locals 的upstreams编译,vhost 编译循环(78-89)也只看到全局 upstreams,只有循环结束后才把每个 vhost 的 scoped upstreamsextend进总表(91-94)。这正确实现了"默认路由看不见 vhost 本地 upstream"且"vhost 间相互不可见"的隔离语义,与新测试validate_keeps_vhost_local_upstream_hidden_from_default_routes对齐。crates/rginx-config/src/compile/route.rs (1)
120-153: LGTM —— 名称解析与错误信息分离得当。
resolved_upstream用于实际的upstreams查表与ProxyTarget.upstream_name(对应ConfigSnapshot.upstreams中的 scoped key),而错误信息仍使用用户原始的upstream字符串,定位更直观。local_upstream_names.get(&upstream).cloned().unwrap_or_else(|| upstream.clone())的回退逻辑也保证了非 vhost 调用路径(传空 map)与原行为一致。crates/rginx-config/src/validate/tests/vhosts.rs (2)
180-248: LGTM —— 用例矩阵覆盖到位。新增四个用例分别覆盖了 vhost-listen 的 happy path、与
server.listen互斥、ssl-listen 与vhost.tls一致性、以及 vhost-local upstream 在自身 vhost 路由可见但对默认路由不可见,正好对齐validate/vhost.rs::validate_vhost_listens、validate/server/listener/listeners.rs::validate_listeners与compile/mod.rs中默认 vhost 先于 vhost-local extend 编译的关键不变量。
250-285: LGTM!
local_upstreamhelper 显式列出了UpstreamConfig的全部字段,避免了未来新增字段时该测试在编译期被默默忽略;同时使用Auto协议 +RoundRobin负载均衡是与 issue#61关注的默认场景一致的最小化构造。crates/rginx-config/src/model/vhost.rs (1)
1-36: LGTM!
deserialize_string_list通过#[serde(untagged)]枚举正确支持单字符串/字符串数组两种形式,与#[serde(default)]配合在字段缺失时也能合理回退。新增的upstreams与http3字段也与compile/vhost.rs、validate/vhost.rs中的处理对齐。crates/rginx-config/src/compile/tests/vhosts.rs (1)
100-109: LGTM!测试明确断言了 vhost-local upstream 优先于全局同名 upstream(
servers[0]::backend解析到:9001),这与compile_routes_with_local+scoped_upstream_name行为一致,是对 issue#61中默认行为的有力回归保护。crates/rginx-config/src/compile/upstream.rs (1)
22-32: LGTM!
compile_scoped_upstreams通过compile_upstreams_with_names复用主流程,scope 仅以名称映射方式注入,避免代码重复。scoped_upstream_name使用::分隔符与vhost_id形如servers[0]的结构一致,且不与合法 upstream 名重叠。crates/rginx-config/src/listen.rs (1)
32-39: nginx 中quic参数不需要显式ssl,此评论基于不正确的 nginx 行为认知。根据官方 nginx 文档,
listen 8443 quic reuseport;和listen 8443 ssl;是两条独立的指令,分别处理 UDP(QUIC) 和 TCP(TLS) 监听。quic和ssl是不同的监听模式,而非在同一指令中的参数组合。nginx 对 QUIC 的 TLS 通过ssl_protocols配置,而非ssl参数。因此,在 rginx 中http3/quic隐式启用ssl实际上是对 nginx 概念模型的合理简化,而非偏差。此评论的批评基础不成立。> Likely an incorrect or invalid review comment.crates/rginx-config/src/compile/vhost.rs (1)
28-42: 本条评审意见有误。编译过程中重复的 vhost 本地上游名称不会被静默丢弃。验证显示
compile_with_base()在第 43 行首先执行validate(&raw)?，随后才进行编译。验证阶段在validate/vhost.rs:46对每个 vhost 的上游调用validate_upstreams(), 该函数通过validate_upstream_name_and_peers()使用HashSet::insert()显式检查重复名称，并在检测到重复时返回错误"duplicate upstream"。因此，任何重复的上游名称都会在验证阶段被拒绝，不会到达编译阶段。行 28–42 的代码安全有效，因为它保证仅处理已通过验证的配置。> Likely an incorrect or invalid review comment.crates/rginx-config/src/compile/server/listener.rs (1)
212-218: 补充 IPv6 vhost 监听器的测试覆盖虽然当前代码中未发现将 listener ID 按冒号拆分或用正则匹配端口的逻辑，但 vhost 监听器的测试用例缺少 IPv6 地址覆盖。现有测试仅使用 IPv4 地址（
127.0.0.1:8080和127.0.0.1:8443），无法验证生成的 ID 格式vhost-listen:[::1]:8443在日志、metrics 和配置重新加载比对中是否正确处理。建议添加 IPv6 vhost 监听器的测试用例。
Allow server TLS defaults to apply to vhost-derived TLS listeners, reject unsupported listen options explicitly, and require shared HTTP/3 listener settings to match. Add coverage for IPv6 vhost listener IDs, server TLS defaults, unsupported listen options, HTTP/3 consistency, and Host header authority branches.
Summary
Fixes #61
Tests