Conversation
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明:本评审报告由 AI 自动生成,当前使用 DeepSeek-V4.1-Flash 模型进行分析。
📖 PR #104 — fix(caddy): merge same-port sites into a single Caddy server, fix Admin API 500
🎯 结论
结论:🔄 Request Changes
评分:功能 ⭐⭐⭐ | 最小改动 ⭐⭐⭐⭐ | 前向兼容 ⭐⭐⭐ | 方案设计 ⭐⭐⭐
变更:1 文件(+98 / −48,backend/service/caddy/manager.go)
行号说明:以下行号均为本 PR 自身改动的新文件行号(
+侧),与 GitHub 累积 diff 的行号可能不同。
涉及 Caddy 行为的结论已在本机核对依赖源码(仓库backend/go.mod指定github.com/caddyserver/caddy/v2 v2.11.1)。
整体方案(2-3 句)
把 Caddy server 的粒度从「每站点一个 server(netpanel_<id>)」改为「每端口一个 server(netpanel_port_<port>)」:Start / Stop / UpdateUpstream 三个入口统一收敛到新的 rebuildPortServer(port, overrides)——从 DB 取出该端口所有 enable + running 站点,合并它们的 routes 与 TLS policies 成一个 server,再整体 DELETE + PUT;端口没有运行中站点时删除该 server。根因判断是准的(同一 listen 地址被多个 server 占用,Caddy 明确拒绝,Admin API 由此 500),「一个 listener 对应一个 server」也正是 Caddy 的官方模型,方案选型没有更优替代。缺口在于「合并」把原来单站点作用域的路由与 TLS 策略变成了跨站点作用域,而只对 routes 加了 orderRoutes 保护,TLS policies 的排序/去重、重建失败的回滚、上游 override 的跨站点保留、以及 rebuild 的互斥都还没有收口。
核心改动清单
manager.go:107-133(Start)— 先写status=running,再整体重建该端口 server;删掉了原先「先 DELETE 旧 server 再 PUT 本站 server」的写法。manager.go:136-143(Stop)— 改为置stopped后重建该端口(本站自然被摘除),并加了site.Port > 0兜底。manager.go:154-180(UpdateUpstream)— 用overrides map[uint]string传入临时上游,保持「热切换不落库、重启回退」的既有语义。manager.go:644-708(rebuildPortServer)— 端口级聚合:逐站点建 routes/policies、合并、排序、写日志、DELETE+PUT。manager.go:710-726(orderRoutes)— 带 host matcher 的路由前置、catch-all 后置,避免后者抢先匹配。
问题清单
-
[P0]
manager.go:674-679(配合manager.go:527-534的buildTLSConfig)— 合并后的tls_connection_policies是「按站点顺序平铺」,但没有像 routes 一样做排序,而buildTLSConfig只在site.Domain != ""时才写match;一旦该端口上存在一个无匹配条件的 TLS policy(空域名站点开了 TLS),它会匹配所有 ClientHello。Caddy 的连接策略是顺序取第一个匹配项、matchers 为空即匹配全部(本机核对caddy@v2.11.1/modules/caddytls/connpolicy.go:45-47「an ordered group ... the first matching policy will be used」与:150-160的getConfigForClient循环),因此同端口其它站点的certificate_selection.any_tag: [cert_X]永远轮不到 → 别人的证书选不出来,HTTPS 握手失败/证书错配。这条路径在 UI 上是可复现的:webpage/src/pages/Caddy.tsx:193-196的「域名 / 主机名」可留空(提示语即「留空监听所有域名」),Caddy.tsx:260的tls_enable是独立 Switch、与域名无任何联动校验,所以「空域名 + 开启 TLS」能正常存库;同时manager.go:647-648的Find没有ORDER BY,站点顺序取决于 DB 执行计划 → 表现为「时好时坏」。修复建议(与orderRoutes保持对称,成本很小):// manager.go — 与 orderRoutes 对称:带 match.sni 的策略在前,无匹配条件的兜底策略在后 func orderTLSPolicies(pols []interface{}) []interface{} { var snis, catchAll []interface{} for _, p := range pols { if m, ok := p.(map[string]interface{}); ok { if _, has := m["match"]; has { snis = append(snis, p) continue } } catchAll = append(catchAll, p) } return append(snis, catchAll...) }
并建议顺手做两件事:①
m.db.Where(...).Order("id asc").Find(&sites)让构建顺序确定;② 在 Create/Update/rebuildPortServer里校验「同一端口上已有 TLS 站点时,空域名站点不允许开 TLS」,命中就setError提示,而不是让它静默污染别人的证书选择。 -
[P1]
manager.go:667-672— 单站点buildRoutes失败时只continue+log.Errorf:该站点的 DB 状态已经在manager.go:121-124被写成running,这里既不回滚也不setError,于是 UI 显示「运行中」,而它根本没被写进任何 server(静默不可达),运维只能去翻容器日志。建议continue前调用m.setError(site.ID, err.Error()),并考虑让本轮整次重建中止(见下一条,避免用一个残缺配置覆盖线上配置)。 -
[P1]
manager.go:703-706—DELETE与PUT之间既没有回滚也没有校验(未用caddy validate类能力,也未用POST /load的整份原子加载):PUT 失败时该端口整个 server 已经被删掉,同端口所有站点一起中断。改动前每个站点各自一个 server,失败只影响单站点;合并后爆炸半径变成了整端口。建议先保留上一份成功的serverCfg,PUT 失败时回写并setError;或改用POST /load一次性原子替换。 -
[P1]
manager.go:646-708—rebuildPortServer全程没有取任何锁:m.mu只在ensureCaddyRunning(manager.go:192-236)和StopAll(manager.go:87-104)里使用,而这是一个「读 DB → DELETE → PUT」的复合操作,内部没有任何互斥。并发来源至少四处:HTTP 的 Start/Stop/UpdateUpstream、StartAll的循环(manager.go:59-84)、linereg 每轮refresh触发的applyCaddySwitch(backend/service/linereg/linereg.go:423-470,DefaultInterval=60s、最小间隔 5s)、以及访问规则变更时的caddyMgr.Restart(backend/api/handlers/domain.go:1915)。交错时会出现「后写的配置覆盖前一个站点的变更」或 DELETE/PUT 命中 404。建议按端口加锁(sync.Map[int]*sync.Mutex)或用一个全局 rebuild mutex 串行化。 -
[P1]
manager.go:154-180+manager.go:646-665—overrides只携带当前站点,但 rebuild 会从 DB 重建该端口下所有站点,于是同端口其它站点此前通过UpdateUpstream打上的热切换上游会被冲掉、退回 DB 里的原始值。而调用方applyCaddySwitch是「遍历所有绑定 Caddy 的站点逐个调用」(linereg.go:428-470),成功后还会记账m.lastUpstream[siteID] = 新地址→ linereg 认为已切换、Caddy 实际是旧上游,状态不一致且无任何告警。改动前每个站点独立 server,不存在互相覆盖。建议在 Manager 内维护一份map[uint]string的上游覆盖表(rebuildPortServer时统一应用,站点停止/配置变更时清除),或让 linereg 侧一次性批量传入全部覆盖。 -
[P1]
manager.go:136-143—Stop丢弃了rebuildPortServer的返回值:删除/停止站点时若重建失败,DB 已标stopped但 Caddy 里该站点仍在对外服务(「停止不生效」)。想确认这里是否有意忽略?建议至少log.Errorf+setError,并让 handler 能把失败回传给前端(目前api/handlers/misc.go:93-101的 Stop/Delete 也不看错误)。 -
[P1]
manager.go:684-696— 同端口混合 TLS 与明文站点时,整个 listener 会被当作 TLS 终止口:Caddy 判定useTLS := len(srv.TLSConnPolicies) > 0 && port != httpPort(本机核对caddy@v2.11.1/modules/caddyhttp/app.go:558),因此只要该端口上有一个站点开了 TLS,同端口未开 TLS 的站点就无法再以 HTTP 访问(明文请求会被当成 TLS 握手)。这个组合在 UI 上是允许的(同端口两个站点、一个开一个不开),建议在合并时检测并给出显式警告/setError,并在docsite/features/caddy.md里写明限制。 -
[P1]
manager.go:710-726(配合manager.go:269-274、789-799)—orderRoutes只区分「有无 matcher」,不区分具体度,同端口下可能出现静默吞站点:①*.example.com站点若先于api.example.com站点入库,会先匹配并吞掉后者;② 两个站点填同一域名时只有一个生效,且没有任何提示;③buildRoutes只生成 host matcher、没有 path matcher,所以「同域名不同路径」这条常规做法在这里无法表达(只能靠match之外的字段,目前没有)。另外isLocalOrIP没把*、:8080这类写法归为「无 matcher」(Caddy.tsx:109把空域名显示为*),这类站点会落进 matched 组。建议:按具体度排序(精确域名 > 通配 > 无 matcher),并对「同端口重复 host」做重复检测 + 告警,避免「站点被吞掉但没人知道」;裸*在 Caddy host matcher 里的匹配语义想确认一下是否等价于 catch-all。 -
[P2]
manager.go:238-264— 改动后buildServerConfig已无调用者(全仓 grep 只剩定义本身;service/frp/manager.go:681的同名函数是另一个包),连同里面netpanel_%d的 logger 命名一起成了死代码。建议删除,避免后人误以为它仍是主路径。 -
[P2]
manager.go:698-700— 每次重建都把整份 server 配置以 INFO 打印,其中basic_auth分支会带上用户的 bcrypt hash(manager.go:345-360、476-524);改动前是单站点,现在放大到该端口所有站点,而且这条路径会被选线循环(5–60s)周期性触发。建议降到 Debug 级别并对password字段脱敏。 -
[P2]
manager.go:684-693—default_logger_name从netpanel_<siteID>变成netpanel_port_<port>,同端口站点共用一份访问日志,按站点过滤/统计的能力下降;另外若有外部脚本/监控按旧 server key(netpanel_<id>)读取 Caddy 配置会失效(仓库内未见依赖,需确认是否有外部消费方)。 -
[P2]
manager.go:59-84+manager.go:121-126—StartAll对每个站点各调一次Start,每个站点一次「重建 + 全配置日志」:N 个站点同端口时会重复 N 次 DELETE+PUT(前 N−1 次都是中间态)。建议按端口分组只重建一次。 -
[P2]
manager.go:526-592— 手动证书模式下每次 rebuild 都会POST /certificates重新上传证书(loadCertificate),重建频率上来后这笔 I/O 属于白做;且证书文件缺失时只打日志、policy 里仍保留any_tag,会演变成「选不到证书」。建议只在首次/证书变更时上传(疑似:反复 POST 是否会在 Caddy 的未托管证书缓存里累积,需确认)。 -
[P2]
manager.go:107-112— 站点Port没有范围校验(api/handlers/misc.go:42-72的 Create/Update 直接ShouldBindJSON,也没有校验),Port=0会生成listen: [":0"]的netpanel_port_0。建议在 Start/Create 校验 1–65535,并拒绝与面板自身端口冲突。
产品视角评估(3-5 句)
这个 PR 修的是真问题:用户把两个站点挂在同一个端口(例如都想用 :42443)时,改动前是「两个站点全 500、HTTPS 也开不起来」,改动后至少能用,属于纯粹的可用性修复,而且合并后不再需要用户理解「一个 listen 地址只能绑一个 server」这种 Caddy 内部细节。产品层面的风险在于新开放的组合没有被显式约束:同端口只能有一种协议栈(混用 TLS/明文时明文站点会失效)、同域名/通配重叠时只有一个站点生效、重建失败会波及整端口——这些都是用户在 UI 上可以自由构造、但界面没有任何提示的场景,建议在 Caddy 页面加一条校验或提示(「该端口已有站点,同端口需使用相同协议」「该域名已被站点 X 使用」)。另一个更小的 MVP 方向是可选的:合并逻辑其实只需 orderRoutes + TLS 排序就能覆盖大多数场景,若担心整端口重建的影响面,也可以先只对「同端口冲突」报明确错误,让用户换端口,再迭代到自动合并。
兼容性/迁移风险
无 DB schema 变更;server key 与日志名语义变化(见 P2)不影响库内数据。升级路径友好:ensureCaddyRunning 以 {"servers":{}} 启动引擎,Caddy 在提供配置时不会回读 autosave,因此旧的 netpanel_<id> server 不会残留(无需迁移脚本);回滚到旧版本只会退回「同端口 500」的旧行为。需要注意的行为变化是「同端口站点现在会真实共存」,建议在 docsite/features/caddy.md 补一节说明三种边界(同端口协议、同域名重叠、重建失败的影响范围),以及重建期间该端口站点会有短暂配置空窗。
值得肯定的点(1-3 条)
- 根因定位准确:
listener address repeated正是 Caddy 对「多个 server 抢同一 listen 地址」的明确拒绝,「一端口一 server」也正是官方推荐模型,不是想办法绕过报错。 UpdateUpstream用overrides参数承载临时覆盖、坚持不写库,完整保留了「热切换、重启后回退用户原始配置」的既有语义,没有顺手改动 DB。orderRoutes意识到 catch-all 会抢匹配、Stop里补了site.Port > 0的兜底,说明作者对「合并后语义变化」是有预判的——只差把同一套思路补到 TLS policies 与失败回滚上。
建议操作理由(1 句)
方向与根因都对,但合并把单站点作用域的 TLS policy 变成了跨站点作用域又没做排序,存在可复现的静默串证书路径,建议先补 TLS 策略排序/校验与重建失败回滚再合入。
d9220ba to
dd8ab38
Compare
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明。
🎯 结论
🔄 Request Changes — 静默失效场景需要日志告警
📖 概要
修复 Caddy 同端口站点合并导致 Admin API 500 错误
🚨 关键问题
P1:
- 💡
caddy/manager.go同端口站点查询缺少确定性.Order(),多个 catch-all(无 host matcher)站点合并后只有一个生效,其余静默失效且无告警,把"显式500"换成了"更隐蔽的静默失败"。建议检测到多个 catch-all 路由时记录 warning。 - 💡 单站点路由构建失败被跳过但不向上传播错误,调用方可能收到误导性的成功响应。
- 💡(设计权衡,需文档化)合并为单 server 后,同端口任一站点操作都会导致该端口下全部站点短暂中断(DELETE+PUT 非原子),建议文档说明或考虑增量更新替代全量重建。
🎯 结论:🔄 Request Changes — 核心思路正确,建议补充边界场景的日志/文档后合并
每个站点此前独立注册 server(netpanel_<id>),多个站点配置相同监听端口 (如 :42443)时 Caddy 报 listener address repeated,配置加载失败返回 500。 - Start/Stop/UpdateUpstream 改为按端口重建服务器:查询该端口所有 enable+running 的站点,把路由与 TLS 策略合并进同一 server (netpanel_port_<port>) - orderRoutes 保证 host matcher 路由排在 catch-all 之前 - UpdateUpstream 热切换以 overrides 参数注入临时上游,不写库 - 端口下无运行站点时删除对应 server 修复 PIKACHUIM#97
dd8ab38 to
56976bb
Compare
Problem
When multiple Caddy sites are configured with the same listen port (e.g.
:42443), each site registers its own server (netpanel_<id>). Caddy then fails to load the config withlistener address repeatedand the Admin API returns 500 — this also breaks HTTPS enablement for those sites.Fixes #97
Solution
Deploy one Caddy server per port (
netpanel_port_<port>) shared by all sites listening on it:Start/Stop/UpdateUpstreamnow rebuild the whole port server: collect allenable + runningsites on that port, merge their routes and TLS policies into a single serverorderRoutesguarantees host-matcher routes are ordered before the catch-all routeUpdateUpstreamhot-switches upstreams via theoverridesparam — no DB writesVerification
go build ./...✅go vet ./service/caddy/✅go test ./service/caddy/ -race✅