Extract /api/ timeout-override middleware out of runServeCmd#47891
Extract /api/ timeout-override middleware out of runServeCmd#47891raju249 wants to merge 1 commit into
Conversation
Move the per-route request read/write deadline overrides (sync script runs, large installer/bootstrap uploads, Android signup SSE, MDM profile batch) into apiTimeoutOverrideHandler in a new cmd/fleet/http_middleware.go. Behavior is unchanged; the closure is wired into rootMux via a single call. Add a unit test covering the installer max-size context threading for package-upload paths and that the wrapped handler is always invoked. Refs fleetdm#33370
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughThe inline HTTP middleware closure registered at 🚥 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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleet/http_middleware_test.go (1)
21-45: ⚡ Quick winExpand table coverage for the remaining middleware route predicates.
This test currently protects only a subset of the branch conditions in
apiTimeoutOverrideHandler. Adding cases for PATCH title-package, package-token GET, and orbit package POST would make this refactor safer against predicate drift.Suggested additions
@@ { name: "non-upload request leaves the default max size", method: http.MethodGet, path: "/api/latest/fleet/hosts", wantInstallerSize: installersize.MaxSoftwareInstallerSize, }, + { + name: "title package patch threads configured max size", + method: http.MethodPatch, + path: "/api/latest/fleet/software/titles/123/package", + wantInstallerSize: customMax, + }, + { + name: "package token route threads configured max size", + method: http.MethodGet, + path: "/api/latest/fleet/software/titles/123/package/token", + wantInstallerSize: customMax, + }, + { + name: "orbit package route threads configured max size", + method: http.MethodPost, + path: "/api/latest/fleet/orbit/software_install/package", + wantInstallerSize: customMax, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet/http_middleware_test.go` around lines 21 - 45, The table-driven test is missing coverage for some route predicates handled by apiTimeoutOverrideHandler. Add three new test cases to the existing table following the same struct pattern with name, method, path, and wantInstallerSize fields: one for PATCH title-package endpoint, one for package-token GET endpoint, and one for orbit package POST endpoint. Each should specify the appropriate HTTP method, route path, and expected installer size value to ensure all branch conditions in apiTimeoutOverrideHandler are tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/fleet/http_middleware_test.go`:
- Around line 21-45: The table-driven test is missing coverage for some route
predicates handled by apiTimeoutOverrideHandler. Add three new test cases to the
existing table following the same struct pattern with name, method, path, and
wantInstallerSize fields: one for PATCH title-package endpoint, one for
package-token GET endpoint, and one for orbit package POST endpoint. Each should
specify the appropriate HTTP method, route path, and expected installer size
value to ensure all branch conditions in apiTimeoutOverrideHandler are tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c65f18e-8048-438d-9adc-494974a50544
📒 Files selected for processing (3)
cmd/fleet/http_middleware.gocmd/fleet/http_middleware_test.gocmd/fleet/serve.go
|
Hello @MagnusHJensen 👋 I extracted the api handler here into its own file with a test. It reduces the size of serve.go marginally. What do you think? Mind taking a review, please? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47891 +/- ##
========================================
Coverage 67.23% 67.24%
========================================
Files 3637 3641 +4
Lines 229949 230164 +215
Branches 11788 11788
========================================
+ Hits 154608 154772 +164
- Misses 61457 61489 +32
- Partials 13884 13903 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Extracts the
/api/request timeout/body-size override middleware out ofrunServeCmdand intoapiTimeoutOverrideHandlerin a newcmd/fleet/http_middleware.go. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562).runServeCmddrops from ~1000 to ~900 lines, andserve.gofrom 1475 to 1373.The middleware is the
~100-linerootMux.HandleFunc("/api/", ...)closure that applies per-route read/write deadline overrides for endpoints that legitimately run long — synchronous script runs, large software-installer and bootstrap-package uploads, the Android enterprise signup SSE stream, and large MDM profile batch operations — and, for package-upload routes, caps the request body and threads the configured max installer size through the request context.Behavior is preserved — the handler is moved verbatim and wired into
rootMuxvia a singleapiTimeoutOverrideHandler(apiHandler, config, logger)call, so the same routes get the same overrides and every request still falls through toapiHandler.ServeHTTP. The now-unusedscriptsandinstallersizeimports drop out ofserve.go.On test scope:
TestAPITimeoutOverrideHandlerverifies the real decision in this middleware — that package-upload paths thread the configured max installer size into the request context (and non-upload requests keep the default) — and that the wrapped API handler is always invoked. The deadline overrides themselves go throughhttp.ResponseController, which a unit-testResponseRecorderdoesn't support (the handler logs and proceeds, as in production), so those are exercised by booting the server rather than asserted in a unit test.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit