[management] Propagate context changes to upstream middleware#5956
[management] Propagate context changes to upstream middleware#5956
Conversation
📝 WalkthroughWalkthroughThis PR refactors the authentication middleware to mutate requests in-place via context propagation instead of returning modified request objects, while adjusting telemetry middleware to preserve and capture context changes made during request handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
management/server/telemetry/http_api_metrics.go (1)
196-201: LGTM — context propagation correctly aligned with auth middleware.Retaining
reqacrossServeHTTPand re-readingreq.Context()after the handler returns is the correct counterpart toauth_middleware.go's in-place*r = *...mutation: because both sides operate on the same*http.Request, the ctx populated withUserAuthbecomes visible to this middleware's subsequent logging/metrics. Thecontext.AfterFuncat line 187 correctly captures the pre-mutation ctx variable, so cancellation semantics are unchanged.One optional readability tweak: since
ris no longer used after line 197 other than throughreq, you could drop the new name:🧹 Optional readability tweak
- // Hold on to req so auth's in-place ctx update is visible after ServeHTTP. - req := r.WithContext(ctx) - h.ServeHTTP(w, req) + // Hold on to r so auth's in-place ctx update is visible after ServeHTTP. + r = r.WithContext(ctx) + h.ServeHTTP(w, r) close(handlerDone) - ctx = req.Context() + ctx = r.Context()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/telemetry/http_api_metrics.go` around lines 196 - 201, The code creates a new local variable req := r.WithContext(ctx) only to call h.ServeHTTP(w, req) and then read ctx = req.Context(); simplify by reusing r instead of introducing req: replace the local req usage so that you call h.ServeHTTP with the updated request (r = r.WithContext(ctx) or equivalent) and then read ctx = r.Context() after ServeHTTP; update references to handlerDone and the surrounding context.AfterFunc usage to ensure behavior is unchanged (ServeHTTP, handlerDone, and auth_middleware.go remain the relevant symbols).management/server/http/middleware/auth_middleware.go (1)
167-171: Duplication of in-place request mutation warrants encapsulation.The pattern
*r = *nbcontext.SetUserAuthInRequest(r, userAuth)at lines 169 and 220 is necessary and correct—the outer telemetry middleware (inmanagement/server/telemetry/http_api_metrics.go:201) readsreq.Context()afterServeHTTPreturns, so context updates must mutate in-place to be observable upstream. However, the pattern is duplicated identically in bothcheckJWTFromRequestandcheckPATFromRequest, with only a terse comment explaining the rationale.Consider extracting a small helper:
♻️ Proposed helper
// setUserAuthOnRequest replaces *r with a shallow copy whose context carries userAuth. // The in-place replacement is deliberate: outer middleware retains the same *http.Request // and must observe the updated context after ServeHTTP returns. func setUserAuthOnRequest(r *http.Request, userAuth auth.UserAuth) { *r = *nbcontext.SetUserAuthInRequest(r, userAuth) }Then at both call sites:
- // propagates ctx change to upstream middleware - *r = *nbcontext.SetUserAuthInRequest(r, userAuth) + setUserAuthOnRequest(r, userAuth)This removes duplication and clarifies intent for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/middleware/auth_middleware.go` around lines 167 - 171, Extract the duplicated in-place request mutation into a small helper (e.g., setUserAuthOnRequest) to replace the repeated pattern "*r = *nbcontext.SetUserAuthInRequest(r, userAuth)" used in checkJWTFromRequest and checkPATFromRequest; implement setUserAuthOnRequest(r *http.Request, userAuth auth.UserAuth) to perform the deliberate in-place replacement and call that helper from both checkJWTFromRequest and checkPATFromRequest so the intent and rationale are encapsulated and duplication removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/store/sql_store.go`:
- Line 48: The import alias nbutil (nbutil "github.com/netbirdio/netbird/util")
is unused in sql_store.go and will break compilation; remove the unused import
line or use the intended symbol from the util package where needed (update
functions in this file that should call util methods to reference nbutil, or
simply delete the nbutil import statement) so the compiler no longer reports an
unused import.
---
Nitpick comments:
In `@management/server/http/middleware/auth_middleware.go`:
- Around line 167-171: Extract the duplicated in-place request mutation into a
small helper (e.g., setUserAuthOnRequest) to replace the repeated pattern "*r =
*nbcontext.SetUserAuthInRequest(r, userAuth)" used in checkJWTFromRequest and
checkPATFromRequest; implement setUserAuthOnRequest(r *http.Request, userAuth
auth.UserAuth) to perform the deliberate in-place replacement and call that
helper from both checkJWTFromRequest and checkPATFromRequest so the intent and
rationale are encapsulated and duplication removed.
In `@management/server/telemetry/http_api_metrics.go`:
- Around line 196-201: The code creates a new local variable req :=
r.WithContext(ctx) only to call h.ServeHTTP(w, req) and then read ctx =
req.Context(); simplify by reusing r instead of introducing req: replace the
local req usage so that you call h.ServeHTTP with the updated request (r =
r.WithContext(ctx) or equivalent) and then read ctx = r.Context() after
ServeHTTP; update references to handlerDone and the surrounding
context.AfterFunc usage to ensure behavior is unchanged (ServeHTTP, handlerDone,
and auth_middleware.go remain the relevant symbols).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a2e34c-3caf-4139-8729-22b45a3c4f03
📒 Files selected for processing (3)
management/server/http/middleware/auth_middleware.gomanagement/server/store/sql_store.gomanagement/server/telemetry/http_api_metrics.go
|
2 participants
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit