From f3b67e9abc2328c683a5b24f0b8eb0a16e0eab01 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sun, 6 Sep 2026 21:36:39 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20keep=20the=20rate=20limiter?= =?UTF-8?q?=20away=20from=20the=20health=20probes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The limiter is installed on the engine and the probes are routes like any other, so above the threshold they are answered with 429 too. Point a liveness probe at one and the failure mode writes itself: traffic crosses the threshold, the probe collects three 429s, the kubelet restarts the container, the capacity that was already short gets shorter, and the instances that are left are pushed further past the threshold. The limiter working exactly as designed is what kills the pod. It is the argument common/health already makes about restarting a process whose database is unreachable, applied to load: turning one outage into a crash loop is not an improvement on the outage. Nothing points a liveness probe at these routes yet. The manifest that will is two commits away, and this has to land first, because that manifest without this change would be actively harmful. The exemption wraps the middleware rather than teaching the limiter about these paths. common/ may not import app/ - the contract check enforces it - so the limiter cannot name routes that are registered over there. Wrapping it in the command package, which imports both, is what keeps the boundary. Naming those routes needs them exported, so the group prefix and the two paths become constants and the router function becomes RegisterMonitorRouter. That also gives a test something real to mount: a probe asserted against a re-implementation of itself is a test of the copy. The check that the middleware never runs is separate from the check that the answer is not 429, because a probe can produce a 429 on its own. What has to be true is that the request never reached the limiter. --- app/other/router/monitor.go | 23 ++++++++++--- app/other/router/router.go | 11 ++++-- cmd/api/server.go | 31 ++++++++++++++++- cmd/api/shutdown_test.go | 67 +++++++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 7 deletions(-) diff --git a/app/other/router/monitor.go b/app/other/router/monitor.go index 999f011d..b2d87556 100644 --- a/app/other/router/monitor.go +++ b/app/other/router/monitor.go @@ -13,7 +13,7 @@ import ( ) func init() { - routerNoCheckRole = append(routerNoCheckRole, registerMonitorRouter) + routerNoCheckRole = append(routerNoCheckRole, RegisterMonitorRouter) } // readyTimeout bounds the whole probe. It has to stay under whatever period @@ -21,8 +21,23 @@ func init() { // a queue of readiness checks. const readyTimeout = 2 * time.Second +// HealthPath and ReadyPath are the two probe routes, relative to APIPrefix. +// +// Exported for the same reason as the prefix: the rate limiter has to be told +// to skip them, and it is installed in a package that cannot import this one. +const ( + HealthPath = "/health" + ReadyPath = "/ready" +) + +// RegisterMonitorRouter mounts the metrics endpoint and the two probes on v1. +// +// Exported so that a test can put the real probes on a server of its own. The +// alternative - a test that re-implements the handler it means to check - is +// how a probe comes to be asserted against a copy of itself. +// // 无需认证的路由代码 -func registerMonitorRouter(v1 *gin.RouterGroup) { +func RegisterMonitorRouter(v1 *gin.RouterGroup) { v1.GET("/metrics", transfer.Handler(promhttp.Handler())) // 健康检查（存活） @@ -31,7 +46,7 @@ func registerMonitorRouter(v1 *gin.RouterGroup) { // you", and a process whose database is unreachable does not want // restarting - that turns one outage into a crash loop and throws away the // connection pool, the cache and every in-flight request along the way. - v1.GET("/health", func(c *gin.Context) { + v1.GET(HealthPath, func(c *gin.Context) { c.Status(http.StatusOK) }) @@ -41,7 +56,7 @@ func registerMonitorRouter(v1 *gin.RouterGroup) { // is unreachable, and from the moment shutdown begins. Nothing waits on // that second answer today, so it is readable rather than actionable - // the package comment in common/health says what it would take. - v1.GET("/ready", func(c *gin.Context) { + v1.GET(ReadyPath, func(c *gin.Context) { if health.Draining() { c.JSON(http.StatusServiceUnavailable, gin.H{ "status": "draining", diff --git a/app/other/router/router.go b/app/other/router/router.go index 8ed4306b..8897eb5b 100644 --- a/app/other/router/router.go +++ b/app/other/router/router.go @@ -10,6 +10,13 @@ var ( routerCheckRole = make([]func(v1 *gin.RouterGroup, authMiddleware *jwt.GinJWTMiddleware), 0) ) +// APIPrefix is the group every route below is registered under. +// +// Exported because the middleware chain in cmd/api has to name two of those +// routes in full - the rate limiter is installed on the engine and must skip +// the probes - and a prefix spelled in two places is a prefix that drifts. +const APIPrefix = "/api/v1" + // initRouter 路由示例 func initRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) *gin.Engine { @@ -24,7 +31,7 @@ func initRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) *gin.Engine // noCheckRoleRouter 无需认证的路由示例 func noCheckRoleRouter(r *gin.Engine) { // 可根据业务需求来设置接口版本 - v1 := r.Group("/api/v1") + v1 := r.Group(APIPrefix) for _, f := range routerNoCheckRole { f(v1) @@ -34,7 +41,7 @@ func noCheckRoleRouter(r *gin.Engine) { // checkRoleRouter 需要认证的路由示例 func checkRoleRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) { // 可根据业务需求来设置接口版本 - v1 := r.Group("/api/v1") + v1 := r.Group(APIPrefix) for _, f := range routerCheckRole { f(v1, authMiddleware) diff --git a/cmd/api/server.go b/cmd/api/server.go index 87507968..432788f6 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -28,6 +28,7 @@ import ( "go-admin/app/admin/models" "go-admin/app/admin/router" "go-admin/app/jobs" + otherrouter "go-admin/app/other/router" "go-admin/common/database" "go-admin/common/global" "go-admin/common/health" @@ -476,10 +477,38 @@ func initRouter() { r.Use(handler.TlsHandler()) } //r.Use(middleware.Metrics()) - r.Use(common.Sentinel()). + r.Use(exemptProbes(common.Sentinel())). Use(common.RequestId(pkg.TrafficKey)). Use(api.SetRequestLogger) common.InitMiddleware(r) } + +// probePaths are the two routes the rate limiter must not answer for. +var probePaths = map[string]bool{ + otherrouter.APIPrefix + otherrouter.HealthPath: true, + otherrouter.APIPrefix + otherrouter.ReadyPath: true, +} + +// exemptProbes wraps a middleware so the health and readiness routes skip it. +// +// The limiter is installed on the engine and the probes are routes like any +// other, so above the threshold they are answered with 429 as well. A liveness +// probe that collects 429s fails its threshold and the container is restarted, +// which takes capacity out of a deployment that is already short of it and +// pushes the rest closer to the threshold - the limiter working exactly as +// intended is what causes it. It is the argument common/health makes about +// restarting a process whose database is unreachable, applied to load. +// +// Wrapping rather than teaching the limiter about these paths: the limiter +// lives under common/, which may not import the package that registers them. +func exemptProbes(h gin.HandlerFunc) gin.HandlerFunc { + return func(c *gin.Context) { + if probePaths[c.FullPath()] { + c.Next() + return + } + h(c) + } +} diff --git a/cmd/api/shutdown_test.go b/cmd/api/shutdown_test.go index 1112fb65..74df27a5 100644 --- a/cmd/api/shutdown_test.go +++ b/cmd/api/shutdown_test.go @@ -1,9 +1,14 @@ package api import ( + "net/http" + "net/http/httptest" "testing" "time" + "github.com/gin-gonic/gin" + + otherrouter "go-admin/app/other/router" ext "go-admin/config" ) @@ -33,3 +38,65 @@ func TestDefaultBudgetIsTheConfiguredFallback(t *testing.T) { t.Errorf("defaultBudget = %+v, want the unconfigured budget %+v", got, want) } } + +// The rate limiter must not answer for the probes. +// +// It is installed on the engine, so without this the probes are limited like +// any other route and answer 429 above the threshold. A liveness probe that +// collects 429s fails its threshold and the container is restarted - taking +// capacity out of a deployment that is already short of it and pushing the +// rest closer to the threshold. The limiter working exactly as designed is +// what would cause it. +// +// The stand-in rejects everything rather than being a real limiter: what is +// under test is which requests reach it, and a real one would need the traffic +// to cross a threshold before it said anything. +func TestTheProbesSkipTheRateLimiter(t *testing.T) { + gin.SetMode(gin.TestMode) + + var reached []string + r := gin.New() + r.Use(exemptProbes(func(c *gin.Context) { + reached = append(reached, c.FullPath()) + c.AbortWithStatus(http.StatusTooManyRequests) + })) + v1 := r.Group(otherrouter.APIPrefix) + otherrouter.RegisterMonitorRouter(v1) + v1.GET("/business", func(c *gin.Context) { c.Status(http.StatusOK) }) + + for _, tc := range []struct { + path string + limited bool + }{ + {otherrouter.APIPrefix + otherrouter.HealthPath, false}, + {otherrouter.APIPrefix + otherrouter.ReadyPath, false}, + // Not a probe, and deliberately not exempt: the exemption is for the + // two routes an orchestrator acts on, not for everything under + // /api/v1 that happens to be unauthenticated. + {otherrouter.APIPrefix + "/metrics", true}, + {otherrouter.APIPrefix + "/business", true}, + } { + t.Run(tc.path, func(t *testing.T) { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, tc.path, nil)) + + if tc.limited { + if w.Code != http.StatusTooManyRequests { + t.Errorf("answered %d, want the middleware's 429 - it was skipped for a route that is not a probe", w.Code) + } + return + } + if w.Code == http.StatusTooManyRequests { + t.Errorf("answered 429; a probe that can be rate-limited gets the container restarted under load") + } + }) + } + + // Said separately, because a probe could also answer 429 by itself: what + // has to be true is that the middleware never saw the request. + for _, p := range reached { + if probePaths[p] { + t.Errorf("the middleware ran for %s", p) + } + } +}