refactor(entrypoint): move route registry into entrypoint context#200
refactor(entrypoint): move route registry into entrypoint context#200
Conversation
Replace global routes registry with entrypoint-scoped pools and context lookups, and centralize API/metrics startup in config state.
📝 WalkthroughWalkthroughReplaces the global route registry with a context-driven Entrypoint model, adds per-address HTTPServer and route/pool interfaces, moves API/metrics/proxy startup into State methods, introduces context SetCtx/FromCtx helpers (entrypoint, acl, autocert), and rewires handlers/middleware/tests to use Entrypoint from request/task contexts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as "Client"
participant HTTPServer as "Entrypoint HTTPServer"
participant Entrypoint as "Entrypoint"
participant Route as "Route Handler"
participant State as "Config State"
Client->>HTTPServer: HTTP request (Host/Path)
HTTPServer->>Entrypoint: HTTPRoutes().Get / FindRoute (ctx)
Entrypoint->>Entrypoint: GetHealthInfoSimple() (if needed)
Entrypoint->>Route: dispatch to route handler/backend
Route-->>HTTPServer: response
HTTPServer->>Client: HTTP response
Note over State,Entrypoint: State.init -> initEntrypoint -> entrypoint.NewEntrypoint(...)
Note over State: State.StartAPIServers() / State.StartMetrics() manage ancillary servers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // @Failure 500 {object} apitypes.ErrorResponse | ||
| // @Router /health [get] | ||
| func Health(c *gin.Context) { | ||
| ep := entrypoint.FromCtx(c.Request.Context()) |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx returns nil when the request context does not contain an entrypoint value, so calling ep.GetHealthInfoSimple() will panic. The diff doesn’t show any entrypoint.SetCtx usage for request contexts, so this can crash the handler. Consider setting the entrypoint in request context at server/middleware level or guarding against nil here.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (10 files)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/net/gphttp/middleware/forwardauth.go (1)
49-54:⚠️ Potential issue | 🔴 CriticalNil pointer dereference if entrypoint is not in context.
entrypoint.FromCtx(r.Context())returnsnilwhen the context doesn't contain an entrypoint (percontext.go:12-17). Calling.HTTPRoutes()onnilwill panic.🐛 Proposed fix to add nil check
func (m *forwardAuthMiddleware) before(w http.ResponseWriter, r *http.Request) (proceed bool) { + ep := entrypoint.FromCtx(r.Context()) + if ep == nil { + ForwardAuth.LogError(r).Msg("entrypoint not found in request context") + w.WriteHeader(http.StatusInternalServerError) + return false + } + route, ok := ep.HTTPRoutes().Get(m.Route) - route, ok := entrypoint.FromCtx(r.Context()).HTTPRoutes().Get(m.Route) if !ok { ForwardAuth.LogWarn(r).Str("route", m.Route).Msg("forwardauth route not found") w.WriteHeader(http.StatusInternalServerError) return false }internal/api/v1/docker/stats.go (1)
46-57:⚠️ Potential issue | 🟠 MajorMissing nil check for entrypoint from context.
The inline call
entrypoint.FromCtx(c.Request.Context()).GetRoute(id)will panic ifFromCtxreturnsnil.🛡️ Proposed fix to add nil check
dockerCfg, ok := docker.GetDockerCfgByContainerID(id) if !ok { var route types.Route - route, ok = entrypoint.FromCtx(c.Request.Context()).GetRoute(id) + ep := entrypoint.FromCtx(c.Request.Context()) + if ep == nil { + c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not initialized")) + return + } + route, ok = ep.GetRoute(id) if ok {internal/idlewatcher/watcher.go (1)
175-181:⚠️ Potential issue | 🟠 MajorMissing nil check for entrypoint from context.
The call
entrypoint.FromCtx(parent.Context()).GetRoute(dep)will panic if the entrypoint is not in the context. This is particularly critical since it's in theNewWatcherconstructor path.🛡️ Proposed fix to add nil check
if !ok { - depRoute, ok = entrypoint.FromCtx(parent.Context()).GetRoute(dep) + ep := entrypoint.FromCtx(parent.Context()) + if ep != nil { + depRoute, ok = ep.GetRoute(dep) + } if !ok { depErrors.Addf("dependency %q not found", dep) continue } }internal/route/common.go (1)
14-27:⚠️ Potential issue | 🟠 MajorHandle missing entrypoint in context to avoid panic.
entrypoint.FromCtx(ctx)can return nil; callingHTTPRoutes()/StreamRoutes()would panic. Return a clear error instead.🔧 Suggested guard
func checkExists(ctx context.Context, r types.Route) gperr.Error { if r.UseLoadBalance() { // skip checking for load balanced routes return nil } + ep := entrypoint.FromCtx(ctx) + if ep == nil { + return gperr.Errorf("entrypoint missing in context") + } var ( existing types.Route ok bool ) switch r := r.(type) { case types.HTTPRoute: - existing, ok = entrypoint.FromCtx(ctx).HTTPRoutes().Get(r.Key()) + existing, ok = ep.HTTPRoutes().Get(r.Key()) case types.StreamRoute: - existing, ok = entrypoint.FromCtx(ctx).StreamRoutes().Get(r.Key()) + existing, ok = ep.StreamRoutes().Get(r.Key()) }internal/net/gphttp/middleware/crowdsec.go (1)
170-185:⚠️ Potential issue | 🟠 MajorGuard against nil entrypoint before calling GetRoute.
The
entrypoint.FromCtx()function can returnnilif the entrypoint is not in the request context, and calling.GetRoute()on a nil interface will panic. Add a defensive check before method invocation.However, the suggested fix's error handling logic is convoluted. Since the original code falls back to IP address handling when a route is not found, the nil entrypoint case should follow the same fallback pattern:
func (m *crowdsecMiddleware) buildCrowdSecURL(ctx context.Context) (string, error) { // Try to get route first if m.Route != "" { - if route, ok := entrypoint.FromCtx(ctx).GetRoute(m.Route); ok { + ep := entrypoint.FromCtx(ctx) + if ep != nil { + if route, ok := ep.GetRoute(m.Route); ok { + // Using route name + targetURL := *route.TargetURL() + targetURL.Path = m.Endpoint + return targetURL.String(), nil + } + } + + // If not found in routes, assume it's an IP address + if m.Port == 0 { + return "", fmt.Errorf("port must be specified when using IP address") + } + return fmt.Sprintf("http://%s%s", net.JoinHostPort(m.Route, strconv.Itoa(m.Port)), m.Endpoint), nil - // Using route name - targetURL := *route.TargetURL() - targetURL.Path = m.Endpoint - return targetURL.String(), nil - } - // If not found in routes, assume it's an IP address - if m.Port == 0 { - return "", fmt.Errorf("port must be specified when using IP address") - } - return fmt.Sprintf("http://%s%s", net.JoinHostPort(m.Route, strconv.Itoa(m.Port)), m.Endpoint), nil }
🤖 Fix all issues with AI agents
In `@internal/api/v1/favicon.go`:
- Around line 76-77: entrypoint.FromCtx(ctx) can return nil, so guard against
that before calling ep.HTTPRoutes(); update the code around the call to
entrypoint.FromCtx(ctx) (variable ep) to check if ep == nil and handle it the
same way you handle a missing route (e.g., treat as not found or return an
appropriate HTTP error) instead of calling ep.HTTPRoutes() when nil; ensure the
subsequent logic that uses r, ok (from ep.HTTPRoutes().Get(alias)) still behaves
correctly when ep is nil.
In `@internal/api/v1/health.go`:
- Around line 27-34: entrypoint.FromCtx(c.Request.Context()) can return nil; add
a nil check after calling entrypoint.FromCtx (ep := entrypoint.FromCtx(...)) and
handle the error path before calling ep.GetHealthInfoSimple: if ep is nil,
return an appropriate error response (e.g., use apitypes error response) for
both websocket and non-websocket branches instead of calling
websocket.PeriodicWrite or c.JSON with ep.GetHealthInfoSimple; also add the
suggested import apitypes "github.com/yusing/goutils/apitypes" to the file.
In `@internal/api/v1/homepage/categories.go`:
- Around line 23-25: The Categories handler currently calls
entrypoint.FromCtx(c.Request.Context()) and passes the result (ep) to
HomepageCategories without checking for nil; add a defensive nil check after
calling entrypoint.FromCtx in Categories and if ep == nil return an HTTP error
response via c.JSON (e.g., http.StatusInternalServerError or
http.StatusBadRequest with a clear JSON error message), otherwise proceed to
call HomepageCategories(ep) as before; update only the Categories function and
reference the ep variable, entrypoint.FromCtx, HomepageCategories, and c.JSON
when making the change.
In `@internal/api/v1/route/by_provider.go`:
- Around line 27-28: entrypoint.FromCtx(c.Request.Context()) can return nil so
add a nil check after calling entrypoint.FromCtx(...) and return an appropriate
JSON error response instead of calling ep.RoutesByProvider() when ep is nil;
update imports to use apitypes (remove blank import) and respond with a non-200
status (e.g., http.StatusInternalServerError) and an apitypes error payload,
otherwise call ep.RoutesByProvider() and return http.StatusOK as before.
In `@internal/api/v1/route/route.go`:
- Around line 35-36: Guard against a missing entrypoint before calling
ep.GetRoute: check the result of entrypoint.FromCtx(c.Request.Context()) (the
variable ep) for nil/unavailable and if it is missing, write a 5xx response and
abort handling instead of calling ep.GetRoute; update the code around ep :=
entrypoint.FromCtx(...) to return an HTTP 5xx (e.g. via c.AbortWithStatusJSON or
c.JSON + return) with a short error message so the handler does not panic.
In `@internal/api/v1/route/routes.go`:
- Around line 35-45: Routes and RoutesWS call
entrypoint.FromCtx(c.Request.Context()) and then dereference ep (ep.IterRoutes,
ep.NumRoutes) without a nil check; add a guard immediately after ep :=
entrypoint.FromCtx(...) that handles the nil case (e.g., return an appropriate
HTTP response such as empty list or error JSON and c.Abort/return) to avoid
panics, and ensure both handlers (Routes and RoutesWS) use this check before
accessing ep.IterRoutes or ep.NumRoutes.
In `@internal/entrypoint/entrypoint_test.go`:
- Around line 24-43: The test helper run currently constructs its own Entrypoint
via NewEntrypoint (ignoring routes added by callers), so change run to accept an
existing *Entrypoint (or interface) parameter instead of creating one
internally; update the function signature run(t *testing.T, ep *Entrypoint,
match []string, noMatch []string) (or appropriate type) and remove the
NewEntrypoint call, then update all test call sites (e.g.,
TestFindRouteAnyDomain and others that create an ep and call run) to pass their
ep; keep t.Helper() and continue using ep.HTTPRoutes().Get(...) and the same
expect assertions.
In `@internal/entrypoint/http_pool_adapter.go`:
- Around line 3-49: The current httpPoolAdapter implementation (methods Iter,
Get, Size) skips the ProxyHTTPSAddr server entirely which can hide routes when
the HTTP server is absent; change these methods to dedupe by alias instead: in
Iter (httpPoolAdapter.Iter) keep a local seen map[string]bool and only call
yield once per alias while iterating all h.ep.servers.Range and each
srv.routes.Iter; in Get (httpPoolAdapter.Get) search all servers (don't
unconditionally skip ProxyHTTPSAddr) and return the first matching
srv.routes.Get(alias); in Size (httpPoolAdapter.Size) iterate all servers and
aggregate unique aliases into a seen set and return its length (or count only
unseen aliases while iterating) so default HTTPS routes aren’t lost when HTTP is
missing.
In `@internal/entrypoint/routes.go`:
- Around line 55-62: The code calls ep.AddHTTPRoute(r) but ignores its error,
then proceeds to register the route in ep.shortLinkMatcher and attach cancel
hooks; instead call AddHTTPRoute(r) and check its returned error immediately —
if non-nil, do not call ep.shortLinkMatcher.AddRoute(r.Key()) or
r.Task().OnCancel(...) and return/abort early; if AddHTTPRoute succeeds then
proceed to AddRoute and set the OnCancel hook that calls ep.delHTTPRoute and
shortLinkMatcher.DelRoute. Ensure you reference and handle the error from
AddHTTPRoute, and only register the matcher and cancel callback when
AddHTTPRoute has succeeded.
- Around line 104-110: delHTTPRoute only checks route.ListenURL().Host so it
fails to remove routes that were registered under the default HTTP/HTTPS servers
when port was empty/0; update Entrypoint.delHTTPRoute to mirror AddHTTPRoute by:
compute addr := route.ListenURL().Host and if addr is empty or port == "0" also
attempt to load and call DelRoute on both default addresses (the default HTTP
and HTTPS server keys used in AddHTTPRoute) in addition to the specific addr;
keep the existing srv, _ := ep.servers.Load(addr) lookup and nil check and call
srv.DelRoute(route) for each candidate server, ensuring you attempt both
defaults when port is empty/0.
In `@internal/metrics/period/poller.go`:
- Around line 156-157: The Start method for Poller (Poller.Start(parent
task.Parent)) currently calls parent.Subtask without validating parent and there
is a callsite in agent/cmd/main.go invoking Poller.Start() with no arguments;
fix both by updating the callsite to pass a valid task.Parent (e.g., call
systeminfo.Poller.Start(task.RootTask("metrics", false)) or another appropriate
parent) and add a defensive nil-guard in Poller.Start that checks if parent ==
nil, logs a descriptive error via the poller's logger or task logger, and
returns early instead of calling parent.Subtask; reference Poller.Start,
task.Parent, parent.Subtask and the call in agent/cmd/main.go when applying the
changes.
In `@internal/metrics/uptime/uptime.go`:
- Around line 43-47: Guard against a missing entrypoint in getStatuses by
checking entrypoint.FromCtx(ctx) before calling GetHealthInfoWithoutDetail: if
FromCtx(ctx) is nil return a StatusByAlias with an empty Map (or map indicating
unknown status) and Timestamp time.Now().Unix() instead of calling methods on a
nil pointer; apply the same nil-check and fallback to empty/unknown status for
any other uptime aggregation functions that call entrypoint.FromCtx (the similar
code block referenced around the other aggregation function).
In `@internal/route/fileserver.go`:
- Line 129: entrypoint.FromCtx(parent.Context()) can return nil so calling
AddRoute(s) directly may panic; retrieve the entrypoint into a local (e.g., ep
:= entrypoint.FromCtx(parent.Context())), check if ep == nil and handle the nil
case (return/skip/log/error) before calling ep.AddRoute(s), ensuring AddRoute is
only invoked on a non-nil entrypoint.
In `@internal/route/reverse_proxy.go`:
- Around line 166-170: The calls to entrypoint.FromCtx(parent.Context()) can
return nil and currently are dereferenced (calling AddRoute) which will panic;
change both places (the block that calls r.addToLoadBalancer(parent) vs
entrypoint.FromCtx(...) and the similar block at lines around 186-188) to first
assign the result to a variable (e := entrypoint.FromCtx(parent.Context())),
check if e == nil and if so log a clear error and return (or return an error)
instead of proceeding, otherwise call e.AddRoute(r); keep the load-balancer path
(r.addToLoadBalancer(parent)) unchanged.
In `@internal/route/route.go`:
- Around line 519-524: The code calls entrypoint.FromCtx(parent.Context()) and
immediately uses ep.ExcludedRoutes(), which can panic if entrypoint.FromCtx
returns nil; update the block around ep := entrypoint.FromCtx(parent.Context())
(used with r.task, parent.Subtask, r.impl, and r.task.OnCancel) to check for nil
before calling ep.ExcludedRoutes(): if ep is nil, skip adding to or removing
from ExcludedRoutes and avoid registering the OnCancel removal closure that
references ep; ensure the absence path still sets up r.task via parent.Subtask
but does not dereference ep.
In `@internal/route/rules/do.go`:
- Around line 196-216: The route command's build function (build in the route
rule) currently returns a no-op TerminatingCommand and so fails silently; update
build/TerminatingCommand to either (preferred) return a handler that writes a
clear HTTP error (e.g., http.Error with 500 or 501 and a message like "route
command disabled: circular dependency" plus the requested alias) so failures are
visible at runtime, or if you prefer to block earlier change the command
validation to reject/disable the "route <alias>" invocation; locate the build
closure and TerminatingCommand return and replace the commented-out routing
logic with a determinist error response or add validation logic that prevents
creation of the route command until entrypoint circular dependency is resolved.
In `@internal/route/stream.go`:
- Line 85: Add a nil-guard around the entrypoint obtained from
entrypoint.FromCtx(parent.Context()) before calling AddRoute to avoid panics in
misconfigured/test contexts: call entrypoint.FromCtx(parent.Context()), check if
the returned value is non-nil, and only then invoke its AddRoute method (which
registers OnCancel callbacks). Apply the same defensive check in the other
occurrences in reverse_proxy.go and fileserver.go to ensure robust route cleanup
without dereferencing a nil entrypoint.
In `@internal/route/stream/tcp_tcp.go`:
- Around line 61-63: Guard the call to entrypoint.FromCtx(ctx) before accessing
Config(): assign the result to a local (e.g., entry := entrypoint.FromCtx(ctx))
and check entry != nil (and optionally entry.Config() != nil) before reading
SupportProxyProtocol; only wrap s.listener in &proxyproto.Listener{...} when the
guard passes so you avoid a nil deref in the block that currently calls
entrypoint.FromCtx(ctx).Config().SupportProxyProtocol.
In `@internal/types/health.go`:
- Around line 77-89: The HealthInfoWithoutDetail and HealthInfo structs use
time.Duration but are missing custom JSON marshaling, causing default nanosecond
serialization; implement MarshalJSON on HealthInfoWithoutDetail to build a JSON
object converting Uptime to milliseconds and Latency to microseconds (matching
the documented units), and implement MarshalJSON on HealthInfo to include the
embedded HealthInfoWithoutDetail marshaled representation plus the Detail field;
ensure both methods produce the same JSON field names
("status","uptime","latency") and use the same numeric types as HealthJSONRepr's
conversions (milliseconds for uptime, microseconds for latency).
🟡 Minor comments (5)
internal/route/route.go-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorRemove the
divevalidator tag from theBindfield.The
divevalidator is designed for traversing elements within slices, maps, or arrays. SinceBindis a plainstringtype, this tag is incorrectly applied and serves no purpose. All other uses ofdivein the codebase are correctly applied to collection types. For validating thatBindis a valid IP address when provided, use only:-Bind string `json:"bind,omitempty" validate:"omitempty,dive,ip_addr" extensions:"x-nullable"` +Bind string `json:"bind,omitempty" validate:"omitempty,ip_addr" extensions:"x-nullable"`internal/entrypoint/entrypoint_benchmark_test.go-92-107 (1)
92-107:⚠️ Potential issue | 🟡 MinorInitialize and bind an entrypoint before starting routes in benchmarks.
r.Start(...)now relies onentrypoint.FromCtxfor registry checks and server creation. Withtask.NewTestTask/task.RootTaskalone, no entrypoint is stored in the context, so route start may panic orep.GetServer/ep.HTTPRoutesremain empty. Please create an entrypoint (use the same constructor as production/tests) and set it into the task context before callingStart.Also applies to: 147-163
internal/entrypoint/shortlink_test.go-167-170 (1)
167-170:⚠️ Potential issue | 🟡 MinorAvoid binding a fixed port and clean up the test server
Using a hard-coded port can collide with other tests/CI, and the listener is never closed. Prefer an ephemeral port and register cleanup to close the server/listener after the test.
💡 Suggested tweak
- err := server.Listen("localhost:8080", HTTPProtoHTTP) + err := server.Listen("127.0.0.1:0", HTTPProtoHTTP)internal/entrypoint/http_server.go-90-92 (1)
90-92:⚠️ Potential issue | 🟡 MinorEnsure
stopFuncis non-nil before calling.If
Close()is called beforeListen(),srv.stopFuncwill be nil, causing a panic. Consider adding a nil guard.🛡️ Proposed defensive fix
func (srv *httpServer) Close() { + if srv.stopFunc != nil { srv.stopFunc(nil) + } }internal/entrypoint/http_server.go-161-167 (1)
161-167:⚠️ Potential issue | 🟡 MinorHeaders must be set before
WriteHeader.
w.Header().Set(...)on line 164 has no effect becausew.WriteHeader(...)was already called on line 163. HTTP headers must be set before writing the status code.🐛 Proposed fix
errorPage, ok := errorpage.GetErrorPageByStatus(http.StatusNotFound) if ok { - w.WriteHeader(http.StatusNotFound) w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusNotFound) if _, err := w.Write(errorPage); err != nil { log.Err(err).Msg("failed to write error page") }
🧹 Nitpick comments (5)
internal/route/route.go (1)
276-316: Consider consolidating FileServer URL handling to avoid redundant construction.
ProxyURLis constructed at line 282 forSchemeFileServer, then immediately overwritten at lines 312-316. This works correctly but is slightly inefficient.Optional: consolidate FileServer handling
if r.ShouldExclude() { r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("%s://%s", r.Scheme, net.JoinHostPort(r.Host, strconv.Itoa(r.Port.Proxy)))) } else { switch r.Scheme { case route.SchemeFileServer: r.LisURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("https://%s", net.JoinHostPort(r.Bind, strconv.Itoa(r.Port.Listening)))) - r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("%s://%s", r.Scheme, net.JoinHostPort(r.Host, strconv.Itoa(r.Port.Proxy)))) + r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, "file://"+r.Root) + r.Host = "" + r.Port.Proxy = 0 case route.SchemeHTTP, route.SchemeHTTPS, route.SchemeH2C: // ... unchanged } } - - if r.Scheme == route.SchemeFileServer { - r.Host = "" - r.Port.Proxy = 0 - r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, "file://"+r.Root) - }internal/route/stream/udp_udp.go (1)
85-89: Track the TODO for entrypoint integration.The ACL guard is good. Please capture the TODO as an issue so it doesn’t get lost; I can help wire entrypoint registration here if you want.
internal/entrypoint/query.go (2)
43-49: Consider pre-sizing the map if provider count is predictable.The map is created without capacity hints. If the number of providers is typically small and stable, this is fine. However, if there's a known upper bound or typical count available, pre-sizing could reduce allocations.
51-91: Optional: Consolidate helper functions to reduce duplication.The three helper functions share identical nil-check logic and HealthMonitor retrieval. Consider extracting a common pattern if this code grows.
♻️ Example consolidation using a generic approach
func getHealthData[T any](r types.Route, withMonitor func(types.HealthMonitor) T, withoutMonitor T) T { mon := r.HealthMonitor() if mon == nil { return withoutMonitor } return withMonitor(mon) }Then each helper becomes a one-liner calling
getHealthDatawith appropriate transformers.internal/entrypoint/entrypoint.go (1)
21-25: Consider documenting the HTTPRoutes interface.This interface is central to the new routing architecture. A brief doc comment explaining its role and relationship to the entrypoint would aid maintainability.
📝 Suggested documentation
+// HTTPRoutes provides access to HTTP route lookups. +// Implementations include the HTTP pool adapter which aggregates routes +// across all HTTP servers managed by the entrypoint. type HTTPRoutes interface { Get(alias string) (types.HTTPRoute, bool) }
|
|
||
| HealthInfoWithoutDetail struct { | ||
| Status HealthStatus `json:"status" swaggertype:"string" enums:"healthy,unhealthy,napping,starting,error,unknown"` | ||
| Uptime time.Duration `json:"uptime" swaggertype:"number"` // uptime in milliseconds | ||
| Latency time.Duration `json:"latency" swaggertype:"number"` // latency in microseconds | ||
| } // @name HealthInfoWithoutDetail | ||
|
|
||
| HealthInfo struct { | ||
| HealthInfoWithoutDetail | ||
| Detail string `json:"detail"` | ||
| } // @name HealthInfo | ||
|
|
||
| HealthMap = map[string]HealthStatusString // @name HealthMap |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the complete health.go file to understand the context
wc -l internal/types/health.go
echo "---"
# View the full file to understand the structure
cat -n internal/types/health.goRepository: yusing/godoxy
Length of output: 7127
🏁 Script executed:
# Search for MarshalJSON methods in health.go
rg "MarshalJSON" internal/types/health.go -A 10Repository: yusing/godoxy
Length of output: 662
🏁 Script executed:
# Search for how HealthInfo/HealthInfoWithoutDetail are used
rg "HealthInfo|HealthInfoWithoutDetail" --type=go -B2 -A2 | head -100Repository: yusing/godoxy
Length of output: 5778
🏁 Script executed:
# Check the HealthJSON implementation mentioned in the comment
rg "HealthJSON" internal/types/health.go -B5 -A10Repository: yusing/godoxy
Length of output: 1632
🏁 Script executed:
#!/bin/bash
# Check the implementation of getHealthInfo and getHealthInfoWithoutDetail functions
rg "func getHealthInfo" internal/entrypoint/query.go -A 15Repository: yusing/godoxy
Length of output: 954
🏁 Script executed:
# Check how StatusByAlias (which uses HealthInfoWithoutDetail) is used
rg "StatusByAlias" internal/metrics/uptime/ -B2 -A5Repository: yusing/godoxy
Length of output: 4707
🏁 Script executed:
# Check if there are any custom marshalers for HealthInfo/HealthInfoWithoutDetail
rg "MarshalJSON.*HealthInfo" --type=goRepository: yusing/godoxy
Length of output: 39
Add MarshalJSON methods to HealthInfoWithoutDetail and HealthInfo for correct unit serialization.
These structs use time.Duration fields but currently lack custom JSON marshaling. Go's default encoder serializes time.Duration as nanoseconds, contradicting the comments (uptime: milliseconds, latency: microseconds). Add MarshalJSON methods to convert to the documented units, consistent with how HealthJSONRepr handles the conversion at lines 199-200 (Uptime.Seconds(), Latency.Milliseconds()).
🤖 Prompt for AI Agents
In `@internal/types/health.go` around lines 77 - 89, The HealthInfoWithoutDetail
and HealthInfo structs use time.Duration but are missing custom JSON marshaling,
causing default nanosecond serialization; implement MarshalJSON on
HealthInfoWithoutDetail to build a JSON object converting Uptime to milliseconds
and Latency to microseconds (matching the documented units), and implement
MarshalJSON on HealthInfo to include the embedded HealthInfoWithoutDetail
marshaled representation plus the Detail field; ensure both methods produce the
same JSON field names ("status","uptime","latency") and use the same numeric
types as HealthJSONRepr's conversions (milliseconds for uptime, microseconds for
latency).
…ndling - Introduced `NewTestRoute` function to simplify route creation in benchmark tests. - Replaced direct route validation and starting with error handling using `require.NoError`. - Updated server retrieval to use `common.ProxyHTTPAddr` for consistency. - Improved logging for HTTP route addition errors in `AddRoute` method.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent/cmd/main.go (2)
145-166:⚠️ Potential issue | 🔴 Critical
systeminfo.Poller.Start(t)is unreachable when the socket proxy is enabled.
srv.Serve(l)on line 161 is a blocking call. Whensocketproxy.ListenAddr != "", execution never reaches line 164, so the system-info poller will never start. The AI summary confirms the poller was previously started earlier in a parallel startup sequence, so this move introduced a regression.Move the
Poller.Start(t)call before the socket proxy block (e.g., before line 145), or run the socket-proxy server in a goroutine like the other servers.Proposed fix (move poller start before blocking section)
+ systeminfo.Poller.Start(t) + if socketproxy.ListenAddr != "" { runtime := strutils.Title(string(env.Runtime)) @@ -161,3 +163,1 @@ srv.Serve(l) } - systeminfo.Poller.Start(t) task.WaitExit(3)
48-51:⚠️ Potential issue | 🟠 MajorPre-existing bug: error from
srv.Loadis silently discarded.Line 48 discards the return value of
srv.Load(env.AgentSSLCert). Theerrchecked on line 49 is the stale value from line 42 (ca.ToTLSCert()). If the SSL cert fails to load, the error goes unnoticed.Since you're already touching this file, consider fixing this:
Proposed fix
- srv.Load(env.AgentSSLCert) + err = srv.Load(env.AgentSSLCert)
🤖 Fix all issues with AI agents
In `@internal/route/reverse_proxy.go`:
- Around line 166-169: The early-return in Start when
entrypoint.FromCtx(parent.Context()) yields nil omits calling r.task.Finish(err)
and thus leaks the subtask; update the nil-check branch in Start (where ep is
obtained from entrypoint.FromCtx and ep==nil) to call r.task.Finish(err) with
the created error before returning (i.e., create the gperr.New error into a
variable, call r.task.Finish(err) and then return that err) so the subtask
created at the earlier r.task initialization is always finished.
In `@internal/route/route.go`:
- Around line 948-962: The default assignment to r.Port.Proxy inside the
non-TCP/UDP branch is dead because r.Port.Proxy is later overwritten by pp;
instead set the local proxy-port variable pp to the default
(common.ProxyHTTPSPort) when r.Port.Proxy is zero so that the final assignment
r.Port.Listening, r.Port.Proxy = lp, pp preserves the intended default; update
the branch that currently does `if r.Port.Proxy == 0 { r.Port.Proxy =
common.ProxyHTTPSPort }` to assign to pp, leaving references to r.Scheme, pp,
lp, r.Port.Proxy and common.ProxyHTTPSPort to locate the change.
In `@internal/route/stream/tcp_tcp.go`:
- Around line 61-65: Replace the redundant call to entrypoint.FromCtx(ctx)
inside the nil-guard with the already-cached variable ep: after verifying ep !=
nil, call ep.SupportProxyProtocol() rather than
entrypoint.FromCtx(ctx).SupportProxyProtocol(), and wrap s.listener with
proxyproto.Listener only when ep.SupportProxyProtocol() returns true (preserving
the existing behavior for s.listener).
🧹 Nitpick comments (6)
internal/route/reverse_proxy.go (1)
159-163: Health monitor failure is now silently downgraded to a warning.Previously, a
HealthMon.Starterror would propagate and fail route startup. Now it's logged and the monitor is set tonil, meaning the route will run without any health checking. This is a deliberate design choice per the PR, but it means misconfigured health checks (wrong port, bad path, etc.) won't surface as startup errors — they'll silently disappear.Consider whether this should at least be a visible warning in the route's status or homepage metadata so operators can notice that health monitoring is unexpectedly absent.
internal/entrypoint/entrypoint_benchmark_test.go (1)
52-61:taskvariable shadows thetaskpackage import.
task := task.NewTestTask(b)on line 53 shadows the importedtaskpackage. This works here because the package is only needed forNewTestTask, but it can be confusing. Consider renaming the variable (e.g.,testTask) for clarity.♻️ Optional rename for clarity (applies to both benchmarks)
- task := task.NewTestTask(b) - ep := NewEntrypoint(task, nil) + testTask := task.NewTestTask(b) + ep := NewEntrypoint(testTask, nil) ... - entrypoint.SetCtx(task, ep) + entrypoint.SetCtx(testTask, ep) ... - r, err := route.NewTestRoute(b, task, &route.Route{ + r, err := route.NewTestRoute(b, testTask, &route.Route{internal/route/route.go (1)
280-320: FileServer ProxyURL is set twice for non-excluded routes.When
Scheme == SchemeFileServerand!ShouldExclude(),ProxyURLis first built at line 286 (e.g.,fileserver://host:port), then overwritten at line 319 (file://root). The first assignment is wasted work. Additionally, for excluded FileServer routes, lines 316-320 still run and clearHost/Port.Proxy, which may be unintended side effects on already-computed data.Consider moving the FileServer block (lines 316-320) into the
case route.SchemeFileServerbranch so it's only executed once and only in the correct path.Proposed refactor
if r.ShouldExclude() { r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("%s://%s", r.Scheme, net.JoinHostPort(r.Host, strconv.Itoa(r.Port.Proxy)))) } else { switch r.Scheme { case route.SchemeFileServer: - r.LisURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("https://%s", net.JoinHostPort(r.Bind, strconv.Itoa(r.Port.Listening)))) - r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("%s://%s", r.Scheme, net.JoinHostPort(r.Host, strconv.Itoa(r.Port.Proxy)))) + r.LisURL = gperr.Collect(&errs, nettypes.ParseURL, fmt.Sprintf("https://%s", net.JoinHostPort(r.Bind, strconv.Itoa(r.Port.Listening)))) + r.Host = "" + r.Port.Proxy = 0 + r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, "file://"+r.Root) case route.SchemeHTTP, route.SchemeHTTPS, route.SchemeH2C: ... } } - - if r.Scheme == route.SchemeFileServer { - r.Host = "" - r.Port.Proxy = 0 - r.ProxyURL = gperr.Collect(&errs, nettypes.ParseURL, "file://"+r.Root) - }internal/api/v1/health.go (1)
8-13: Redundant blank import ofapitypes.Line 9 imports
apitypes "github.com/yusing/goutils/apitypes"(used at line 30), so the blank import at line 13 (_ "github.com/yusing/goutils/apitypes") is unnecessary. If it existed before for swagger side-effects, the named import already ensures the package is linked.Proposed fix
apitypes "github.com/yusing/goutils/apitypes" "github.com/yusing/goutils/http/httpheaders" "github.com/yusing/goutils/http/websocket" - - _ "github.com/yusing/goutils/apitypes" )internal/api/v1/homepage/categories.go (1)
10-11: Redundant blank import ofapitypes.Same as in
health.go— line 11 already imports the package with theapitypesalias, so the blank import at line 10 is unnecessary.Proposed fix
- _ "github.com/yusing/goutils/apitypes" apitypes "github.com/yusing/goutils/apitypes"internal/config/types/config.go (1)
11-11: Redundant import alias.The package name is already
entrypoint, so the explicit alias can be dropped.Suggested fix
- entrypoint "github.com/yusing/godoxy/internal/entrypoint" + "github.com/yusing/godoxy/internal/entrypoint"
| switch r.Scheme { | ||
| case route.SchemeTCP, route.SchemeUDP: | ||
| if r.Bind == "" { | ||
| r.Bind = "0.0.0.0" | ||
| } | ||
| default: | ||
| if r.Bind == "" { | ||
| r.Bind = common.ProxyHTTPSHost | ||
| } | ||
| if r.Port.Proxy == 0 { | ||
| r.Port.Proxy = common.ProxyHTTPSPort | ||
| } | ||
| } | ||
|
|
||
| r.Port.Listening, r.Port.Proxy = lp, pp |
There was a problem hiding this comment.
Bug: r.Port.Proxy default at line 958 is dead code — overwritten at line 962.
Lines 957-959 set r.Port.Proxy = common.ProxyHTTPSPort, but line 962 unconditionally overwrites r.Port.Proxy with the local variable pp (captured at line 877). The new default is never observed. This likely should update pp instead of r.Port.Proxy.
Proposed fix
default:
if r.Bind == "" {
r.Bind = common.ProxyHTTPSHost
}
- if r.Port.Proxy == 0 {
- r.Port.Proxy = common.ProxyHTTPSPort
+ if pp == 0 {
+ pp = common.ProxyHTTPSPort
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch r.Scheme { | |
| case route.SchemeTCP, route.SchemeUDP: | |
| if r.Bind == "" { | |
| r.Bind = "0.0.0.0" | |
| } | |
| default: | |
| if r.Bind == "" { | |
| r.Bind = common.ProxyHTTPSHost | |
| } | |
| if r.Port.Proxy == 0 { | |
| r.Port.Proxy = common.ProxyHTTPSPort | |
| } | |
| } | |
| r.Port.Listening, r.Port.Proxy = lp, pp | |
| switch r.Scheme { | |
| case route.SchemeTCP, route.SchemeUDP: | |
| if r.Bind == "" { | |
| r.Bind = "0.0.0.0" | |
| } | |
| default: | |
| if r.Bind == "" { | |
| r.Bind = common.ProxyHTTPSHost | |
| } | |
| if pp == 0 { | |
| pp = common.ProxyHTTPSPort | |
| } | |
| } | |
| r.Port.Listening, r.Port.Proxy = lp, pp |
🤖 Prompt for AI Agents
In `@internal/route/route.go` around lines 948 - 962, The default assignment to
r.Port.Proxy inside the non-TCP/UDP branch is dead because r.Port.Proxy is later
overwritten by pp; instead set the local proxy-port variable pp to the default
(common.ProxyHTTPSPort) when r.Port.Proxy is zero so that the final assignment
r.Port.Listening, r.Port.Proxy = lp, pp preserves the intended default; update
the branch that currently does `if r.Port.Proxy == 0 { r.Port.Proxy =
common.ProxyHTTPSPort }` to assign to pp, leaving references to r.Scheme, pp,
lp, r.Port.Proxy and common.ProxyHTTPSPort to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/v1/health.go (1)
14-24:⚠️ Potential issue | 🟡 MinorUpdate swagger annotation to match actual return type of
GetHealthInfoSimple().The annotation on line 21 references
routes.HealthMap, but the routes package doesn't define this type and isn't imported. The actual return type fromGetHealthInfoSimple()ismap[string]types.HealthStatus(wheretypesisinternal/types). Update the annotation to correctly reference the returned type, or use a compatible swagger representation.internal/route/reverse_proxy.go (1)
196-202:⚠️ Potential issue | 🟡 MinorPotential data race on
linked.Homepageafter releasinglbLock.After
lbLock.Unlock()at line 196, multiple goroutines can concurrently read/writelinked.Homepage.Nameandlinked.Homepageon lines 200-202. While the practical impact is low (last-writer-wins with similar values), this is technically a data race that tools like-racewill flag.
🤖 Fix all issues with AI agents
In `@internal/route/stream/tcp_tcp.go`:
- Around line 54-65: The listener wrapping order is wrong: when proxy-protocol
is enabled the proxyproto.Listener must wrap the raw net.Listener before ACL
wrapping so that acl.WrapTCP (applied to s.listener) sees the parsed client IP;
change the code to first check entrypoint.FromCtx() and, if
ep.SupportProxyProtocol() is true, set s.listener =
&proxyproto.Listener{Listener: s.listener} and only afterwards if
acl.FromCtx(ctx) != nil call acl.WrapTCP(s.listener) (references: s.listener,
entrypoint.FromCtx, ep.SupportProxyProtocol, proxyproto.Listener, acl.FromCtx,
acl.WrapTCP).
🧹 Nitpick comments (4)
internal/route/reverse_proxy.go (1)
193-197: Unguarded type assertion can panic at runtime.
ep.HTTPRoutes().Get(cfg.Link)returnstypes.HTTPRoute, and the assertion to*ReveseProxyRouteon line 197 will panic if a different concrete type is stored undercfg.Link. Use a comma-ok assertion and handle the mismatch gracefully.🛡️ Proposed safe assertion
l, ok := ep.HTTPRoutes().Get(cfg.Link) var linked *ReveseProxyRoute if ok { lbLock.Unlock() - linked = l.(*ReveseProxyRoute) + linked, ok = l.(*ReveseProxyRoute) + if !ok { + gperr.LogWarn("linked route is not a reverse proxy route", gperr.Errorf("unexpected type %T for %s", l, cfg.Link), &r.rp.Logger) + return + } lb = linked.loadBalancerinternal/route/stream/tcp_tcp.go (1)
56-59: Nit: localaclshadows the package alias.
acl := acl.FromCtx(ctx)shadows the import alias within theifblock, making the package unreachable there. Consider a different local name (e.g.,aclCfg) for clarity.Suggested diff
- if acl := acl.FromCtx(ctx); acl != nil { + if aclCfg := acl.FromCtx(ctx); aclCfg != nil { log.Debug().Str("listener", s.listener.Addr().String()).Msg("wrapping listener with ACL") - s.listener = acl.WrapTCP(s.listener) + s.listener = aclCfg.WrapTCP(s.listener) }internal/route/route.go (1)
953-962: Minor: reuse the already-loadedworkingStatevariable.Line 962 calls
config.WorkingState.Load()again, butworkingStatewas already loaded at line 953 and nil-guarded. Using the existing variable avoids a redundant atomic load.Proposed fix
- // TODO: default value from context - r.HealthCheck.ApplyDefaults(config.WorkingState.Load().Value().Defaults.HealthCheck) + // TODO: default value from context + r.HealthCheck.ApplyDefaults(workingState.Value().Defaults.HealthCheck)internal/route/fileserver.go (1)
120-126: Health monitor failure is now non-fatal — verify this is intentional given the inconsistency with excluded-route handling.Here, a health monitor start failure logs a warning and nils out
HealthMon, allowing the file server to continue serving. In contrast,route.golines 531–534 treat the same failure as fatal for excluded routes (returns error, aborting start).If the intent is that file servers are inherently "healthy" (local filesystem), this is reasonable. Worth a brief code comment explaining why the behavior differs.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/route/reverse_proxy.go (1)
206-221:⚠️ Potential issue | 🔴 CriticalLinked routes lack required fields for all usage contexts, risking nil pointer panics.
The linked route is created with minimal fields: only
Alias,Homepage,Bind,LisURL, andtaskin Metadata. Critically,ProxyURLis nil and never set. While the current code path inreverse_proxy.goonly accesses theloadBalancerfield, linked routes are added to the active HTTPRoutes pool and can be retrieved elsewhere. The forwardauth middleware (atinternal/net/gphttp/middleware/forwardauth.go:49-57) retrieves routes from this pool and immediately callsroute.TargetURL(), which returnsr.ProxyURL. For a linked route with nilProxyURL, this dereferences nil and causes a panic. Linked routes should either be excluded from general pool queries, marked distinctly, or have all required fields initialized.
🤖 Fix all issues with AI agents
In `@internal/entrypoint/README.md`:
- Around line 379-385: The README example shows the wrong SetCtx signature;
update the snippet to call entrypoint.SetCtx with a task.Parent (e.g., the task
variable) rather than a context.Context, and keep the corresponding retrieval
via entrypoint.FromCtx (passing the same task/parent) and use
ep.GetRoute("alias") as shown; locate the example around the SetCtx/FromCtx
usage and replace the context-based calls with the task/parent-based calls to
match the SetCtx/FromCtx API (symbols: entrypoint.SetCtx, entrypoint.FromCtx,
ep.GetRoute).
In `@internal/net/gphttp/middleware/bypass_test.go`:
- Around line 231-233: There is a duplicate assertion for the same error
variable from strconv.Atoi in the test; remove the redundant expect.NoError(t,
err) that follows the original check so the strconv.Atoi error is asserted only
once (locate the duplicate in internal net gphttp middleware bypass_test.go
around the strconv.Atoi call and the two expect.NoError(t, err) lines and delete
the second assertion).
In `@internal/route/README.md`:
- Around line 231-232: The "Route with Custom Bind Address" heading is empty;
either add the missing YAML example that mirrors the existing Go snippet showing
custom bind address usage (the Go example later in the README) or remove this
orphaned heading; update the section under the heading "Route with Custom Bind
Address" to include a short YAML configuration sample and brief explanation
matching the Go example's intent (or delete the heading if no YAML example is
desired).
In `@internal/route/reverse_proxy.go`:
- Around line 193-199: The code does an unsafe type assertion
l.(*ReveseProxyRoute) which may panic; update the block that calls
ep.HTTPRoutes().Get(cfg.Link) to use a comma-ok assertion when converting to
*ReveseProxyRoute (e.g., linked, ok := l.(*ReveseProxyRoute)), handle the case
where ok is false by unlocking lbLock and returning or skipping gracefully (do
not proceed to use linked or lb), and only call linked.loadBalancer and
lb.UpdateConfigIfNeeded(cfg) when the assertion succeeds to avoid runtime
panics.
In `@internal/route/route_test.go`:
- Around line 212-251: The subtest descriptions and assertion messages are
inconsistent with the scheme under test: update any subtest names or messages
that say "HTTP" to "HTTPS" (e.g., tests named HTTPSchemeWithoutBind,
HTTPSchemeWithBind, HTTPSchemeWithBindAndPort) and fix the require.NoError
message strings passed to r.Validate() so they read accurately (e.g., "Validate
should not return error for HTTPS route without bind", "Validate should not
return error for HTTPS route with bind", "Validate should not return error for
HTTPS route with bind and port") to match Route.Validate and route.SchemeHTTPS
usage.
🧹 Nitpick comments (10)
internal/route/stream/tcp_tcp.go (1)
56-60: Simplify the proxy-protocol conditional.The intermediate variable
proxyProtoserves no purpose — the bool is used only once and immediately.♻️ Proposed simplification
if ep := entrypoint.FromCtx(ctx); ep != nil { - if proxyProto := ep.SupportProxyProtocol(); proxyProto { + if ep.SupportProxyProtocol() { s.listener = &proxyproto.Listener{Listener: s.listener} } }internal/route/rules/presets/webui_dev.yml (1)
15-15: Redundant outer+on the capturing group.
([A-Za-z0-9_\-/]+)+contains a quantified group that is itself quantified — a classic ReDoS shape. Go's RE2 engine is immune to catastrophic backtracking, so this isn't a security issue here, but the outer+on the group serves no functional purpose (the inner+already matches one-or-more). Dropping the group or the outer quantifier makes the intent clearer.Suggested simplification
- !path regex("([A-Za-z0-9_\-/]+)+\.(css|ts|js|mjs|svg|png|jpg|jpeg|gif|ico|webp|woff2?|eot|ttf|otf|txt)(\?.*)?") + !path regex("[A-Za-z0-9_\-/]+\.(css|ts|js|mjs|svg|png|jpg|jpeg|gif|ico|webp|woff2?|eot|ttf|otf|txt)(\?.*)?")internal/entrypoint/entrypoint.go (2)
50-52: Shared mutable zero-value config could be a subtle hazard.
emptyCfgis a package-levelvarwhose address is shared by all entrypoints created withcfg == nil. If any code path later mutates fields onep.cfg, all nil-config entrypoints would be affected. Consider using&Config{}in the constructor to allocate a fresh zero-value each time, which is cheap and eliminates the shared-state risk.Proposed fix
-var emptyCfg Config - func NewEntrypoint(parent task.Parent, cfg *Config) *Entrypoint { if cfg == nil { - cfg = &emptyCfg + cfg = &Config{} }
4-8: MoveNewTestEntrypointtointernal/entrypoint/export_test.goto avoid importingtestingin production code.The
testingpackage is imported inentrypoint.gosolely forNewTestEntrypoint, which is a test helper. This bloats the production binary with test-only code. Use the idiomaticexport_test.gopattern: sinceNewTestEntrypointis used by test files in both the entrypoint package (entrypoint_test.go) and other packages (bypass_test.go), it can be exported throughexport_test.gowithout being part of production builds.internal/entrypoint/README.md (1)
17-18: Markdown links use non-standardfile:lineformat that won't resolve.Links like
[PoolLike](internal/entrypoint/types/entrypoint.go:27)use colon-delimited line references that aren't valid in standard Markdown or GitHub rendering. Consider using plain file paths or GitHub-style#L27anchors if hosted on GitHub.internal/entrypoint/entrypoint_test.go (3)
50-63: Asymmetric assertion: match path usesserver.FindRoutebut no-match path usesep.HTTPRoutes().Get.The match branch tests domain-based resolution via
server.FindRoute(test), but the no-match branch bypasses it and does a direct pool lookup viaep.HTTPRoutes().Get(test). This means the no-match tests only verify the alias doesn't exist literally in the pool—not thatFindRoutewouldn't resolve the hostname. For example, ifFindRoutehad a bug that incorrectly resolved"sub.app1.com", the no-match assertion wouldn't catch it.Consider using
server.FindRoute(test)for the no-match branch as well:♻️ Proposed fix
for _, test := range noMatch { t.Run(test, func(t *testing.T) { - found, ok := ep.HTTPRoutes().Get(test) - assert.False(t, ok) - assert.Nil(t, found) + route := server.FindRoute(test) + assert.Nil(t, route) }) }
3-15: Mixed assertion libraries:testify(assert/require) andgoutils/testing(expect) used side-by-side.The file imports both
github.com/stretchr/testify/assert+requireandgithub.com/yusing/goutils/testingasexpect. Some tests useassert.NotNil/require.Truewhile others useexpect.Equal/expect.True. This makes the test style inconsistent and increases cognitive load. Consider standardizing on one assertion library across the file.
240-253: Test relies on empty-string provider name — fragile if default changes.
byProvider[""]depends on the implicit provider name being an empty string. If the default provider name changes, this test silently breaks. Consider using a named constant or extracting the key from the map.internal/route/reverse_proxy_test.go (1)
12-38: Smoke test covers the happy path — consider adding assertions on LB state.The test verifies that two routes with load balancer linking can be created without errors, which is good for catching panics and basic wiring issues. However, it doesn't verify the actual load balancer state (e.g., that the linked route has two servers, or that both routes share the same
loadBalancerinstance). Consider adding post-creation assertions if this is the primary test for LB linking.internal/entrypoint/entrypoint_benchmark_test.go (1)
52-61: Variabletaskshadows its package import.
task := task.GetTestTask(b)on line 53 shadows thetaskpackage imported on line 20. Any subsequent call to ataskpackage function in this scope would reference the variable instead. It works here becauseGetTestTaskis the only package-level call needed, but it's fragile if the function is later extended.A simple rename (e.g.,
t := task.GetTestTask(b)) would avoid the shadow, thoughtconflicts with the common*testing.Tconvention. Something liketestTaskwould be unambiguous.
| // Set entrypoint in context | ||
| entrypoint.SetCtx(r.Context(), ep) | ||
|
|
||
| // Get entrypoint from context | ||
| if ep := entrypoint.FromCtx(r.Context()); ep != nil { | ||
| route, ok := ep.GetRoute("alias") | ||
| } |
There was a problem hiding this comment.
SetCtx API example uses incorrect signature.
The example shows entrypoint.SetCtx(r.Context(), ep) taking a context.Context, but actual usage throughout the codebase (e.g., entrypoint_benchmark_test.go line 61 and internal/route/test_route.go line 18) passes a task.Parent:
entrypoint.SetCtx(task, ep)📝 Proposed fix
-// Set entrypoint in context
-entrypoint.SetCtx(r.Context(), ep)
+// Set entrypoint in task context
+entrypoint.SetCtx(task, ep)
// Get entrypoint from context
if ep := entrypoint.FromCtx(r.Context()); ep != nil {🤖 Prompt for AI Agents
In `@internal/entrypoint/README.md` around lines 379 - 385, The README example
shows the wrong SetCtx signature; update the snippet to call entrypoint.SetCtx
with a task.Parent (e.g., the task variable) rather than a context.Context, and
keep the corresponding retrieval via entrypoint.FromCtx (passing the same
task/parent) and use ep.GetRoute("alias") as shown; locate the example around
the SetCtx/FromCtx usage and replace the context-based calls with the
task/parent-based calls to match the SetCtx/FromCtx API (symbols:
entrypoint.SetCtx, entrypoint.FromCtx, ep.GetRoute).
| l, ok := ep.HTTPRoutes().Get(cfg.Link) | ||
| var linked *ReveseProxyRoute | ||
| if ok { | ||
| lbLock.Unlock() | ||
| linked = l.(*ReveseProxyRoute) | ||
| linked = l.(*ReveseProxyRoute) // it must be a reverse proxy route | ||
| lb = linked.loadBalancer | ||
| lb.UpdateConfigIfNeeded(cfg) |
There was a problem hiding this comment.
Unsafe type assertion l.(*ReveseProxyRoute) will panic if the linked route is a different type.
If ep.HTTPRoutes().Get(cfg.Link) returns a route that isn't a *ReveseProxyRoute (e.g., a file server route or any other HTTPRoute implementation registered under the same alias), this will panic at runtime.
Consider using a comma-ok type assertion:
🛡️ Proposed fix
if ok {
lbLock.Unlock()
- linked = l.(*ReveseProxyRoute) // it must be a reverse proxy route
+ linked, ok = l.(*ReveseProxyRoute)
+ if !ok {
+ gperr.LogWarn("linked route is not a reverse proxy route", gperr.Newf("got %T", l), &r.rp.Logger)
+ return
+ }
lb = linked.loadBalancer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| l, ok := ep.HTTPRoutes().Get(cfg.Link) | |
| var linked *ReveseProxyRoute | |
| if ok { | |
| lbLock.Unlock() | |
| linked = l.(*ReveseProxyRoute) | |
| linked = l.(*ReveseProxyRoute) // it must be a reverse proxy route | |
| lb = linked.loadBalancer | |
| lb.UpdateConfigIfNeeded(cfg) | |
| l, ok := ep.HTTPRoutes().Get(cfg.Link) | |
| var linked *ReveseProxyRoute | |
| if ok { | |
| lbLock.Unlock() | |
| linked, ok = l.(*ReveseProxyRoute) | |
| if !ok { | |
| gperr.LogWarn("linked route is not a reverse proxy route", gperr.Newf("got %T", l), &r.rp.Logger) | |
| return | |
| } | |
| lb = linked.loadBalancer | |
| lb.UpdateConfigIfNeeded(cfg) |
🤖 Prompt for AI Agents
In `@internal/route/reverse_proxy.go` around lines 193 - 199, The code does an
unsafe type assertion l.(*ReveseProxyRoute) which may panic; update the block
that calls ep.HTTPRoutes().Get(cfg.Link) to use a comma-ok assertion when
converting to *ReveseProxyRoute (e.g., linked, ok := l.(*ReveseProxyRoute)),
handle the case where ok is false by unlocking lbLock and returning or skipping
gracefully (do not proceed to use linked or lb), and only call
linked.loadBalancer and lb.UpdateConfigIfNeeded(cfg) when the assertion succeeds
to avoid runtime panics.
| t.Run("HTTPSchemeWithoutBind", func(t *testing.T) { | ||
| r := &Route{ | ||
| Alias: "test-http", | ||
| Scheme: route.SchemeHTTPS, | ||
| Host: "example.com", | ||
| Port: route.Port{Proxy: 443}, | ||
| } | ||
| err := r.Validate() | ||
| require.NoError(t, err, "Validate should not return error for HTTP route with bind") | ||
| require.NotNil(t, r.LisURL, "LisURL should be set") | ||
| require.Equal(t, "https://:0", r.LisURL.String(), "LisURL should contain bind address") | ||
| }) | ||
|
|
||
| t.Run("HTTPSchemeWithBind", func(t *testing.T) { | ||
| r := &Route{ | ||
| Alias: "test-http", | ||
| Scheme: route.SchemeHTTPS, | ||
| Host: "example.com", | ||
| Port: route.Port{Proxy: 443}, | ||
| Bind: "0.0.0.0", | ||
| } | ||
| err := r.Validate() | ||
| require.NoError(t, err, "Validate should not return error for HTTP route with bind") | ||
| require.NotNil(t, r.LisURL, "LisURL should be set") | ||
| require.Equal(t, "https://0.0.0.0:0", r.LisURL.String(), "LisURL should contain bind address") | ||
| }) | ||
|
|
||
| t.Run("HTTPSchemeWithBindAndPort", func(t *testing.T) { | ||
| r := &Route{ | ||
| Alias: "test-http", | ||
| Scheme: route.SchemeHTTPS, | ||
| Host: "example.com", | ||
| Port: route.Port{Listening: 8443, Proxy: 443}, | ||
| Bind: "0.0.0.0", | ||
| } | ||
| err := r.Validate() | ||
| require.NoError(t, err, "Validate should not return error for HTTP route with bind and port") | ||
| require.NotNil(t, r.LisURL, "LisURL should be set") | ||
| require.Equal(t, "https://0.0.0.0:8443", r.LisURL.String(), "LisURL should contain bind address and listening port") | ||
| }) |
There was a problem hiding this comment.
Misleading subtest names: these test HTTPS, not HTTP.
All three subtests (HTTPSchemeWithoutBind, HTTPSchemeWithBind, HTTPSchemeWithBindAndPort) use route.SchemeHTTPS but are named with "HTTP". Additionally, the error message on line 220 says "HTTP route with bind" even though this subtest is the "WithoutBind" variant.
Consider renaming to HTTPSSchemeWithoutBind, HTTPSSchemeWithBind, etc., and fixing the message on line 220.
Suggested fix
- t.Run("HTTPSchemeWithoutBind", func(t *testing.T) {
+ t.Run("HTTPSSchemeWithoutBind", func(t *testing.T) {
r := &Route{
Alias: "test-http",
Scheme: route.SchemeHTTPS,
Host: "example.com",
Port: route.Port{Proxy: 443},
}
err := r.Validate()
- require.NoError(t, err, "Validate should not return error for HTTP route with bind")
+ require.NoError(t, err, "Validate should not return error for HTTPS route without bind")
require.NotNil(t, r.LisURL, "LisURL should be set")
require.Equal(t, "https://:0", r.LisURL.String(), "LisURL should contain bind address")
})
- t.Run("HTTPSchemeWithBind", func(t *testing.T) {
+ t.Run("HTTPSSchemeWithBind", func(t *testing.T) {
r := &Route{
Alias: "test-http",
Scheme: route.SchemeHTTPS,
Host: "example.com",
Port: route.Port{Proxy: 443},
Bind: "0.0.0.0",
}
err := r.Validate()
- require.NoError(t, err, "Validate should not return error for HTTP route with bind")
+ require.NoError(t, err, "Validate should not return error for HTTPS route with bind")
- t.Run("HTTPSchemeWithBindAndPort", func(t *testing.T) {
+ t.Run("HTTPSSchemeWithBindAndPort", func(t *testing.T) {🤖 Prompt for AI Agents
In `@internal/route/route_test.go` around lines 212 - 251, The subtest
descriptions and assertion messages are inconsistent with the scheme under test:
update any subtest names or messages that say "HTTP" to "HTTPS" (e.g., tests
named HTTPSchemeWithoutBind, HTTPSchemeWithBind, HTTPSchemeWithBindAndPort) and
fix the require.NoError message strings passed to r.Validate() so they read
accurately (e.g., "Validate should not return error for HTTPS route without
bind", "Validate should not return error for HTTPS route with bind", "Validate
should not return error for HTTPS route with bind and port") to match
Route.Validate and route.SchemeHTTPS usage.
…ream serving Changed AddRoute and ListenAndServe methods to return errors instead of logging them internally and continuing. This allows callers to properly handle and propagate errors, improving error visibility and enabling better error management across the codebase. Updated all callers in fileserver, reverse_proxy, stream routes to handle these errors appropriately.
| // @Router /health [get] | ||
| func Health(c *gin.Context) { | ||
| ep := entrypoint.FromCtx(c.Request.Context()) | ||
| if ep == nil { // impossible, but just in case |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx returns nil when the request context does not contain an entrypoint value. Calling ep.GetHealthInfoSimple() on a nil pointer will cause a panic. Consider guarding against nil or ensuring the context is always populated.
| if ep == nil { // impossible, but just in case | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| if httpheaders.IsWebsocket(c.Request.Header) { |
| // @Success 200 {array} string | ||
| // @Failure 403 {object} apitypes.ErrorResponse | ||
| // @Failure 500 {object} apitypes.ErrorResponse | ||
| // @Router /homepage/categories [get] |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx can return nil. Calling ep.RoutesByProvider() will panic if ep is nil.
| // @Router /homepage/categories [get] | |
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| byProvider := ep.RoutesByProvider() |
| // @Router /route/by_provider [get] | ||
| func ByProvider(c *gin.Context) { | ||
| c.JSON(http.StatusOK, routes.ByProvider()) | ||
| ep := entrypoint.FromCtx(c.Request.Context()) |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx can return nil. Calling ep.RoutesByProvider() will panic if ep is nil.
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| byProvider := ep.RoutesByProvider() |
| } | ||
|
|
||
| route, ok := routes.GetIncludeExcluded(request.Which) | ||
| ep := entrypoint.FromCtx(c.Request.Context()) |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx can return nil. Calling ep.GetRoute(request.Which) will panic if ep is nil.
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| route, ok := ep.GetRoute(request.Which) |
| return | ||
| } | ||
|
|
||
| ep := entrypoint.FromCtx(c.Request.Context()) |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx can return nil. Calling ep.GetHealthInfo() will panic if ep is nil.
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| if httpheaders.IsWebsocket(c.Request.Header) { |
| } | ||
|
|
||
| func RoutesWS(c *gin.Context) { | ||
| ep := entrypoint.FromCtx(c.Request.Context()) |
There was a problem hiding this comment.
CRITICAL: Possible nil dereference when entrypoint context is missing
entrypoint.FromCtx can return nil. Calling ep.GetHealthInfoWithoutDetail() will panic if ep is nil.
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| ep := entrypoint.FromCtx(c.Request.Context()) | |
| if ep == nil { | |
| c.JSON(http.StatusInternalServerError, apitypes.Error("entrypoint not found in context", nil)) | |
| return | |
| } | |
| c.JSON(http.StatusOK, ep.GetHealthInfoWithoutDetail()) |
| func (r *ReveseProxyRoute) addToLoadBalancer(parent task.Parent, ep entrypoint.Entrypoint) error { | ||
| var lb *loadbalancer.LoadBalancer | ||
| cfg := r.LoadBalance | ||
| lbLock.Lock() |
There was a problem hiding this comment.
CRITICAL: Unsafe type assertion l.(*ReveseProxyRoute) will panic if the linked route is a different type.
If ep.HTTPRoutes().Get(cfg.Link) returns a route that isn't a *ReveseProxyRoute, this will panic. Use a comma-ok type assertion.
| lbLock.Lock() | |
| l, ok := ep.HTTPRoutes().Get(cfg.Link) | |
| var linked *ReveseProxyRoute | |
| if ok { | |
| lbLock.Unlock() | |
| linked, ok = l.(*ReveseProxyRoute) | |
| if !ok { | |
| return fmt.Errorf("linked route %s is not a reverse proxy route", cfg.Link) | |
| } | |
| lb = linked.loadBalancer | |
| lb.UpdateConfigIfNeeded(cfg) |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/route/stream/README.md (3)
256-263:⚠️ Potential issue | 🟡 MinorUsage example ignores the new error return from
ListenAndServe.The interface now returns
error, but this example discards it. This will mislead consumers into ignoring the error.📝 Proposed fix
-tcpStream.ListenAndServe(ctx, preDialHook, onReadHook) +if err := tcpStream.ListenAndServe(ctx, preDialHook, onReadHook); err != nil { + return err +}
265-279:⚠️ Potential issue | 🟡 MinorSame issue: "Using Hook Functions" example also discards the error.
-stream.ListenAndServe(ctx, +if err := stream.ListenAndServe(ctx, func(ctx context.Context) error { // Pre-dial: authentication, rate limiting log.Println("Pre-dial check") return nil }, func(ctx context.Context) error { // On-read: metrics, throttling return nil }, -) +); err != nil { + return err +}
78-102:⚠️ Potential issue | 🟡 MinorMermaid class diagrams omit the
errorreturn onListenAndServe.The
Stream,TCPTCPStream, andUDPUDPStreamclass entries at lines 82, 91, and 100 show+ListenAndServe(ctx, preDial, onRead)without theerrorreturn, inconsistent with the interface definition shown at line 66.internal/route/stream.go (1)
85-87:⚠️ Potential issue | 🟠 MajorStreamRoute.ListenAndServe violates the Stream interface contract and breaks caller error handling.
StreamRoute.ListenAndServedoes not return an error, but theStreaminterface (defined ininternal/net/types/stream.go) requiresListenAndServeto returnerror. All other Stream implementations (TCPTCPStream, UDPUDPStream, and Watcher) correctly return the error. Additionally, the caller ininternal/entrypoint/routes.goalready attempts to capture and handle the error (err := r.ListenAndServe(...)), which fails because StreamRoute doesn't return it.Proposed fix
-func (r *StreamRoute) ListenAndServe(ctx context.Context, preDial, onRead nettypes.HookFunc) { - r.stream.ListenAndServe(ctx, preDial, onRead) +func (r *StreamRoute) ListenAndServe(ctx context.Context, preDial, onRead nettypes.HookFunc) error { + return r.stream.ListenAndServe(ctx, preDial, onRead) }
🤖 Fix all issues with AI agents
In `@internal/entrypoint/README.md`:
- Around line 41-62: The README's Entrypoint interface is out of date: it
documents AddRoute(r types.Route) error but the actual interface uses
StartAddRoute(r types.Route) error; update the README entry for Entrypoint to
replace AddRoute with StartAddRoute and ensure the signature and surrounding
comments match the real interface (referencing the Entrypoint interface and
StartAddRoute method name to locate and correct the doc).
- Around line 364-374: The README example instantiates route.Port with a
non-existent Target field; update the example to use the actual fields defined
on route.Port (Listening and Proxy) so it matches the route.Route definition.
Specifically, modify the sample where Port: route.Port{Proxy: 80, Target: 3000}
to use route.Port{Proxy: 80, Listening: 3000} (keeping the rest of the example
using route.Route and ep.AddRoute unchanged) so readers can follow the example
against the real struct definition.
In `@internal/entrypoint/routes.go`:
- Around line 100-113: AddHTTPRoute can leave a route registered on one server
if the other server add fails; detect partial failure after calling
ep.addHTTPRoute and roll back the successful add: if httpErr != nil && httpsErr
== nil call the counterpart removal (e.g., ep.removeHTTPRoute(route, httpsAddr,
HTTPProtoHTTPS) or the actual route-unregister method) to undo the successful
HTTPS add, and vice-versa when httpsErr != nil && httpErr == nil; join the
original add error with any removal error via errors.Join (and/or log removal
failures) so callers (like StartAddRoute) don't observe a partially-registered
route.
In `@internal/route/README.md`:
- Around line 314-324: The README examples use a non-existent Port field
"Target" (and YAML "target") instead of the actual field "Proxy" defined on the
Port struct in types/port.go; update the Go examples that set Port{Target: ...}
to Port{Proxy: ...} and change any YAML examples using "target:" to "proxy:" so
they match the Port struct's Listening and Proxy fields.
In `@internal/route/reverse_proxy.go`:
- Around line 197-206: The code currently defers lbLock.Unlock() at the top and
then calls lbLock.Unlock() again inside the if ok branch causing a double-unlock
panic; remove the explicit lbLock.Unlock() inside the if branch so the deferred
unlock is the only unlock, and ensure you still perform the type assertion to
*ReveseProxyRoute and call linked.loadBalancer and lb.UpdateConfigIfNeeded(cfg)
as before (symbols to edit: lbLock, ep.HTTPRoutes().Get, ReveseProxyRoute,
linked.loadBalancer, lb.UpdateConfigIfNeeded).
🧹 Nitpick comments (7)
internal/api/v1/homepage/categories.go (1)
10-11: Redundant blank import ofapitypes.Line 10 has a blank import (
_ "github.com/yusing/goutils/apitypes") while line 11 imports the same package with theapitypesalias. The blank import is unnecessary since the named import already ensures the package is linked.Proposed fix
- _ "github.com/yusing/goutils/apitypes" - apitypes "github.com/yusing/goutils/apitypes" + apitypes "github.com/yusing/goutils/apitypes"internal/route/README.md (1)
33-37: Consider clarifying the relationship between Host and Bind fields.The
Hostfield documentation states "Virtual host / target address" which serves dual purposes depending on route type. The newBindfield is for the listening address. To avoid confusion, consider explicitly documenting:
- For HTTP/HTTPS routes: Host = virtual hostname for routing (HTTP Host header matching)
- For stream/TCP/UDP routes: Host = target/upstream address
- Bind = local IP address to bind the listener (optional, defaults to all interfaces)
This clarification would help users understand when and how to use each field.
internal/route/stream/tcp_tcp.go (1)
53-57: Nit: unnecessary intermediate variable.
proxyProtois only used in the condition; you can simplify.Proposed simplification
if ep := entrypoint.FromCtx(ctx); ep != nil { - if proxyProto := ep.SupportProxyProtocol(); proxyProto { + if ep.SupportProxyProtocol() { s.listener = &proxyproto.Listener{Listener: s.listener} } }internal/entrypoint/http_server.go (2)
22-29: Comment on line 22 describes the concrete type, not the interface.The doc comment says
// httpServer is a server…but is attached to the exportedHTTPServerinterface. Consider updating to describe the interface itself (e.g.,// HTTPServer defines the interface for…).
153-175:serveNotFoundlogs every unmatched request atErrorlevel — this will be noisy under scanner traffic.Bots and vulnerability scanners routinely hit random subdomains/paths. Logging every miss at
Errorwill flood logs and mask real errors. ConsiderWarnorDebug, or rate-limiting these entries.Proposed change
if served := middleware.ServeStaticErrorPageFile(w, r); !served { - log.Error(). + log.Debug(). Str("method", r.Method). Str("url", r.URL.String()). Str("remote", r.RemoteAddr). Msgf("not found: %s", r.Host)internal/entrypoint/routes.go (1)
83-98:getAddrsilently returns emptyhttpAddrfor custom-port routes.When the route's listen port is set (non-empty, non-
"0"), onlyhttpsAddris populated (line 96) whilehttpAddrstays as the zero-value empty string. This means custom-port routes are only served over HTTPS. If this is intentional (custom listen ports always imply TLS), a brief comment would help future readers.internal/entrypoint/entrypoint.go (1)
4-20: Production code importstesting— this pulls test infrastructure into non-test builds.
testing.TBis used only byNewTestEntrypoint(line 54). Placing a test-only helper in a non-_test.gofile causestestingto be linked into the production binary. Consider movingNewTestEntrypointinto atesting_helpers_test.goor anexport_test.gofile, or into a dedicatedentrypointtestsub-package that tests import explicitly.
| route := &route.Route{ | ||
| Alias: "myapp", | ||
| Scheme: route.SchemeHTTP, | ||
| Host: "myapp", | ||
| Port: route.Port{Proxy: 80, Target: 3000}, | ||
| } | ||
|
|
||
| if err := ep.AddRoute(route); err != nil { | ||
| return err | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Example uses non-existent Port.Target field.
The route.Port struct uses Listening and Proxy (see route.go line 873), not Target. This will confuse readers trying to follow the example.
Proposed fix
route := &route.Route{
Alias: "myapp",
Scheme: route.SchemeHTTP,
Host: "myapp",
- Port: route.Port{Proxy: 80, Target: 3000},
+ Port: route.Port{Proxy: 3000},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| route := &route.Route{ | |
| Alias: "myapp", | |
| Scheme: route.SchemeHTTP, | |
| Host: "myapp", | |
| Port: route.Port{Proxy: 80, Target: 3000}, | |
| } | |
| if err := ep.AddRoute(route); err != nil { | |
| return err | |
| } | |
| ``` | |
| route := &route.Route{ | |
| Alias: "myapp", | |
| Scheme: route.SchemeHTTP, | |
| Host: "myapp", | |
| Port: route.Port{Proxy: 3000}, | |
| } | |
| if err := ep.AddRoute(route); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In `@internal/entrypoint/README.md` around lines 364 - 374, The README example
instantiates route.Port with a non-existent Target field; update the example to
use the actual fields defined on route.Port (Listening and Proxy) so it matches
the route.Route definition. Specifically, modify the sample where Port:
route.Port{Proxy: 80, Target: 3000} to use route.Port{Proxy: 80, Listening:
3000} (keeping the rest of the example using route.Route and ep.AddRoute
unchanged) so readers can follow the example against the real struct definition.
| // AddHTTPRoute adds a HTTP route to the entrypoint's server. | ||
| // | ||
| // If the server does not exist, it will be created, started and return any error. | ||
| func (ep *Entrypoint) AddHTTPRoute(route types.HTTPRoute) error { | ||
| httpAddr, httpsAddr := getAddr(route) | ||
| var httpErr, httpsErr error | ||
| if httpAddr != "" { | ||
| httpErr = ep.addHTTPRoute(route, httpAddr, HTTPProtoHTTP) | ||
| } | ||
| if httpsAddr != "" { | ||
| httpsErr = ep.addHTTPRoute(route, httpsAddr, HTTPProtoHTTPS) | ||
| } | ||
| return errors.Join(httpErr, httpsErr) | ||
| } |
There was a problem hiding this comment.
Partial route registration when only one of HTTP/HTTPS server creation fails.
If httpErr != nil but httpsErr == nil (or vice-versa), AddHTTPRoute returns a non-nil error. The caller in StartAddRoute (line 58) returns immediately without registering a cancel hook, but the route was already added to the successful server and will never be cleaned up.
Either:
- Roll back the successful add when the other fails, or
- Treat each server independently so one failure doesn't block the other.
Option 1 – rollback on partial failure
func (ep *Entrypoint) AddHTTPRoute(route types.HTTPRoute) error {
httpAddr, httpsAddr := getAddr(route)
var httpErr, httpsErr error
if httpAddr != "" {
httpErr = ep.addHTTPRoute(route, httpAddr, HTTPProtoHTTP)
}
if httpsAddr != "" {
httpsErr = ep.addHTTPRoute(route, httpsAddr, HTTPProtoHTTPS)
}
- return errors.Join(httpErr, httpsErr)
+ if err := errors.Join(httpErr, httpsErr); err != nil {
+ // rollback any successful add
+ if httpErr == nil && httpAddr != "" {
+ if srv, ok := ep.servers.Load(httpAddr); ok {
+ srv.DelRoute(route)
+ }
+ }
+ if httpsErr == nil && httpsAddr != "" {
+ if srv, ok := ep.servers.Load(httpsAddr); ok {
+ srv.DelRoute(route)
+ }
+ }
+ return err
+ }
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // AddHTTPRoute adds a HTTP route to the entrypoint's server. | |
| // | |
| // If the server does not exist, it will be created, started and return any error. | |
| func (ep *Entrypoint) AddHTTPRoute(route types.HTTPRoute) error { | |
| httpAddr, httpsAddr := getAddr(route) | |
| var httpErr, httpsErr error | |
| if httpAddr != "" { | |
| httpErr = ep.addHTTPRoute(route, httpAddr, HTTPProtoHTTP) | |
| } | |
| if httpsAddr != "" { | |
| httpsErr = ep.addHTTPRoute(route, httpsAddr, HTTPProtoHTTPS) | |
| } | |
| return errors.Join(httpErr, httpsErr) | |
| } | |
| // AddHTTPRoute adds a HTTP route to the entrypoint's server. | |
| // | |
| // If the server does not exist, it will be created, started and return any error. | |
| func (ep *Entrypoint) AddHTTPRoute(route types.HTTPRoute) error { | |
| httpAddr, httpsAddr := getAddr(route) | |
| var httpErr, httpsErr error | |
| if httpAddr != "" { | |
| httpErr = ep.addHTTPRoute(route, httpAddr, HTTPProtoHTTP) | |
| } | |
| if httpsAddr != "" { | |
| httpsErr = ep.addHTTPRoute(route, httpsAddr, HTTPProtoHTTPS) | |
| } | |
| if err := errors.Join(httpErr, httpsErr); err != nil { | |
| // rollback any successful add | |
| if httpErr == nil && httpAddr != "" { | |
| if srv, ok := ep.servers.Load(httpAddr); ok { | |
| srv.DelRoute(route) | |
| } | |
| } | |
| if httpsErr == nil && httpsAddr != "" { | |
| if srv, ok := ep.servers.Load(httpsAddr); ok { | |
| srv.DelRoute(route) | |
| } | |
| } | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/entrypoint/routes.go` around lines 100 - 113, AddHTTPRoute can leave
a route registered on one server if the other server add fails; detect partial
failure after calling ep.addHTTPRoute and roll back the successful add: if
httpErr != nil && httpsErr == nil call the counterpart removal (e.g.,
ep.removeHTTPRoute(route, httpsAddr, HTTPProtoHTTPS) or the actual
route-unregister method) to undo the successful HTTPS add, and vice-versa when
httpsErr != nil && httpErr == nil; join the original add error with any removal
error via errors.Join (and/or log removal failures) so callers (like
StartAddRoute) don't observe a partially-registered route.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/entrypoint/entrypoint.go`:
- Line 48: The global var emptyCfg and returning &emptyCfg hands every
nil-config Entrypoint the same Config pointer, risking cross-instance mutation
and races; replace uses of &emptyCfg with a fresh zero-value allocation (e.g.,
new(Config) or &Config{}) at the point where a default config is needed (where
Entrypoint is constructed/where nil-config is handled—references: emptyCfg,
Config, Entrypoint, and the function(s) that currently return &emptyCfg) so each
Entrypoint gets its own zeroed Config instance instead of a shared pointer.
In `@internal/entrypoint/http_server.go`:
- Around line 95-101: Add a nil-guard to AddRoute and DelRoute so they don't
panic when called before Listen: check if srv.routes is nil and initialize it
using the same initialization logic used in Listen (or call a new helper
initRoutes that encapsulates that logic) before calling srv.routes.Add/Del;
alternatively, if you prefer to require Listen first, replace the silent panic
with an explicit, clear error/log and return early. Reference functions:
AddRoute, DelRoute, and Listen (or create/initRoutes helper) to locate where to
add the nil-check and initialization.
- Around line 56-86: The Listen method starts the server with server.StartServer
while srv.routes is still nil, causing ServeHTTP -> findRouteFunc(srv.routes,
...) -> routes.Get to panic on early requests; fix by initializing srv.routes
(and set its DisableLog state) before calling server.StartServer so the handler
has a valid routes pool when StartServer begins accepting connections (update
the initialization that currently occurs after StartServer in Listen to occur
prior to the StartServer call).
🧹 Nitpick comments (2)
internal/entrypoint/entrypoint.go (1)
99-101:HTTPRoutes()creates a new adapter on every call.Each invocation of
HTTPRoutes()allocates a freshnewHTTPPoolAdapter(ep). If callers invoke this frequently (e.g., in hot paths or loops), this produces unnecessary allocations. Consider caching the adapter or documenting that callers should cache the return value.internal/entrypoint/http_server.go (1)
151-173:log.Error()for not-found requests is too noisy for production.Every scanner/bot hit on an unknown host will produce an error-level log entry. This pollutes logs and can trigger alert fatigue. Consider
log.Debug()orlog.Warn()instead, reservingErrorfor actual server-side failures.Proposed fix
- log.Error(). + log.Debug(). Str("method", r.Method). Str("url", r.URL.String()). Str("remote", r.RemoteAddr). Msgf("not found: %s", r.Host)
| var ActiveConfig atomic.Pointer[entrypoint.Config] | ||
| var _ entrypoint.Entrypoint = &Entrypoint{} | ||
|
|
||
| var emptyCfg Config |
There was a problem hiding this comment.
Shared pointer to emptyCfg risks unintended mutation across instances.
&emptyCfg hands every nil-config Entrypoint the same Config pointer. If Config has any mutable field that is written after construction (e.g., by a setter or a test), the mutation leaks to all nil-config entrypoints and introduces a data race.
Consider allocating a fresh zero-value instead:
Proposed fix
-var emptyCfg Config
func NewEntrypoint(parent task.Parent, cfg *Config) *Entrypoint {
if cfg == nil {
- cfg = &emptyCfg
+ cfg = &Config{}
}Also applies to: 59-62
🤖 Prompt for AI Agents
In `@internal/entrypoint/entrypoint.go` at line 48, The global var emptyCfg and
returning &emptyCfg hands every nil-config Entrypoint the same Config pointer,
risking cross-instance mutation and races; replace uses of &emptyCfg with a
fresh zero-value allocation (e.g., new(Config) or &Config{}) at the point where
a default config is needed (where Entrypoint is constructed/where nil-config is
handled—references: emptyCfg, Config, Entrypoint, and the function(s) that
currently return &emptyCfg) so each Entrypoint gets its own zeroed Config
instance instead of a shared pointer.
| func (srv *httpServer) Listen(addr string, proto HTTPProto) error { | ||
| if srv.addr != "" { | ||
| return errors.New("server already started") | ||
| } | ||
|
|
||
| opts := server.Options{ | ||
| Name: addr, | ||
| Handler: srv, | ||
| ACL: acl.FromCtx(srv.ep.task.Context()), | ||
| SupportProxyProtocol: srv.ep.cfg.SupportProxyProtocol, | ||
| } | ||
|
|
||
| switch proto { | ||
| case HTTPProtoHTTP: | ||
| opts.HTTPAddr = addr | ||
| case HTTPProtoHTTPS: | ||
| opts.HTTPSAddr = addr | ||
| opts.CertProvider = autocert.FromCtx(srv.ep.task.Context()) | ||
| } | ||
|
|
||
| task := srv.ep.task.Subtask("http_server", false) | ||
| _, err := server.StartServer(task, opts) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| srv.stopFunc = task.FinishAndWait | ||
| srv.addr = addr | ||
| srv.routes = pool.New[types.HTTPRoute](fmt.Sprintf("[%s] %s", proto, addr)) | ||
| srv.routes.DisableLog(srv.ep.httpPoolDisableLog.Load()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Race condition: srv.routes is nil when server starts accepting requests.
server.StartServer (line 77) wires srv as the handler and begins accepting connections. srv.routes is only initialized on line 83 after the server has started. Any request arriving in between will cause a nil-pointer dereference in ServeHTTP → findRouteFunc(srv.routes, r.Host) → routes.Get(...).
Initialize srv.routes before starting the server:
Proposed fix
func (srv *httpServer) Listen(addr string, proto HTTPProto) error {
if srv.addr != "" {
return errors.New("server already started")
}
+ srv.addr = addr
+ srv.routes = pool.New[types.HTTPRoute](fmt.Sprintf("[%s] %s", proto, addr))
+ srv.routes.DisableLog(srv.ep.httpPoolDisableLog.Load())
+
opts := server.Options{
Name: addr,
Handler: srv,
ACL: acl.FromCtx(srv.ep.task.Context()),
SupportProxyProtocol: srv.ep.cfg.SupportProxyProtocol,
}
switch proto {
case HTTPProtoHTTP:
opts.HTTPAddr = addr
case HTTPProtoHTTPS:
opts.HTTPSAddr = addr
opts.CertProvider = autocert.FromCtx(srv.ep.task.Context())
}
task := srv.ep.task.Subtask("http_server", false)
_, err := server.StartServer(task, opts)
if err != nil {
+ srv.addr = ""
+ srv.routes = nil
return err
}
srv.stopFunc = task.FinishAndWait
- srv.addr = addr
- srv.routes = pool.New[types.HTTPRoute](fmt.Sprintf("[%s] %s", proto, addr))
- srv.routes.DisableLog(srv.ep.httpPoolDisableLog.Load())
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (srv *httpServer) Listen(addr string, proto HTTPProto) error { | |
| if srv.addr != "" { | |
| return errors.New("server already started") | |
| } | |
| opts := server.Options{ | |
| Name: addr, | |
| Handler: srv, | |
| ACL: acl.FromCtx(srv.ep.task.Context()), | |
| SupportProxyProtocol: srv.ep.cfg.SupportProxyProtocol, | |
| } | |
| switch proto { | |
| case HTTPProtoHTTP: | |
| opts.HTTPAddr = addr | |
| case HTTPProtoHTTPS: | |
| opts.HTTPSAddr = addr | |
| opts.CertProvider = autocert.FromCtx(srv.ep.task.Context()) | |
| } | |
| task := srv.ep.task.Subtask("http_server", false) | |
| _, err := server.StartServer(task, opts) | |
| if err != nil { | |
| return err | |
| } | |
| srv.stopFunc = task.FinishAndWait | |
| srv.addr = addr | |
| srv.routes = pool.New[types.HTTPRoute](fmt.Sprintf("[%s] %s", proto, addr)) | |
| srv.routes.DisableLog(srv.ep.httpPoolDisableLog.Load()) | |
| return nil | |
| } | |
| func (srv *httpServer) Listen(addr string, proto HTTPProto) error { | |
| if srv.addr != "" { | |
| return errors.New("server already started") | |
| } | |
| srv.addr = addr | |
| srv.routes = pool.New[types.HTTPRoute](fmt.Sprintf("[%s] %s", proto, addr)) | |
| srv.routes.DisableLog(srv.ep.httpPoolDisableLog.Load()) | |
| opts := server.Options{ | |
| Name: addr, | |
| Handler: srv, | |
| ACL: acl.FromCtx(srv.ep.task.Context()), | |
| SupportProxyProtocol: srv.ep.cfg.SupportProxyProtocol, | |
| } | |
| switch proto { | |
| case HTTPProtoHTTP: | |
| opts.HTTPAddr = addr | |
| case HTTPProtoHTTPS: | |
| opts.HTTPSAddr = addr | |
| opts.CertProvider = autocert.FromCtx(srv.ep.task.Context()) | |
| } | |
| task := srv.ep.task.Subtask("http_server", false) | |
| _, err := server.StartServer(task, opts) | |
| if err != nil { | |
| srv.addr = "" | |
| srv.routes = nil | |
| return err | |
| } | |
| srv.stopFunc = task.FinishAndWait | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/entrypoint/http_server.go` around lines 56 - 86, The Listen method
starts the server with server.StartServer while srv.routes is still nil, causing
ServeHTTP -> findRouteFunc(srv.routes, ...) -> routes.Get to panic on early
requests; fix by initializing srv.routes (and set its DisableLog state) before
calling server.StartServer so the handler has a valid routes pool when
StartServer begins accepting connections (update the initialization that
currently occurs after StartServer in Listen to occur prior to the StartServer
call).
| func (srv *httpServer) AddRoute(route types.HTTPRoute) { | ||
| srv.routes.Add(route) | ||
| } | ||
|
|
||
| func (srv *httpServer) DelRoute(route types.HTTPRoute) { | ||
| srv.routes.Del(route) | ||
| } |
There was a problem hiding this comment.
AddRoute/DelRoute will panic if called before Listen.
srv.routes is nil until Listen is called. A nil-guard or documented precondition would prevent a confusing panic.
🤖 Prompt for AI Agents
In `@internal/entrypoint/http_server.go` around lines 95 - 101, Add a nil-guard to
AddRoute and DelRoute so they don't panic when called before Listen: check if
srv.routes is nil and initialize it using the same initialization logic used in
Listen (or call a new helper initRoutes that encapsulates that logic) before
calling srv.routes.Add/Del; alternatively, if you prefer to require Listen
first, replace the silent panic with an explicit, clear error/log and return
early. Reference functions: AddRoute, DelRoute, and Listen (or create/initRoutes
helper) to locate where to add the nil-check and initialization.
|
2 participants
Replace global routes registry with entrypoint-scoped pools and context lookups, and centralize API/metrics startup in config state.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Chores
Documentation