Conversation
✅ Deploy Preview for agent-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @wllbo! |
|
Hi @wllbo. 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 Regular contributors should join the org to skip this step. 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. |
| "sigs.k8s.io/agent-sandbox/clients/go/sandbox" | ||
| ) | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Thank you for this PR. We are migrating the existing Python client to manage multiple Sandboxes: https://github.com/kubernetes-sigs/agent-sandbox/pull/361/changes.
The work is in progress: #382
I highly encourage to simulate a similar pattern for Go client. There are lot of re-usable components you have already defined which is great.
There was a problem hiding this comment.
@wllbo - @SHRUTI6991's change is in. You can make the changes.
There was a problem hiding this comment.
Thanks for the heads up @aditya-shantanu! I went ahead and rebased on main after @SHRUTI6991's change merged and pushed the refactor (eac94eb).
Now the Go SDK follows the same handle pattern: Sandbox handle, shareable K8sHelper, ConnectionStrategy interface, and separate Commands/Files engines. I also added Disconnect(ctx) and Handle/Info interfaces for mocking.
I'll add the remaining parts of KEP #361 (SandboxClient factory, re-attach, ProcessSystem, lifecycle states) once they're in the Python SDK so I can match the design.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wllbo 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 |
clients/go/sandbox/commands.go
Outdated
| return nil, fmt.Errorf("%s: run failed: %w", c.errPrefix(), err) | ||
| } | ||
| defer resp.Body.Close() | ||
| defer func() { _, _ = io.Copy(io.Discard, resp.Body) }() |
There was a problem hiding this comment.
defer func() { _, _ = io.Copy(io.Discard, resp.Body) }() is used in multiple files. This defer executes before the body is closed and will synchronously download the entire remainder of any response payload. If a user attempts to read a file that exceeds MaxDownloadSize, the SDK will correctly catch the limit, but the defer will then stall the goroutine while it silently downloads the rest of the file into /dev/null.
Consider either removing (for error paths) or bounding it (for success paths) with io.LimitReader. It's cheaper to let the transport close the TCP connection than to drain a large, unwanted payload.
There was a problem hiding this comment.
Good catch, I've now capped drains at 4KB and skip them on error paths
Closes #227
Summary
Go client for agent-sandbox at
clients/go/sandbox. Builds on the clientsets from #233, adding the user facing layer on top.API lands close to what was proposed in #227, handles claim lifecycle, connectivity, and cleanup so callers don't have to work with the K8s API directly. Covers the full core SandboxClient surface from the Python SDK: run, write, read, list, exists, lifecycle management, and opt-in OTel tracing. Doesn't include the extensions yet (ComputerUseSandbox, podsnapshot) but I think those can also come as follow-ups.
Three connectivity modes depending on what you set in Options:
Notes
Exported a
Clientinterface separate from the concreteSandboxClientso consumers can mock it in tests. Identity accessors (ClaimName, SandboxName, PodName, Annotations) are on a separateSandboxInfointerface so adding new accessors later isn't a breaking change for anyone who's written a mock.HTTP operations go through a retrying transport (exponential backoff + jitter, 6 attempts, per-attempt timeout). Only retries 5xx and connection errors (4xx are not retried since they indicate a client problem).
Port-forward mode uses native SPDY via client-go instead of shelling out to kubectl. A background goroutine monitors the tunnel and clears client state immediately on death, so subsequent ops fail with ErrNotReady instead of blocking until timeout.
If Close() can't reach the API server to delete the claim, the client hangs onto the claim name so Close() can be retried. Calling Open() on a client with a dangling claim returns ErrOrphanedClaim so callers don't silently leak resources.
Testing
-tags=integration), all three connectivity modesgo vet/staticcheckclean