fix: propagate url.Parse error in buildURL to prevent nil pointer pan…#270
fix: propagate url.Parse error in buildURL to prevent nil pointer pan…#270sicaario wants to merge 6 commits intovolcano-sh:mainfrom
Conversation
…ic in reverse proxy Signed-off-by: sicaario <hrmnp8@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a router panic by properly propagating url.Parse errors from buildURL so malformed sandbox endpoints are handled as normal errors rather than causing a nil-pointer panic inside the reverse proxy.
Changes:
- Propagate
url.Parseerrors by changingbuildURLto return(*url.URL, error). - Introduce
errNoEntryPointsentinel to distinguish “no entry points” (404) from “invalid endpoint config” (500). - Update
forwardToSandboxto return 500 on invalid endpoint configuration instead of relying on gin recovery.
| if errors.Is(err, errNoEntryPoint) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) | ||
| } else { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "invalid sandbox endpoint configuration"}) | ||
| } |
There was a problem hiding this comment.
The PR description says the 500 response will include a “descriptive message”, but the current body always returns the generic string "invalid sandbox endpoint configuration". If the intent is to be descriptive to clients, consider including the underlying error message (or a sanitized subset) in the JSON response; alternatively, update the PR description to reflect that the detailed reason is only available in logs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the router's URL construction logic. It introduces a specific errNoEntryPoint error, updates buildURL to validate parsed URLs, and refines forwardToSandbox to distinguish between missing entry points (404) and invalid configurations (500). I have no feedback to provide.
…ole struct Signed-off-by: sicaario <hrmnp8@gmail.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
==========================================
+ Coverage 35.60% 43.65% +8.04%
==========================================
Files 29 30 +1
Lines 2533 2623 +90
==========================================
+ Hits 902 1145 +243
+ Misses 1505 1355 -150
+ Partials 126 123 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| c.JSON(http.StatusNotFound, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| if errors.Is(err, errNoEntryPoint) { |
There was a problem hiding this comment.
Since this branch is newly distinguishing missing entry points from malformed URLs, it would be good to add a small test that covers EntryPoints == nil/empty and asserts we now return 404 here.
There was a problem hiding this comment.
Added TestForwardToSandbox_NoEntryPoints in the latest commit (02fae19) which sets EntryPoints: [] and asserts a 404 Not Found response, covering the errNoEntryPoint path.
| if errors.Is(err, errNoEntryPoint) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) | ||
| } else { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "invalid sandbox endpoint configuration"}) |
There was a problem hiding this comment.
The new behavior makes sense, but the response body here is still fairly generic. Since the PR description mentions a more descriptive error, I suggest to include a sanitized parse failure in the JSON response
There was a problem hiding this comment.
@acsoto Updated in 02fae19 — the 500 response now includes the sanitized parse error via fmt.Sprintf("invalid sandbox endpoint configuration: %v", err), so the caller gets context like invalid endpoint URL "://invalid-url": instead of the generic message.
There was a problem hiding this comment.
Thanks, the earlier comments are addressed. I think there is still one edge case left: \url.Parse may succeed for inputs like "" or "http://", but those are still not usable proxy targets. It would be better to validate u.Scheme and u.Host in buildURL so these cases also return 500 as invalid endpoint configuration, and add a small test for that.`
… points returning 404 Signed-off-by: sicaario <hrmnp8@gmail.com>
| u, err := url.Parse(endpoint) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid endpoint URL %q: %w", endpoint, err) | ||
| } |
There was a problem hiding this comment.
url.Parse can return a non-nil *url.URL without error for inputs that are still unusable for httputil.NewSingleHostReverseProxy (e.g., empty endpoint, http:// with empty host, missing scheme/host). To fully prevent runtime failures, validate the parsed URL has the required components (at least u.Scheme and u.Host), and return an error when they’re missing.
| } | |
| } | |
| if u.Scheme == "" || u.Host == "" { | |
| return nil, fmt.Errorf("invalid endpoint URL %q: missing scheme or host", endpoint) | |
| } |
| func TestForwardToSandbox_NoEntryPoints(t *testing.T) { | ||
| setupEnv() | ||
| defer teardownEnv() |
There was a problem hiding this comment.
This test mutates process-global environment via setupEnv/teardownEnv, which can cause cross-test interference and flakiness if the suite uses parallel tests. Consider switching to t.Setenv(...) within the test to scope env changes to the test lifecycle and avoid global cleanup ordering issues.
Signed-off-by: sicaario <hrmnp8@gmail.com>
Signed-off-by: sicaario <hrmnp8@gmail.com>
| klog.Errorf("Failed to get sandbox access address %s: %v", sandbox.SandboxID, err) | ||
| c.JSON(http.StatusNotFound, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| if errors.Is(err, errNoEntryPoint) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}) | ||
| } else { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("invalid sandbox endpoint configuration: %v", err)}) | ||
| } |
There was a problem hiding this comment.
The 500 response here includes the detailed endpoint parsing error (and potentially the raw configured endpoint string) in the user-facing JSON. Given handleGetSandboxError masks 500s as "internal server error", consider logging the detailed error but returning a more generic 500 message (optionally with a stable error code) to avoid leaking internal configuration details.
|
|
||
| // Compare key components instead of whole struct — ParsePKCS1PrivateKey may | ||
| // not precompute Dp/Dq/Qinv identically to the original key. | ||
| assert.Equal(t, manager.privateKey.PublicKey.N, privateKey.PublicKey.N, "Public key N should match") |
There was a problem hiding this comment.
In this test, assert.Equal on *big.Int fields (e.g., PublicKey.N) relies on deep structural equality and can be more brittle than value comparison. For consistency with the checks below (using Cmp), consider comparing N via Cmp as well.
| assert.Equal(t, manager.privateKey.PublicKey.N, privateKey.PublicKey.N, "Public key N should match") | |
| assert.Equal(t, 0, manager.privateKey.PublicKey.N.Cmp(privateKey.PublicKey.N), "Public key N should match") |
| if u.Scheme == "" { | ||
| return nil, fmt.Errorf("invalid endpoint URL %q: missing scheme", endpoint) | ||
| } | ||
| url, _ := url.Parse(endpoint) |
There was a problem hiding this comment.
donot we have already guranteed this always succeed in
func buildSandboxInfo(sandbox *sandboxv1alpha1.Sandbox, podIP string, entry *sandboxEntry) *types.SandboxInfo {
createdAt := sandbox.GetCreationTimestamp().Time
expiresAt := createdAt.Add(DefaultSandboxTTL)
if sandbox.Spec.ShutdownTime != nil {
expiresAt = sandbox.Spec.ShutdownTime.Time
}
accesses := make([]types.SandboxEntryPoint, 0, len(entry.Ports))
for _, port := range entry.Ports {
accesses = append(accesses, types.SandboxEntryPoint{
Path: port.PathPrefix,
Protocol: string(port.Protocol),
Endpoint: net.JoinHostPort(podIP, strconv.Itoa(int(port.Port))),
})
There was a problem hiding this comment.
BuildSandboxInfo does guarantee the format. The validation still handles the case where Protocol is empty: url.Parse("host:port") succeeds but parses it as scheme="host" with an empty Host, giving a silently unusable proxy target. Also keeps buildURL safe for any future callers. Happy to move the validation upstream if you prefer though.
There was a problem hiding this comment.
I cannot get what you mean by but parses it as scheme="host" with an empty Host,.
From the api object validation, we have a hard limit that it must be set to http or https now
There was a problem hiding this comment.
@hzxuzhonghu to address this buildURL now takes protocol as a separate parameter and prepends it explicitly before parsing — so "HTTP" + "host:port" becomes "http://host:port" before url.Parse is called. This removes the misparse concern entirely.
The scheme == "" check is still kept because SandboxEntryPoint.Protocol is a plain string (not the API-validated ProtocolType), and the session manager can produce entries with an empty Protocol field — as seen in several tests. So it's a real path
Signed-off-by: sicaario <hrmnp8@gmail.com>
| } | ||
|
|
||
| func buildURL(protocol, endpoint string) *url.URL { | ||
| func buildURL(protocol, endpoint string) (*url.URL, error) { |
There was a problem hiding this comment.
If iirc， the endpoint is constructed by us in buildSandboxInfor, it is podIP:port. And also the protocol can be only http and https now.
So i think this is actually not worth verifying.
What type of PR is this?
/kind bug
What this PR does / why we need it:
buildURLin the router was silently discarding the error returned byurl.Parseusing a blank identifier (_). This meant that when a sandbox entry point contained a malformed or empty endpoint string,url.Parsewould return anil*url.URLwhich was then passed directly tohttputil.NewSingleHostReverseProxy. Passing anilURL to the reverse proxy causes a panic at request time, which was only masked by gin's built-in recovery middleware returning a 500 — not proper error handling.This PR fixes the issue by:
buildURLto return(*url.URL, error)and propagating the parse error to the callererrNoEntryPointsentinel to distinguish between "no entry points configured" (404) and "malformed endpoint URL" (500) inforwardToSandbox500 Internal Server Errorwith a descriptive message instead of relying on panic recoveryWhich issue(s) this PR fixes:
Fixes #269
Special notes for your reviewer:
The existing test
TestForwardToSandbox_InvalidEndpointwas already covering this scenario but was passing for the wrong reason — it expected 500 because gin's panic recovery was catching the nil pointer dereference. The test still passes after this fix, but now tests legitimate error handling instead of panic recovery behaviour.Does this PR introduce a user-facing change?: