feat(python): expose additionalPodMetadata in sandbox client#979
Conversation
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
✅ Deploy Preview for agent-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mvanhorn 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 |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @mvanhorn. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughBoth ChangesPod labels/annotations propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
k8s-ci-robot
added
size/L
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
janetkuo
added
ok-to-test
There was a problem hiding this comment.
Pull request overview
This PR adds first-class Python SDK support for setting SandboxClaim.spec.additionalPodMetadata so callers can stamp labels/annotations onto the Sandbox Pod (not just the claim object). This aligns the Python client with existing CRD/controller capabilities and enables in-sandbox identity/tenant signaling via the Kubernetes Downward API.
Changes:
- Add
pod_labelsandpod_annotationskeyword-only parameters toSandboxClient.create_sandboxandAsyncSandboxClient.create_sandbox, threading them through to claim creation. - Extend sync/async Kubernetes manifest builders to emit
spec.additionalPodMetadataonly when provided. - Add/adjust unit tests and update Python client README to document the claim-labels vs pod-metadata distinction.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| clients/python/agentic-sandbox-client/README.md | Documents claim-level labels vs pod-level pod_labels / pod_annotations with an example. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/sandbox_client.py | Adds new create_sandbox kwargs, builds additionalPodMetadata payload, validates pod_labels, and passes through to claim creation. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_sandbox_client.py | Async parity for new kwargs and pod-metadata threading/validation behavior. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/k8s_helper.py | Adds optional pod_metadata to create_sandbox_claim and emits spec.additionalPodMetadata when set. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/async_k8s_helper.py | Async parity for manifest emission of spec.additionalPodMetadata. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_sandboxclient.py | Updates signature expectations and adds coverage for pod metadata plumbing + invalid pod-label validation. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_async_sandboxclient.py | Async client tests for pod metadata plumbing + invalid pod-label validation. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_k8s_helper.py | Tests that additionalPodMetadata is included/omitted appropriately in sync manifests. |
| clients/python/agentic-sandbox-client/k8s_agent_sandbox/test/unit/test_async_k8s_helper.py | Tests that additionalPodMetadata is included/omitted appropriately in async manifests. |
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
Overall looks good to me, a few non-blocking comments in-line.
| SandboxClient._validate_label_name(value, f"value '{value}' for key '{key}'") | ||
|
|
||
| @classmethod | ||
| def _build_pod_metadata(cls, pod_labels: dict[str, str] | None, pod_annotations: dict[str, str] | None) -> dict | None: |
There was a problem hiding this comment.
Nit (non-blocking): _build_pod_metadata is duplicated in async_sandbox_client.py (identical body/docstring; the signature is just wrapped across multiple lines). It and _validate_labels (plus its _validate_label_name/regex-constant helpers) could move to utils.py to de-dup across both clients.
| of *now + shutdown_after_seconds* (UTC) and a ``shutdownPolicy`` | ||
| of ``"Delete"``, so the controller auto-deletes the claim on | ||
| expiry. Must be a positive integer. | ||
| pod_labels: Optional labels stamped onto the running Sandbox **Pod** |
There was a problem hiding this comment.
"readable from inside the sandbox through the Downward API" is accurate, but it's worth noting that client-side validation only checks label syntax (RFC-1123) — it does not replicate the controller's domain allow-list or system-label restrictions, so a client-valid pod_labels value can still be rejected server-side.
A half-sentence clarifying that client validation is fail-fast (not a guarantee of admission) would help. Related, a system-reserved key isn't rejected — it's silently dropped when the Pod is rendered (only a V(1) debug log, no error), so such a key would simply be absent on the Pod with no signal in the client.
| asyncio.run(main()) | ||
| ``` | ||
|
|
||
| ### 7. Labels and Pod Metadata |
There was a problem hiding this comment.
Optional: a couple of behavioral caveats worth a line here.
-
A
pod_label/pod_annotationwhose key is already defined on the warmpool template with a different value is rejected by the controller's "No Overrides" rule, so the reconcile errors — there's no client-side signal for this, so a caller may see a stuck claim with the reason only in controller logs. -
Once
pod_labels/pod_annotationsare set, the controller treats a template-fetch failure as fatal (reconcile retried) rather than a silent best-effort skip, and may issue an extra Sandbox update on the first reconcile after adoption.
Neither is a problem, just non-obvious when debugging.
Move the shared label-validation and pod-metadata assembly out of both SandboxClient and AsyncSandboxClient into a single pod_metadata module so the sync and async clients no longer carry identical copies of _build_pod_metadata, _validate_labels, and _validate_label_name. Behavior and error messages are unchanged; the migrated unit tests call the module-level functions. Document that client-side validation only checks RFC-1123 label syntax and does not replicate the controller's domain allow-list or system-label restrictions, both in the build_pod_metadata docstring and the README. Add a README note that a pod_label/pod_annotation whose key already exists on the warmpool template with a different value is rejected by the controller's No Overrides rule. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Thanks @igooch, all three addressed in 127dfbb:
Appreciate the careful review. |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: igooch, mvanhorn 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 |
kubernetes-prow
Bot
added
the
approved
…tes-sigs#979) * feat(python): expose additionalPodMetadata in sandbox client Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * address review: dedup pod-metadata helpers, document validation scope Move the shared label-validation and pod-metadata assembly out of both SandboxClient and AsyncSandboxClient into a single pod_metadata module so the sync and async clients no longer carry identical copies of _build_pod_metadata, _validate_labels, and _validate_label_name. Behavior and error messages are unchanged; the migrated unit tests call the module-level functions. Document that client-side validation only checks RFC-1123 label syntax and does not replicate the controller's domain allow-list or system-label restrictions, both in the build_pod_metadata docstring and the README. Add a README note that a pod_label/pod_annotation whose key already exists on the warmpool template with a different value is rejected by the controller's No Overrides rule. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --------- Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
What this PR does / why we need it:
The
SandboxClaimCRD already supportsspec.additionalPodMetadata, and theclaim controller propagates its
labelsandannotationsonto the runningSandbox Pod. Until now the Python client had no way to set it: the existing
labelsargument oncreate_sandboxonly wires the claim object's ownmetadata.labels, which lands on theSandboxClaimresource and never reachesthe Pod.
This PR adds two new optional, keyword-only parameters to both the sync
(
SandboxClient) and async (AsyncSandboxClient)create_sandbox:pod_labels->spec.additionalPodMetadata.labelspod_annotations->spec.additionalPodMetadata.annotationsThey flow from the public client through
_create_claiminto thecreate_sandbox_claimmanifest builders (k8s_helper/async_k8s_helper),mirroring the existing
labels/lifecycleplumbing rather than introducing anew pattern. With this, a caller can stamp a tenant / client identifier onto the
Pod and the running sandbox can read its own identity via the Downward API.
Details:
pod_labelsreuse the existing_validate_labelsRFC-1123 validation so badkeys/values fail fast client-side (the controller validates server-side too).
additionalPodMetadatais written whenneither
pod_labelsnorpod_annotationsis provided.labelsand Pod-levelpod_labels/pod_annotations.Which issue(s) this PR is related to:
Fixes #914
Ref #914
Testing
spec.additionalPodMetadata; key omitted when unset)._create_claim/create_sandbox_claimas the expectedpod_metadatadict;invalid pod-label keys raise the same validation error as claim labels).
_create_claim/create_sandbox_claimcall signature.python -m pytest k8s_agent_sandbox/test/unitpasses (223 passed).AI was used for assistance.
Release Note
Summary by CodeRabbit
New Features
pod_labelsandpod_annotationsparameters to the Python sandbox clients to apply metadata directly to the running Sandbox Pod.Documentation