Skip to content

Commit 49a4dfe

Browse files
authored
feat: dedicated admin port for endpoint isolation (#78)
* feat: add dedicated admin port for endpoint isolation 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 * fix: address PR review — runHTTP for admin, combined mode when ports 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 * fix: address second round of review comments - 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 * fix: nil guard in swagger test, regenerate README - Add adminServer nil check before handler access in swagger test - Regenerate config/README.md to match updated doc comment
1 parent b99af5a commit 49a4dfe

6 files changed

Lines changed: 274 additions & 6 deletions

File tree

README.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ func OTELMeterProvider() otelmetric.MeterProvider
137137
OTELMeterProvider returns the global OTel MeterProvider. This is a convenience accessor for code that needs the interface type.
138138

139139
<a name="SetOTELGRPCClientOptions"></a>
140-
## func [SetOTELGRPCClientOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L600>)
140+
## func [SetOTELGRPCClientOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L619>)
141141

142142
```go
143143
func SetOTELGRPCClientOptions(opts ...otelgrpc.Option)
@@ -146,7 +146,7 @@ func SetOTELGRPCClientOptions(opts ...otelgrpc.Option)
146146
Deprecated: Use SetOTELOptions instead. Only applies when OTEL\_USE\_LEGACY\_INSTRUMENTATION=true.
147147

148148
<a name="SetOTELGRPCServerOptions"></a>
149-
## func [SetOTELGRPCServerOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L594>)
149+
## func [SetOTELGRPCServerOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L613>)
150150

151151
```go
152152
func SetOTELGRPCServerOptions(opts ...otelgrpc.Option)
@@ -155,7 +155,7 @@ func SetOTELGRPCServerOptions(opts ...otelgrpc.Option)
155155
Deprecated: Use SetOTELOptions instead. Only applies when OTEL\_USE\_LEGACY\_INSTRUMENTATION=true.
156156

157157
<a name="SetOTELOptions"></a>
158-
## func [SetOTELOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L607>)
158+
## func [SetOTELOptions](<https://github.com/go-coldbrew/core/blob/main/core.go#L626>)
159159

160160
```go
161161
func SetOTELOptions(opts grpcotel.Options)
@@ -314,7 +314,7 @@ type CB interface {
314314
```
315315

316316
<a name="New"></a>
317-
### func [New](<https://github.com/go-coldbrew/core/blob/main/core.go#L920>)
317+
### func [New](<https://github.com/go-coldbrew/core/blob/main/core.go#L956>)
318318

319319
```go
320320
func New(c config.Config) CB

config/README.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ import "github.com/go-coldbrew/core/config"
6666

6767

6868
<a name="Config"></a>
69-
## type [Config](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L12-L198>)
69+
## type [Config](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L12-L203>)
7070

7171
Config is the configuration for the Coldbrew server It is populated from environment variables and has sensible defaults for all fields so that you can just use it as is without any configuration The following environment variables are supported and can be used to override the defaults for the fields
7272

@@ -78,6 +78,11 @@ type Config struct {
7878
GRPCPort int `envconfig:"GRPC_PORT" default:"9090"`
7979
// HTTP Port, defaults to 9091
8080
HTTPPort int `envconfig:"HTTP_PORT" default:"9091"`
81+
// AdminPort is an optional dedicated port for admin endpoints (pprof, metrics, swagger).
82+
// When set to a non-zero value, admin endpoints are served on this port instead of HTTPPort.
83+
// This allows network-level isolation (e.g., Kubernetes NetworkPolicy) to restrict access
84+
// to profiling and metrics data. Default 0 (no dedicated admin server; admin endpoints served on HTTPPort).
85+
AdminPort int `envconfig:"ADMIN_PORT" default:"0"`
8186
// Name of the Application
8287
AppName string `envconfig:"APP_NAME" default:""`
8388
// Environment e.g. Production / Integration / Development
@@ -256,7 +261,7 @@ type Config struct {
256261
```
257262

258263
<a name="Config.Validate"></a>
259-
### func \(Config\) [Validate](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L203>)
264+
### func \(Config\) [Validate](<https://github.com/go-coldbrew/core/blob/main/config/config.go#L208>)
260265

261266
```go
262267
func (c Config) Validate() []string

config/config.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ type Config struct {
1616
GRPCPort int `envconfig:"GRPC_PORT" default:"9090"`
1717
// HTTP Port, defaults to 9091
1818
HTTPPort int `envconfig:"HTTP_PORT" default:"9091"`
19+
// AdminPort is an optional dedicated port for admin endpoints (pprof, metrics, swagger).
20+
// When set to a non-zero value, admin endpoints are served on this port instead of HTTPPort.
21+
// This allows network-level isolation (e.g., Kubernetes NetworkPolicy) to restrict access
22+
// to profiling and metrics data. Default 0 (no dedicated admin server; admin endpoints served on HTTPPort).
23+
AdminPort int `envconfig:"ADMIN_PORT" default:"0"`
1924
// Name of the Application
2025
AppName string `envconfig:"APP_NAME" default:""`
2126
// Environment e.g. Production / Integration / Development
@@ -212,6 +217,17 @@ func (c Config) Validate() []string {
212217
if c.GRPCPort == c.HTTPPort && c.GRPCPort != 0 {
213218
warnings = append(warnings, "GRPCPort and HTTPPort are the same, this will cause a port conflict")
214219
}
220+
if c.AdminPort < 0 || c.AdminPort > 65535 {
221+
warnings = append(warnings, "AdminPort is out of valid range (0-65535)")
222+
}
223+
if c.AdminPort > 0 {
224+
if c.AdminPort == c.GRPCPort {
225+
warnings = append(warnings, "AdminPort and GRPCPort are the same, this will cause a port conflict")
226+
}
227+
if c.AdminPort == c.HTTPPort {
228+
warnings = append(warnings, "AdminPort equals HTTPPort; admin endpoints will be served on HTTPPort (combined mode)")
229+
}
230+
}
215231
if c.NewRelicOpentelemetrySample < 0 || c.NewRelicOpentelemetrySample > 1.0 {
216232
warnings = append(warnings, "NewRelicOpentelemetrySample should be between 0.0 and 1.0")
217233
}

config/config_test.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,74 @@ func TestValidateLogLevel(t *testing.T) {
186186
}
187187
}
188188

189+
func TestValidateAdminPortRange(t *testing.T) {
190+
c := Config{
191+
GRPCPort: 9090,
192+
HTTPPort: 9091,
193+
AdminPort: 70000,
194+
}
195+
warnings := c.Validate()
196+
found := false
197+
for _, w := range warnings {
198+
if strings.Contains(w, "AdminPort") && strings.Contains(w, "range") {
199+
found = true
200+
}
201+
}
202+
if !found {
203+
t.Error("out-of-range AdminPort should produce a warning")
204+
}
205+
}
206+
207+
func TestValidateAdminPortConflictGRPC(t *testing.T) {
208+
c := Config{
209+
GRPCPort: 9090,
210+
HTTPPort: 9091,
211+
AdminPort: 9090,
212+
}
213+
warnings := c.Validate()
214+
found := false
215+
for _, w := range warnings {
216+
if strings.Contains(w, "AdminPort") && strings.Contains(w, "GRPCPort") {
217+
found = true
218+
}
219+
}
220+
if !found {
221+
t.Error("AdminPort == GRPCPort should produce a warning")
222+
}
223+
}
224+
225+
func TestValidateAdminPortConflictHTTP(t *testing.T) {
226+
c := Config{
227+
GRPCPort: 9090,
228+
HTTPPort: 9091,
229+
AdminPort: 9091,
230+
}
231+
warnings := c.Validate()
232+
found := false
233+
for _, w := range warnings {
234+
if strings.Contains(w, "AdminPort") && strings.Contains(w, "HTTPPort") {
235+
found = true
236+
}
237+
}
238+
if !found {
239+
t.Error("AdminPort == HTTPPort should produce a warning")
240+
}
241+
}
242+
243+
func TestValidateAdminPortZeroNoWarning(t *testing.T) {
244+
c := Config{
245+
GRPCPort: 9090,
246+
HTTPPort: 9091,
247+
AdminPort: 0,
248+
}
249+
warnings := c.Validate()
250+
for _, w := range warnings {
251+
if strings.Contains(w, "AdminPort") {
252+
t.Errorf("AdminPort=0 should not produce AdminPort warnings, got: %s", w)
253+
}
254+
}
255+
}
256+
189257
func TestValidateTimeoutExceedsShutdown(t *testing.T) {
190258
c := Config{
191259
GRPCPort: 9090,

core.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ type cb struct {
5959
closers []io.Closer
6060
grpcServer *grpc.Server
6161
httpServer *http.Server
62+
adminServer *http.Server
6263
cancelFunc context.CancelFunc
6364
gracefulWait sync.WaitGroup
6465
creds credentials.TransportCredentials
@@ -550,6 +551,24 @@ func (c *cb) initHTTP(ctx context.Context) (*http.Server, error) {
550551
}
551552
adminMux.Handle(swaggerPattern, http.StripPrefix(swaggerPattern, c.openAPIHandler))
552553
}
554+
if c.config.AdminPort > 0 && c.config.AdminPort != c.config.HTTPPort {
555+
// Separate servers: admin endpoints on AdminPort, gateway on HTTPPort.
556+
adminAddr := fmt.Sprintf("%s:%d", c.config.ListenHost, c.config.AdminPort)
557+
c.adminServer = &http.Server{
558+
Addr: adminAddr,
559+
Handler: adminMux,
560+
}
561+
log.Info(ctx, "msg", "Starting admin server", "address", adminAddr)
562+
563+
gwServer := &http.Server{
564+
Addr: gatewayAddr,
565+
Handler: gzipHandler,
566+
}
567+
log.Info(ctx, "msg", "Starting HTTP gateway server", "address", gatewayAddr)
568+
return gwServer, nil
569+
}
570+
571+
// Combined server: admin + gateway on HTTPPort (default behavior).
553572
adminMux.Handle("/", gzipHandler)
554573
gwServer := &http.Server{
555574
Addr: gatewayAddr,
@@ -817,6 +836,15 @@ func (c *cb) Run() error {
817836
}
818837
return err
819838
})
839+
if c.adminServer != nil {
840+
g.Go(func() error {
841+
err := c.runHTTP(gctx, c.adminServer)
842+
if errors.Is(err, http.ErrServerClosed) {
843+
return nil
844+
}
845+
return err
846+
})
847+
}
820848
// When one server exits with an unexpected error (or parent context is
821849
// cancelled by signal handler), stop the peer so g.Wait() completes.
822850
g.Go(func() error {
@@ -827,6 +855,9 @@ func (c *cb) Run() error {
827855
if c.httpServer != nil {
828856
c.httpServer.Close()
829857
}
858+
if c.adminServer != nil {
859+
c.adminServer.Close()
860+
}
830861
return nil
831862
})
832863
err = g.Wait()
@@ -878,6 +909,11 @@ func (c *cb) Stop(dur time.Duration) error {
878909
log.Info(context.Background(), "msg", "graceful shutdown timer finished", "duration", d)
879910
}
880911
log.Info(context.Background(), "msg", "Server shut down started, bye bye")
912+
if c.adminServer != nil {
913+
if err := c.adminServer.Shutdown(ctx); err != nil {
914+
log.Error(context.Background(), "msg", "admin server shutdown error", "err", err)
915+
}
916+
}
881917
if c.httpServer != nil {
882918
if err := c.httpServer.Shutdown(ctx); err != nil {
883919
log.Error(context.Background(), "msg", "http server shutdown error", "err", err)

core_coverage_test.go

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net/http"
1010
"net/http/httptest"
1111
"os"
12+
"strings"
1213
"testing"
1314
"time"
1415

@@ -1449,3 +1450,145 @@ func TestProcessConfig_NRAutoDisable(t *testing.T) {
14491450
})
14501451
}
14511452
}
1453+
1454+
func TestInitHTTP_AdminPortSeparation(t *testing.T) {
1455+
// removed t.Parallel() — core tests mutate package-level globals
1456+
c := &cb{
1457+
config: config.Config{
1458+
GRPCPort: 19090,
1459+
HTTPPort: 19091,
1460+
AdminPort: 19092,
1461+
ListenHost: "127.0.0.1",
1462+
},
1463+
svc: []CBService{&testService{}},
1464+
}
1465+
svr, err := c.initHTTP(context.Background())
1466+
if err != nil {
1467+
t.Fatalf("initHTTP failed: %v", err)
1468+
}
1469+
1470+
// Gateway server should NOT serve admin endpoints.
1471+
for _, path := range []string{"/debug/pprof/", "/metrics"} {
1472+
req := httptest.NewRequest("GET", path, nil)
1473+
w := httptest.NewRecorder()
1474+
svr.Handler.ServeHTTP(w, req)
1475+
// Gateway has no routes for admin paths — must not return pprof/prometheus content.
1476+
body := w.Body.String()
1477+
if strings.Contains(body, "pprof") || strings.Contains(body, "go_goroutines") {
1478+
t.Errorf("admin endpoint %s should NOT be on gateway server when AdminPort is set (got body containing admin content)", path)
1479+
}
1480+
}
1481+
1482+
// Admin server should serve admin endpoints.
1483+
if c.adminServer == nil {
1484+
t.Fatal("expected adminServer to be set when AdminPort > 0")
1485+
}
1486+
for _, path := range []string{"/debug/pprof/", "/metrics"} {
1487+
req := httptest.NewRequest("GET", path, nil)
1488+
w := httptest.NewRecorder()
1489+
c.adminServer.Handler.ServeHTTP(w, req)
1490+
if w.Code != http.StatusOK {
1491+
t.Fatalf("expected 200 for admin %s, got %d", path, w.Code)
1492+
}
1493+
}
1494+
}
1495+
1496+
func TestInitHTTP_AdminPortZero_CombinedBehavior(t *testing.T) {
1497+
// removed t.Parallel() — core tests mutate package-level globals
1498+
c := &cb{
1499+
config: config.Config{
1500+
GRPCPort: 19090,
1501+
HTTPPort: 19091,
1502+
AdminPort: 0,
1503+
ListenHost: "127.0.0.1",
1504+
},
1505+
svc: []CBService{&testService{}},
1506+
}
1507+
svr, err := c.initHTTP(context.Background())
1508+
if err != nil {
1509+
t.Fatalf("initHTTP failed: %v", err)
1510+
}
1511+
1512+
// Admin server should NOT be created when AdminPort is 0.
1513+
if c.adminServer != nil {
1514+
t.Fatal("expected adminServer to be nil when AdminPort is 0")
1515+
}
1516+
1517+
// Combined server should serve admin endpoints on HTTPPort.
1518+
for _, path := range []string{"/debug/pprof/", "/metrics"} {
1519+
req := httptest.NewRequest("GET", path, nil)
1520+
w := httptest.NewRecorder()
1521+
svr.Handler.ServeHTTP(w, req)
1522+
if w.Code != http.StatusOK {
1523+
t.Fatalf("expected 200 for %s on combined server, got %d", path, w.Code)
1524+
}
1525+
}
1526+
}
1527+
1528+
func TestInitHTTP_AdminPortEqualsHTTPPort_CombinedMode(t *testing.T) {
1529+
// removed t.Parallel() — core tests mutate package-level globals
1530+
c := &cb{
1531+
config: config.Config{
1532+
GRPCPort: 19090,
1533+
HTTPPort: 19091,
1534+
AdminPort: 19091, // same as HTTPPort — should use combined mode
1535+
ListenHost: "127.0.0.1",
1536+
},
1537+
svc: []CBService{&testService{}},
1538+
}
1539+
svr, err := c.initHTTP(context.Background())
1540+
if err != nil {
1541+
t.Fatalf("initHTTP failed: %v", err)
1542+
}
1543+
1544+
// Should NOT create a separate admin server.
1545+
if c.adminServer != nil {
1546+
t.Fatal("expected adminServer to be nil when AdminPort == HTTPPort")
1547+
}
1548+
1549+
// Combined server should serve admin endpoints.
1550+
for _, path := range []string{"/debug/pprof/", "/metrics"} {
1551+
req := httptest.NewRequest("GET", path, nil)
1552+
w := httptest.NewRecorder()
1553+
svr.Handler.ServeHTTP(w, req)
1554+
if w.Code != http.StatusOK {
1555+
t.Fatalf("expected 200 for %s on combined server, got %d", path, w.Code)
1556+
}
1557+
}
1558+
}
1559+
1560+
func TestInitHTTP_AdminPortSwagger(t *testing.T) {
1561+
// removed t.Parallel() — core tests mutate package-level globals
1562+
handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
1563+
w.Write([]byte("swagger-admin"))
1564+
})
1565+
c := &cb{
1566+
config: config.Config{
1567+
GRPCPort: 19090,
1568+
HTTPPort: 19091,
1569+
AdminPort: 19092,
1570+
ListenHost: "127.0.0.1",
1571+
SwaggerURL: "/swagger/",
1572+
},
1573+
svc: []CBService{&testService{}},
1574+
openAPIHandler: handler,
1575+
}
1576+
_, err := c.initHTTP(context.Background())
1577+
if err != nil {
1578+
t.Fatalf("initHTTP failed: %v", err)
1579+
}
1580+
1581+
// Swagger should be on admin server.
1582+
if c.adminServer == nil {
1583+
t.Fatal("expected adminServer to be set when AdminPort > 0")
1584+
}
1585+
req := httptest.NewRequest("GET", "/swagger/index.html", nil)
1586+
w := httptest.NewRecorder()
1587+
c.adminServer.Handler.ServeHTTP(w, req)
1588+
if w.Code != http.StatusOK {
1589+
t.Fatalf("expected 200 for swagger on admin server, got %d", w.Code)
1590+
}
1591+
if w.Body.String() != "swagger-admin" {
1592+
t.Fatalf("expected 'swagger-admin' body, got %q", w.Body.String())
1593+
}
1594+
}

0 commit comments

Comments
 (0)