🐛 (CLI): Add context timeout to HTTP calls in alpha update#5695
Conversation
k8s-ci-robot
added
needs-ok-to-test
|
Hi @SebTardif. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
|
|
||
| func fetchLatestRelease() (string, error) { | ||
| resp, err := http.Get("https://api.github.com/repos/kubernetes-sigs/kubebuilder/releases/latest") | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
There was a problem hiding this comment.
30 secs is really enough?
Should we add a bigger timeout?
What about 1 or 2 minutes?
There was a problem hiding this comment.
Done. Increased the timeout to 2 minutes for all three HTTP call sites.
|
@camilamacedo86 The 30s is for That said, happy to bump the API call timeout if you have a preferred value. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
This PR adds bounded contexts to two GitHub HTTP GET calls in the alpha update flow to avoid indefinite blocking during release lookup and binary download.
Changes:
- Adds a 30-second timeout to fetching the latest Kubebuilder release.
- Adds a 2-minute timeout to downloading a Kubebuilder release binary.
- Replaces direct
http.Getcalls with context-aware requests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/cli/alpha/internal/update/prepare.go |
Adds a context timeout around the GitHub latest-release API request. |
internal/cli/alpha/internal/update.go |
Adds a context timeout around downloading the Kubebuilder binary from GitHub releases. |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cancel() |
There was a problem hiding this comment.
Could we address this one?
Could you please ensure that we have only 1 commit after the changes ? (squash)
There was a problem hiding this comment.
Done. Added a 2-minute context timeout to validateBinaryAvailability (the http.Head call) as well. Everything is squashed into a single commit.
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
Could we address this one?
There was a problem hiding this comment.
Addressed in the same update. All three HTTP calls (fetchLatestRelease, downloadKubebuilderBinary, validateBinaryAvailability) now use a 2-minute context timeout.
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Replace bare http.Get and http.Head calls with context-bounded requests using http.NewRequestWithContext so that alpha update never hangs when GitHub is unreachable. Affected call sites: - fetchLatestRelease (GET, 2 min timeout) - downloadKubebuilderBinary (GET, 2 min timeout) - validateBinaryAvailability (HEAD, 2 min timeout) Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
SebTardif
force-pushed
the
fix/http-get-no-timeout
branch
from
b3d62cb to
2e38fe5
Compare
k8s-ci-robot
added
size/M
|
/retest pull-kubebuilder-e2e-k8s-1-36-0 |
|
/test pull-kubebuilder-e2e-k8s-1-36-0 |
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, SebTardif 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 |
k8s-ci-robot
added
the
approved
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override pull-kubebuilder-e2e-k8s-1-36-0 |
|
@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-36-0 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What
Replace
http.Get(no timeout) withhttp.NewRequestWithContextin two call sites in thealpha updatecommand.Why
Both call sites use
http.DefaultClientwith no timeout. If GitHub is unreachable, the CLI blocks indefinitely with no way to cancel. The newerhelpers/download.goin the same package already usescontext.WithTimeoutcorrectly for the same kind of download operation.Call sites fixed:
downloadKubebuilderBinary(update.go:133): downloads a release binary from GitHub. Now uses a 2-minute timeout, matchinghelpers/download.go:71.fetchLatestRelease(prepare.go:95): fetches the latest release tag from the GitHub API. Now uses a 30-second timeout (generous for a single JSON response).How
http.Get(url)withhttp.NewRequestWithContext(ctx, http.MethodGet, url, nil)+http.DefaultClient.Do(req)context.WithTimeoutwith appropriate durations for each call sitehelpers/download.go:71-81Both call sites were introduced in #4871 (2025-06-12).