Fix Caddy route creation when routes array is null#25
Merged
Conversation
Caddy returns a 500 error when appending a route to a server whose `routes` field is null (rather than an empty array). This fixes `devx caddy check --fix` by: - Discovering the port-80 server dynamically instead of hardcoding srv1 - Adding EnsureRoutesArray() to initialize null/missing routes before route creation batches - Making GetAllRoutes resilient to null bodies and 404 responses - Adding comprehensive unit tests with httptest mocks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces dynamic HTTP server discovery in the Caddy client, replaces hard-coded /srv1 paths with server-specific paths, adds EnsureRoutesArray to initialize missing routes arrays, and integrates that initialization into health and provisioning flows. Extensive tests cover discovery, initialization, and null/404 handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CaddyClient
participant Admin as Caddy Admin API
participant ServerConfig as Server Config
Client->>Admin: GET /config/apps/http/servers
Admin-->>Client: JSON list of servers
Client->>Client: discoverServerName() -> pick serverName (port 80 or fallback)
Client->>ServerConfig: PATCH /config/apps/http/servers/{serverName}/routes {"routes":[]}
ServerConfig-->>Client: 200 OK (routes initialized) / 404 or error
Client->>ServerConfig: GET /config/apps/http/servers/{serverName}/routes
ServerConfig-->>Client: routes (or null/404 handled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@caddy/routes_test.go`:
- Around line 180-238: Handlers in the tests use json.NewEncoder(w).Encode(...)
and w.Write(...) with unchecked error returns which fails errcheck; update each
test handler (the http.HandlerFunc closures in the t.Run cases) to explicitly
handle or discard the errors by assigning the returns (e.g. _, _ =
json.NewEncoder(w).Encode(...)) or by checking the error and t.Fatalf if
non-nil; specifically address the Encode calls inside the first three handlers
and the w.Write calls in the last two handlers so json.NewEncoder(w).Encode and
w.Write return values are not left unchecked.
In `@caddy/routes.go`:
- Around line 70-97: The port-matching in discoverServerName uses
strings.Contains(addr, ":80") which false-positives on ports like :8080; update
the loop that iterates srv.Listen (the addr variable) to reliably extract the
port (use net.SplitHostPort or handle leading ":" cases) and compare the port
string exactly to "80" before setting c.serverName; ensure IPv6 forms like
"[::]:80" and short forms like ":80" are handled and skip entries that don’t
parse to a host:port pair.
Comment on lines
+70
to
+97
| func (c *CaddyClient) discoverServerName() { | ||
| c.serverName = "srv1" // default fallback | ||
|
|
||
| resp, err := c.client.R().Get(c.baseURL + "/config/apps/http/servers") | ||
| if err != nil || resp.StatusCode() != http.StatusOK { | ||
| return | ||
| } | ||
|
|
||
| var servers map[string]json.RawMessage | ||
| if err := json.Unmarshal(resp.Body(), &servers); err != nil { | ||
| return | ||
| } | ||
|
|
||
| for name, raw := range servers { | ||
| var srv struct { | ||
| Listen []string `json:"listen"` | ||
| } | ||
| if err := json.Unmarshal(raw, &srv); err != nil { | ||
| continue | ||
| } | ||
| for _, addr := range srv.Listen { | ||
| if strings.Contains(addr, ":80") { | ||
| c.serverName = name | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Port-matching logic can produce false positives for ports like :8080, :800, :8000.
strings.Contains(addr, ":80") matches any address containing the substring :80, not just port 80. A server listening on :8080 or :8000 would be incorrectly selected.
🐛 Proposed fix: use a stricter match
for _, addr := range srv.Listen {
- if strings.Contains(addr, ":80") {
+ if addr == ":80" || strings.HasSuffix(addr, ":80") {
c.serverName = name
return
}This covers both :80 and 0.0.0.0:80 / [::]:80 while avoiding false matches on :8080, :800, etc.
📝 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.
Suggested change
| func (c *CaddyClient) discoverServerName() { | |
| c.serverName = "srv1" // default fallback | |
| resp, err := c.client.R().Get(c.baseURL + "/config/apps/http/servers") | |
| if err != nil || resp.StatusCode() != http.StatusOK { | |
| return | |
| } | |
| var servers map[string]json.RawMessage | |
| if err := json.Unmarshal(resp.Body(), &servers); err != nil { | |
| return | |
| } | |
| for name, raw := range servers { | |
| var srv struct { | |
| Listen []string `json:"listen"` | |
| } | |
| if err := json.Unmarshal(raw, &srv); err != nil { | |
| continue | |
| } | |
| for _, addr := range srv.Listen { | |
| if strings.Contains(addr, ":80") { | |
| c.serverName = name | |
| return | |
| } | |
| } | |
| } | |
| } | |
| func (c *CaddyClient) discoverServerName() { | |
| c.serverName = "srv1" // default fallback | |
| resp, err := c.client.R().Get(c.baseURL + "/config/apps/http/servers") | |
| if err != nil || resp.StatusCode() != http.StatusOK { | |
| return | |
| } | |
| var servers map[string]json.RawMessage | |
| if err := json.Unmarshal(resp.Body(), &servers); err != nil { | |
| return | |
| } | |
| for name, raw := range servers { | |
| var srv struct { | |
| Listen []string `json:"listen"` | |
| } | |
| if err := json.Unmarshal(raw, &srv); err != nil { | |
| continue | |
| } | |
| for _, addr := range srv.Listen { | |
| if addr == ":80" || strings.HasSuffix(addr, ":80") { | |
| c.serverName = name | |
| return | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@caddy/routes.go` around lines 70 - 97, The port-matching in
discoverServerName uses strings.Contains(addr, ":80") which false-positives on
ports like :8080; update the loop that iterates srv.Listen (the addr variable)
to reliably extract the port (use net.SplitHostPort or handle leading ":" cases)
and compare the port string exactly to "80" before setting c.serverName; ensure
IPv6 forms like "[::]:80" and short forms like ":80" are handled and skip
entries that don’t parse to a host:port pair.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
srv1, with graceful fallbackEnsureRoutesArray()that initializes the server's routes array via PATCH when it's null/missing, preventing Caddy 500 errors on route appendTest plan
go test -v -race ./...)devx caddy check --fixwith fresh Caddy config (null routes) succeedsdevx session createprovisions routes correctly with non-srv1 server names🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests