feat: dedicated admin port for endpoint isolation#78
Merged
Conversation
Add ADMIN_PORT config that serves pprof, metrics, and swagger on a separate HTTP server, enabling network-level isolation via Kubernetes NetworkPolicy. When ADMIN_PORT=0 (default), behavior is unchanged — all endpoints share HTTPPort. - New config field: AdminPort (env: ADMIN_PORT, default: 0) - Separate http.Server for admin mux when AdminPort > 0 - Admin server in errgroup with proper shutdown lifecycle - Config validation: range check, conflict with GRPC_PORT/HTTP_PORT - Tests: separation, combined behavior, swagger on admin, validation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an exported Config.AdminPort, validates its value and conflicts, and changes HTTP initialization to optionally run a separate admin HTTP server for pprof/metrics/swagger; includes unit tests and README API anchor/link updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant Core
participant HTTPServer as "Gateway\n(HTTP Server)"
participant AdminServer as "Admin\n(HTTP Server)"
rect rgba(100,150,200,0.5)
Note over Client,Config: Startup flow
Client->>Config: load config (AdminPort)
Config-->>Core: provide config
end
rect rgba(150,100,200,0.5)
Note over Core,HTTPServer: initHTTP decision
Core->>HTTPServer: create gateway server on HTTPPort
Core->>AdminServer: if AdminPort>0 && AdminPort!=HTTPPort create admin server on AdminPort
end
rect rgba(200,150,100,0.5)
Note over Core,HTTPServer: runtime
Core->>HTTPServer: start goroutine
Core->>AdminServer: start goroutine (if created)
Client->>HTTPServer: GET /api/...
Client->>AdminServer: GET /debug/pprof or /metrics or /swagger
end
rect rgba(120,180,120,0.5)
Note over Core,HTTPServer: shutdown
Core->>AdminServer: Shutdown(ctx) (if exists)
Core->>HTTPServer: Shutdown(ctx)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for isolating admin endpoints (pprof, Prometheus metrics, Swagger) onto a dedicated HTTP server/port via a new ADMIN_PORT config option, while preserving existing combined-port behavior when ADMIN_PORT=0.
Changes:
- Introduces
AdminPortto configuration with validation for port range and conflicts. - Splits HTTP handling so admin endpoints can be served on a separate
adminServerwhen configured. - Adds unit and integration tests covering config validation and admin/gateway endpoint separation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Regenerated docs reflecting updated line references after code changes. |
| core.go | Adds adminServer and updates HTTP initialization/startup/shutdown to optionally run admin endpoints on a separate port. |
| core_coverage_test.go | Adds integration-style tests verifying endpoint separation/combined behavior and Swagger placement. |
| config/README.md | Regenerated config docs to include AdminPort. |
| config/config.go | Adds AdminPort field and validation warnings for range and port conflicts. |
| config/config_test.go | Adds tests for AdminPort validation scenarios. |
💡 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.
🧹 Nitpick comments (1)
core_coverage_test.go (1)
1454-1563: Please add one real split-mode lifecycle test.These assertions only exercise handlers in-memory. A
Run()/Stop()test withAdminPort > 0would catch regressions in the extraListenAndServe/shutdown path, and it’s also the right place to assert that swagger is not still reachable through the gateway in split mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 1454 - 1563, Add a real split-mode lifecycle test that constructs a cb with AdminPort > 0, SwaggerURL set and an openAPIHandler, calls initHTTP to prepare servers, then starts the servers via the public Run/Start method (or the same entrypoint used in production) in a goroutine and later stops them via Stop/Shutdown; assert that (1) the admin port responds to /swagger/index.html with the openAPIHandler body, (2) the gateway (HTTPPort) does NOT serve the swagger path (returns 404 or not the swagger body), and (3) after Stop/Shutdown the TCP ports are closed (e.g. net.Dial to ListenHost:AdminPort/HTTPPort fails). Locate and modify tests near TestInitHTTP_AdminPortSeparation/TestInitHTTP_AdminPortSwagger and use cb, initHTTP, adminServer, openAPIHandler, Run/Stop (or the package's start/stop entrypoints) to implement the lifecycle assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 1454-1563: Add a real split-mode lifecycle test that constructs a
cb with AdminPort > 0, SwaggerURL set and an openAPIHandler, calls initHTTP to
prepare servers, then starts the servers via the public Run/Start method (or the
same entrypoint used in production) in a goroutine and later stops them via
Stop/Shutdown; assert that (1) the admin port responds to /swagger/index.html
with the openAPIHandler body, (2) the gateway (HTTPPort) does NOT serve the
swagger path (returns 404 or not the swagger body), and (3) after Stop/Shutdown
the TCP ports are closed (e.g. net.Dial to ListenHost:AdminPort/HTTPPort fails).
Locate and modify tests near
TestInitHTTP_AdminPortSeparation/TestInitHTTP_AdminPortSwagger and use cb,
initHTTP, adminServer, openAPIHandler, Run/Stop (or the package's start/stop
entrypoints) to implement the lifecycle assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7adf57b9-7dc5-4395-94b3-1bc15cc4aa4d
📒 Files selected for processing (6)
README.mdconfig/README.mdconfig/config.goconfig/config_test.gocore.gocore_coverage_test.go
…match - Use c.runHTTP(gctx, ...) for admin server to short-circuit startup when errgroup context is already cancelled (Copilot review) - When AdminPort == HTTPPort, use combined mode instead of two servers binding the same port — avoids bind failure - Validation warns about combined fallback instead of erroring - Add test for AdminPort == HTTPPort combined mode behavior
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core_coverage_test.go (1)
1454-1498: Strengthen isolation assertions to avoid false positives.The gateway-side checks are currently permissive (Line 1478) and swagger separation is only validated on the admin side. Please assert that gateway does not return
200for/debug/pprof/,/metrics, and/swagger/index.htmlwhenAdminPort > 0.Suggested test tightening
func TestInitHTTP_AdminPortSeparation(t *testing.T) { @@ - // Gateway server should NOT serve admin endpoints. + // Gateway server should NOT serve admin endpoints. for _, path := range []string{"/debug/pprof/", "/metrics"} { req := httptest.NewRequest("GET", path, nil) w := httptest.NewRecorder() svr.Handler.ServeHTTP(w, req) - // Gateway handler has no routes for these — expect 404 from the - // gateway mux (grpc-gateway returns its own status for unknown paths). - // The key assertion is that the response does NOT come from pprof/prometheus. - if w.Code == http.StatusOK && (path == "/debug/pprof/" || path == "/metrics") { - body := w.Body.String() - if strings.Contains(body, "pprof") || strings.Contains(body, "go_goroutines") { - t.Errorf("admin endpoint %s should NOT be on gateway server when AdminPort is set", path) - } - } + if w.Code == http.StatusOK { + t.Fatalf("gateway unexpectedly served admin endpoint %s with 200", path) + } } @@ func TestInitHTTP_AdminPortSwagger(t *testing.T) { @@ - _, err := c.initHTTP(context.Background()) + svr, err := c.initHTTP(context.Background()) if err != nil { t.Fatalf("initHTTP failed: %v", err) } @@ if w.Body.String() != "swagger-admin" { t.Fatalf("expected 'swagger-admin' body, got %q", w.Body.String()) } + + // Gateway must not expose swagger when AdminPort is separated. + gwReq := httptest.NewRequest("GET", "/swagger/index.html", nil) + gwW := httptest.NewRecorder() + svr.Handler.ServeHTTP(gwW, gwReq) + if gwW.Code == http.StatusOK { + t.Fatalf("gateway unexpectedly served swagger with 200 in separated mode") + } }Also applies to: 1564-1595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 1454 - 1498, Update TestInitHTTP_AdminPortSeparation to assert the gateway server does NOT return HTTP 200 for admin endpoints by adding `/swagger/index.html` to the checked paths and changing the gateway-side assertion to fail if any of `/debug/pprof/`, `/metrics`, or `/swagger/index.html` return status 200 (rather than only inspecting the body). Locate the gateway check in TestInitHTTP_AdminPortSeparation (after calling c.initHTTP) and replace the permissive body-inspection logic with a strict assert that w.Code != http.StatusOK for those three paths; keep the existing positive assertions against c.adminServer.Handler for the admin server. Apply the same tightening to the duplicate test block mentioned (the similar code around the second test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 1454-1498: Update TestInitHTTP_AdminPortSeparation to assert the
gateway server does NOT return HTTP 200 for admin endpoints by adding
`/swagger/index.html` to the checked paths and changing the gateway-side
assertion to fail if any of `/debug/pprof/`, `/metrics`, or
`/swagger/index.html` return status 200 (rather than only inspecting the body).
Locate the gateway check in TestInitHTTP_AdminPortSeparation (after calling
c.initHTTP) and replace the permissive body-inspection logic with a strict
assert that w.Code != http.StatusOK for those three paths; keep the existing
positive assertions against c.adminServer.Handler for the admin server. Apply
the same tightening to the duplicate test block mentioned (the similar code
around the second test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad8b731d-b6f8-42cc-927f-6b91a4ffe956
📒 Files selected for processing (3)
config/config.gocore.gocore_coverage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clarify AdminPort doc comment: "no dedicated admin server; admin endpoints served on HTTPPort" instead of ambiguous "disabled" - Strengthen admin separation test: check body content unconditionally instead of only on 200 status
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core_coverage_test.go (1)
1471-1479: Strengthen gateway isolation assertion beyond body sniffing.On Line 1477, body-substring checks alone are brittle. Add a status assertion so this test fails loudly if admin routes are accidentally mounted on the gateway handler.
Suggested test hardening
for _, path := range []string{"/debug/pprof/", "/metrics"} { req := httptest.NewRequest("GET", path, nil) w := httptest.NewRecorder() svr.Handler.ServeHTTP(w, req) + if w.Code == http.StatusOK { + t.Errorf("admin endpoint %s should not return 200 on gateway server when AdminPort is set", path) + } // Gateway has no routes for admin paths — must not return pprof/prometheus content. body := w.Body.String() if strings.Contains(body, "pprof") || strings.Contains(body, "go_goroutines") { t.Errorf("admin endpoint %s should NOT be on gateway server when AdminPort is set (got body containing admin content)", path) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core_coverage_test.go` around lines 1471 - 1479, The test currently only checks the response body for admin content; strengthen it by asserting the HTTP status code for each admin path is a non-success (e.g., expect http.StatusNotFound) before/alongside the body checks: in the loop over paths replace or augment the body-sniffing by getting resp := w.Result() and using resp.StatusCode (compare to http.StatusNotFound or assert >=400) and call t.Errorf if the status is 200/2xx; reference the test loop variables and httptest usage (w, req, svr.Handler.ServeHTTP) and import/ensure net/http is available for the StatusNotFound constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core_coverage_test.go`:
- Around line 1581-1585: The test dereferences c.adminServer directly when
calling c.adminServer.Handler.ServeHTTP; add an explicit nil guard that asserts
c.adminServer is non-nil before accessing Handler (e.g., check c.adminServer !=
nil and fail the test with a clear message if it is nil) so the test fails with
a readable assertion instead of panicking; update the test around the lines
using req, w and c.adminServer.Handler.ServeHTTP to first assert/require
c.adminServer != nil, then proceed to call c.adminServer.Handler.ServeHTTP(w,
req).
---
Nitpick comments:
In `@core_coverage_test.go`:
- Around line 1471-1479: The test currently only checks the response body for
admin content; strengthen it by asserting the HTTP status code for each admin
path is a non-success (e.g., expect http.StatusNotFound) before/alongside the
body checks: in the loop over paths replace or augment the body-sniffing by
getting resp := w.Result() and using resp.StatusCode (compare to
http.StatusNotFound or assert >=400) and call t.Errorf if the status is 200/2xx;
reference the test loop variables and httptest usage (w, req,
svr.Handler.ServeHTTP) and import/ensure net/http is available for the
StatusNotFound constant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6795a668-345a-4c1d-a15a-057a577c7e11
📒 Files selected for processing (2)
config/config.gocore_coverage_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add adminServer nil check before handler access in swagger test - Regenerate config/README.md to match updated doc comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ADMIN_PORTconfig (env:ADMIN_PORT, default:0) that serves pprof, metrics, and swagger on a dedicated HTTP serverADMIN_PORT > 0, admin endpoints move to a separate port —HTTP_PORTserves only the gRPC-gatewayADMIN_PORT=0(default), behavior is unchanged — all endpoints shareHTTP_PORTChanges
config/config.go: NewAdminPortfield + validation (range, port conflicts)core.go:adminServerfield oncbstruct, split mux ininitHTTP(), admin server in errgroup, graceful shutdownconfig/config_test.go: 4 validation tests (range, GRPC conflict, HTTP conflict, zero no-warn)core_coverage_test.go: 3 integration tests (separation, combined behavior, swagger on admin)make docTest plan
make testpasses with race detectorAdminPort=0AdminPort > 0Summary by CodeRabbit
New Features
Behavior Changes
Validation
Documentation
Tests