fix(ctl): add HTTP client timeout to prevent kmeshctl commands hanging - #1944
Conversation
Signed-off-by: Norway-02 <anshulkhetade02@gmail.com>
|
Welcome @Norway-02! It looks like this is your first PR to kmesh-net/kmesh 🎉 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR prevents kmeshctl admin commands from hanging indefinitely by introducing a shared HTTP client with a default timeout and updating admin request call sites to use it, plus fixing an error path in ctl/dump when port-forward setup fails.
Changes:
- Add
utils.NewAdminHTTPClient()with a default 10s timeout (DefaultAdminHTTPTimeout). - Update multiple
ctl/commands to use the timeout-configured HTTP client instead of the default/no-timeout client. - Add unit tests around the new admin HTTP client behavior and adjust dump behavior on port-forward failures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ctl/utils/utils.go | Introduces a shared admin HTTP client with a default timeout constant. |
| ctl/utils/utils_test.go | Adds tests intended to validate the admin HTTP client timeout configuration/behavior. |
| ctl/version/version.go | Switches version request from http.Get to the admin HTTP client. |
| ctl/log/log.go | Switches GET/POST calls to use the admin HTTP client timeout. |
| ctl/monitoring/monitoring.go | Switches monitoring request client to the admin HTTP client timeout. |
| ctl/authz/authz.go | Switches authz requests to the admin HTTP client timeout. |
| ctl/dump/dump.go | Uses admin HTTP client timeout and exits on port-forward start failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| if err := fw.Start(); err != nil { | ||
| log.Errorf("failed to start port forwarder for Kmesh daemon pod %s: %v", podName, err) | ||
| os.Exit(1) | ||
| } |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| time.Sleep(200 * time.Millisecond) | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| customClient := &http.Client{ | ||
| Timeout: 50 * time.Millisecond, | ||
| } | ||
|
|
||
| resp, err := customClient.Get(server.URL) | ||
| assert.Error(t, err) | ||
| assert.Nil(t, resp) |
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/retest |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiZhenCheng9527 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
kmeshctl log,kmeshctl version,kmeshctl dump,kmeshctl authz, andkmeshctl monitoringsend administrative requests to a port-forwardedkmesh-daemonendpoint using the Go default HTTP client (http.Getor&http.Client{}with zero timeout).If the port-forward tunnel stalls or the daemon is unresponsive, these commands block indefinitely with no feedback to the user. Additionally,
ctl/dump/dump.gocontinued execution after port forwarder startup failures instead of exiting.This PR introduces
utils.NewAdminHTTPClient()inctl/utilswith a default 10-second timeout (DefaultAdminHTTPTimeout), updateskmeshctladmin call sites to use this client, and ensuresctl/dumpexits cleanly on port-forward failure. Unit tests are added inctl/utils/utils_test.goto verify timeout behavior.Which issue(s) this PR fixes:
Fixes #1781
Special notes for your reviewer:
ctl/.go test ./ctl/...andmake format.Does this PR introduce a user-facing change?: