[management] Add reverse proxy services REST client#5454
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded APIError and improved error handling in the management REST client; introduced and wired three new reverse-proxy REST API wrappers on Client: ReverseProxyServicesAPI (CRUD), ReverseProxyClustersAPI (List), and ReverseProxyDomainsAPI (List/Create/Delete/Validate). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shared/management/client/rest/reverse_proxy_services.go (1)
17-87: Add focused unit tests for CRUD request construction and error paths.Please add tests that validate HTTP method/path, payload serialization, and parse/error behavior (
nil, errcontract forGet/Create/Updateon failures). This new client surface is otherwise regression-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/client/rest/reverse_proxy_services.go` around lines 17 - 87, Add focused unit tests for ReverseProxyServicesAPI methods (List, Get, Create, Update, Delete) that assert the HTTP method and URL passed to the underlying client (a.c.NewRequest), validate request body serialization for Create and Update (matching api.PostApiReverseProxiesServicesJSONRequestBody and api.PutApiReverseProxiesServicesServiceIdJSONRequestBody), and cover error paths where NewRequest or parseResponse returns an error to ensure callers receive (nil, err) for Get/Create/Update and error for Delete; use a fake/mock implementation of the client (the c field) to capture method, path, headers, and body, and stub parseResponse or http response bodies to exercise both successful parsing and failure cases. Ensure tests reference the API methods by name (ReverseProxyServicesAPI.List/Get/Create/Update/Delete) and verify resp.Body is closed when non-nil if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/management/client/rest/reverse_proxy_services.go`:
- Around line 38-39: The three methods Get, Create, and Update currently return
a non-nil *api.Service together with an error because they unconditionally do
"ret, err := parseResponse[api.Service](resp); return &ret, err"; change each to
check the parse error and return nil on failure: call "ret, err :=
parseResponse[api.Service](resp)" then "if err != nil { return nil, err }" and
finally "return &ret, nil" so callers receive a nil pointer whenever an error is
returned.
- Line 31: The REST client concatenates path parameters directly (e.g., in
reverse_proxy_services.go where a.c.NewRequest is called with
"/api/reverse-proxies/services/"+serviceID); fix by applying url.PathEscape to
all path parameters before building paths (serviceID, userID, tokenID, peerID,
zoneID, etc.) or centralize into a helper like buildPath(segment, id) that calls
url.PathEscape, and update all occurrences across the client methods to use the
escaped values to prevent reserved-character injection.
---
Nitpick comments:
In `@shared/management/client/rest/reverse_proxy_services.go`:
- Around line 17-87: Add focused unit tests for ReverseProxyServicesAPI methods
(List, Get, Create, Update, Delete) that assert the HTTP method and URL passed
to the underlying client (a.c.NewRequest), validate request body serialization
for Create and Update (matching api.PostApiReverseProxiesServicesJSONRequestBody
and api.PutApiReverseProxiesServicesServiceIdJSONRequestBody), and cover error
paths where NewRequest or parseResponse returns an error to ensure callers
receive (nil, err) for Get/Create/Update and error for Delete; use a fake/mock
implementation of the client (the c field) to capture method, path, headers, and
body, and stub parseResponse or http response bodies to exercise both successful
parsing and failure cases. Ensure tests reference the API methods by name
(ReverseProxyServicesAPI.List/Get/Create/Update/Delete) and verify resp.Body is
closed when non-nil if applicable.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shared/management/client/rest/client.goshared/management/client/rest/reverse_proxy_services.go
lixmal
force-pushed
the
reverse-proxy-rest-client
branch
from
5523148 to
6f4964a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shared/management/client/rest/reverse_proxy_services.go (1)
55-56:⚠️ Potential issue | 🟡 MinorReturn
nil, erron parse failure instead of&ret, errinCreateandUpdate.Same issue as flagged for
Get: returning a non-nil pointer together with an error violates Go conventions. Apply the same fix pattern to both methods.🔧 Proposed fix for Create (lines 55-56)
ret, err := parseResponse[api.Service](resp) - return &ret, err + if err != nil { + return nil, err + } + return &ret, nil🔧 Proposed fix for Update (lines 72-73)
ret, err := parseResponse[api.Service](resp) - return &ret, err + if err != nil { + return nil, err + } + return &ret, nilAlso applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/client/rest/reverse_proxy_services.go` around lines 55 - 56, In Create and Update in reverse_proxy_services.go you currently call parseResponse[api.Service](resp) and always return &ret, err; change the return so that when err != nil you return nil, err instead of &ret, err, and only return &ret, nil when parseResponse succeeded—adjust the return logic around the parseResponse call (look for the ret variable and the lines returning &ret, err) to follow Go conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/management/client/rest/reverse_proxy_domains.go`:
- Around line 42-43: The current return of "&ret, err" after calling
parseResponse[api.ReverseProxyDomain](resp) can return a non-nil pointer when an
error occurred; update the code so you check the err from parseResponse and
return "nil, err" on failure, and only return "&ret, nil" when err is nil (i.e.,
replace the single-line "ret, err :=
parseResponse[api.ReverseProxyDomain](resp); return &ret, err" with an explicit
if err != nil { return nil, err } return &ret, nil).
---
Duplicate comments:
In `@shared/management/client/rest/reverse_proxy_services.go`:
- Around line 55-56: In Create and Update in reverse_proxy_services.go you
currently call parseResponse[api.Service](resp) and always return &ret, err;
change the return so that when err != nil you return nil, err instead of &ret,
err, and only return &ret, nil when parseResponse succeeded—adjust the return
logic around the parseResponse call (look for the ret variable and the lines
returning &ret, err) to follow Go conventions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shared/management/client/rest/client.goshared/management/client/rest/reverse_proxy_clusters.goshared/management/client/rest/reverse_proxy_domains.goshared/management/client/rest/reverse_proxy_services.go
lixmal
force-pushed
the
reverse-proxy-rest-client
branch
from
6f4964a to
ede9115
Compare
lixmal
force-pushed
the
reverse-proxy-rest-client
branch
from
ede9115 to
4e16483
Compare
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/management/client/rest/client.go (1)
20-23: Consider including status code in error message for better diagnostics.The
Error()method only returns the message, which may lose useful context when the error is logged or wrapped. Including the status code helps with debugging.💡 Optional enhancement
// Error implements the error interface. func (e *APIError) Error() string { - return e.Message + return fmt.Sprintf("%d: %s", e.StatusCode, e.Message) }This would require adding
"fmt"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/client/rest/client.go` around lines 20 - 23, The APIError.Error() method currently returns only e.Message; update it to include the HTTP/status code for better diagnostics by returning a formatted string like "status: message" (e.g., using fmt.Sprintf) so logs and wrapped errors show both code and message; add the "fmt" import and modify the Error() method in the APIError type to include e.Status (or the correct status field name) in the returned string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shared/management/client/rest/client.go`:
- Around line 20-23: The APIError.Error() method currently returns only
e.Message; update it to include the HTTP/status code for better diagnostics by
returning a formatted string like "status: message" (e.g., using fmt.Sprintf) so
logs and wrapped errors show both code and message; add the "fmt" import and
modify the Error() method in the APIError type to include e.Status (or the
correct status field name) in the returned string.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shared/management/client/rest/client.goshared/management/client/rest/reverse_proxy_clusters.goshared/management/client/rest/reverse_proxy_domains.goshared/management/client/rest/reverse_proxy_services.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/management/client/rest/reverse_proxy_clusters.go
bcmmbaga
approved these changes
Feb 27, 2026
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
2 participants
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.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Bug Fixes / Improvements