Uh oh!
There was an error while loading. Please reload this page.
feat(kubernetes): create SandboxClaims for matching warm pools - #2460
feat(kubernetes): create SandboxClaims for matching warm pools#2460grs wants to merge 15 commits into
Conversation
cdfe4be to
45f2384CompareSigned-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
…eation Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
rhuss
left a comment
There was a problem hiding this comment.
Review
Well-engineered PR with sound security design. No critical issues. The registration registry, activation flow, and claim lifecycle are all thoughtfully designed. The K8s-specific code refactor out of the gateway (~856 lines removed from k8s_sa.rs) is a major improvement.
What's done well:
- Auth trust chain is sound.
SupervisorBootstrapprincipal locked to a single RPC, explicitly rejected everywhere else. - Session-ID concurrency scheme in the registration registry is clean and well-tested.
- Claim creation is idempotent with deterministic naming and preconditioned deletes.
- Level-triggered activation with periodic relist, concurrency caps, and dedup by claim UID.
- Clean abstraction boundary: no K8s types leak through to the gateway.
- Bootstrap security: pod UID cross-checked against SA token extras, ownerReference chain validated, multi-version CRD support.
- Fail-safe behavior throughout: cache not-ready skips allocation, ambiguous matches skip allocation, GC failure creates safe ambiguity.
Inline comments below cover the specific findings.
| // The initial trial activates already-bound cold pods immediately. Later | ||
| // warm-pool stages keep this stream pending until a SandboxClaim adopts the | ||
| // registered pod. | ||
| rpc RegisterSupervisorPod(RegisterSupervisorPodRequest) |
There was a problem hiding this comment.
naming: RegisterSupervisorPod and PodActivationMessage sit in the driver-agnostic gateway proto. "Pod" leaks K8s terminology into the public API. The internal abstractions already use driver-neutral naming (instance_id, instance_name in SupervisorBootstrapIdentity). Consider renaming to RegisterSupervisor / SupervisorActivationMessage to match the existing ConnectSupervisor pattern. Docker warm containers or pre-booted VMs are plausible future extensions.
| # v1beta1 SandboxClaim resources when a compatible OpenShell-enabled | ||
| # SandboxWarmPool exists in the target namespace. | ||
| warmPooling: | ||
| enabled: true |
There was a problem hiding this comment.
defaults: Both warmPooling.enabled and profiles.enabled default to true. A fresh install on a cluster without Agent Sandbox CRDs will start watching extension CRDs that may not exist, causing noisy error logs. Consider defaulting to false (opt-in), or at minimum profiles.enabled: false while keeping warm-pool consumption enabled.
There was a problem hiding this comment.
On second thought, this concern is overcautious. Agent Sandbox CRDs are a hard dependency for OpenShell on Kubernetes, so the warm pool resource types will always be present on the cluster. Watching empty resource lists is cheap, and the cache correctly returns NotReady / NoMatch when nothing exists. Defaulting to true is fine here.
| - apiGroups: | ||
| - extensions.agents.x-k8s.io | ||
| resources: | ||
| - sandboxclaims |
There was a problem hiding this comment.
RBAC: create/delete on sandboxclaims is granted unconditionally, even when warmPooling.enabled: false. Contrast with sandboxtemplates/sandboxwarmpools below which correctly gate write verbs on profiles.enabled. Consider gating claim write verbs on warmPooling.enabled.
| - apiGroups: | ||
| - extensions.agents.x-k8s.io | ||
| resources: | ||
| - sandboxtemplates |
There was a problem hiding this comment.
RBAC scope: When profiles are enabled, create/patch/update/delete on sandboxtemplates and sandboxwarmpools applies cluster-wide. The profile reconciler only operates in one namespace. A namespaced Role would be more minimal. If the cluster-wide scope is intentional (warm pools spanning namespaces), a comment explaining why would help.
| ) -> Option<Arc<SandboxClaimActivationController>> { | ||
| std::env::var_os("KUBERNETES_SERVICE_HOST")?; | ||
| match kube::Client::try_default().await { |
There was a problem hiding this comment.
duplicate clients: Both kubernetes_supervisor_bootstrap_identity_provider (line 256) and kubernetes_sandbox_claim_activation_controller (here) call kube::Client::try_default() independently, creating two HTTP connection pools to the apiserver. Consider creating one client and sharing it.
| self.sandbox_index.remove_sandbox(sandbox.object_id()); | ||
| Err(Status::failed_precondition(status.message().to_string())) | ||
| } | ||
| Err(status) if status.code() == Code::Unavailable => { |
There was a problem hiding this comment.
orphaned records: When the driver returns Unavailable, the gateway preserves the Provisioning record. If the backend create actually failed, this could leak sandbox names in the index. Is there a staleness reconciler for provisioning records, or do they rely on the sandbox watcher to clean up?
| } | ||
| } | ||
| Err(Status::aborted( |
There was a problem hiding this comment.
observability: After exhausting all 3 retry attempts, this returns Status::aborted but doesn't log. A warn! here would provide operational visibility into rapid registration churn.
| activator: Arc<dyn SupervisorBootstrapActivator>, | ||
| claim: DynamicObject, | ||
| ) { | ||
| if tasks.len() >= ACTIVATION_MAX_CONCURRENCY { |
There was a problem hiding this comment.
observability: When the JoinSet is at ACTIVATION_MAX_CONCURRENCY (32), the claim event is silently dropped, relying on the 15-second resync to retry. A debug! log here would help operators diagnose delayed activations under load.
| data: | ||
| warm-pool.toml: | | ||
| version = 1 | ||
| workspace = "openshell" |
There was a problem hiding this comment.
consistency: All three example ConfigMaps use workspace = "openshell", but E2E tests and docs reference use workspace = "default". Users copying these examples verbatim will create warm pools that don't match sandbox creates in the default workspace. Either align the examples with the default workspace name, or add a comment explaining the workspace must match the operational workspace.
craig-kindo
commented
Aug 3, 2026
Hey, I reviewed this and found that it's not setting the The simplest fix is to set |
| Result<openshell_core::proto::PodActivationMessage, Status>, | ||
| >; | ||
| async fn register_supervisor_pod( |
There was a problem hiding this comment.
I have a question on this approach (pl feel free to correct me if I'm wrong). So as of now in the flow: warm-pool pod -> sandbox binding has no gw side verification or no restriction on who can trigger it. In the sense, in warm-pool path the gw does not decide which sandbox a pod becomes (it is taken from the pool, and is decided by the k8s driver from the cluster state). The gateway mints the token of trust based on the selected pod from the driver. The gateway does independently verify which pod it's talking to (TokenReview -> pod UID, matched to the registered stream). What it doesn't verify is that this pod is entitled to the sandbox_id the driver supplied.
This means, anyone who can create a sandbox CR (with any sandbox-id label), plus a SandboxClaim gets the trusted agent-sandbox controller to adopt a warm pod for it, and the gateway then mints a JWT for that sandbox_id. The gateway's cluster-wide watch on sandboxclaims means a claim created in any namespace, by anyone is honored.
There is no VAP, or restriction on who can create SandboxClaim or Sandbox (ideally it should only be the gateway). I'm not sure if enforcing that would be the right thing to do in the cluster, but is there any other way we could avoid this problem?
Probably the gateway should only honor a binding for an object it created (e.g., validate the CR UID against a gateway-side record, or mint the sandbox_id as a signed value it can verify), rather than trusting a label anyone can set
There was a problem hiding this comment.
Thanks @varshaprasad96 !
The assumption prior to this PR is that creation or mutation of Sandbox CRs in the gateway-managed namespace(s) is restricted to trusted parties. The assumption with this PR is that this restriction is extended to SandboxClaim, SandboxTemplate, and SandboxWarmPool. Additionally, TokenReview ensures that only pods running under the configured sandbox ServiceAccount are eligible for further checking. While
it would certainly be possible to strengthen the current validation to cover threats arising from loosening these assumptions, I believe that would be better handled as a separate PR covering both direct and
warm paths.
| let sandbox_id = sandbox_cr | ||
| .metadata | ||
| .labels | ||
| .as_ref() | ||
| .and_then(|labels| labels.get(LABEL_SANDBOX_ID)) | ||
| .filter(|id| !id.is_empty()) | ||
| .cloned() | ||
| .or_else(|| claim.sandbox_id.clone()) | ||
| .ok_or_else(|| { | ||
| "SandboxClaim and selected Sandbox are missing OpenShell sandbox id label".to_string() | ||
| })?; |
There was a problem hiding this comment.
sandbox_id silent fallback masks disagreement between the Sandbox CR label and the SandboxClaim
.or_else(...) only fires when the CR label is None. So when the CR label and claim.sandbox_id are both present but differ, there is no comparison and no error — the CR label silently wins and the claim's value is discarded.
This is asymmetric with the cold path, which already treats this class of disagreement as a hard error: validate_sandbox_owner_binding in bootstrap.rs rejects when the pod annotation does not match the live Sandbox CR label (if actual_sandbox_id != sandbox_id { return Err(...) }). A two-source disagreement here is exactly the fingerprint of a planted/forged object, a stale or reused claim, or a controller bug — and it is masked rather than surfaced.
There is test coverage for the CR-label case and the claim-fallback case (activation_request_uses_claim_sandbox_id_when_sandbox_lacks_label), but none for the both-present-and-different case — the code has no branch for it.
Suggested fix: when both sources are present, require equality and error otherwise (mirror the cold path).
let cr_label = sandbox_cr
.metadata.labels.as_ref().and_then(|labels| labels.get(LABEL_SANDBOX_ID)).filter(|id| !id.is_empty()).cloned();let sandbox_id = match(cr_label, claim.sandbox_id.clone()){(Some(a),Some(b))if a != b => {returnErr(format!("sandbox id mismatch: Sandbox CR label {a} != SandboxClaim {b}"));}(Some(a), _) => a,(None,Some(b)) => b,(None,None) => {returnErr("SandboxClaim and selected Sandbox are missing OpenShell sandbox id label".to_string(),);}};There was a problem hiding this comment.
Thanks @varshaprasad96! Well spotted, this is indeed a bug. I'll push a fix.
grs
commented
Aug 4, 2026
Thanks @craig-kindo! You are right, I missed that but will fix it and include the fix in an upcoming push. |
Summary
Add transparent Kubernetes warm-pool allocation by matching sandbox create requests to OpenShell-enabled
SandboxWarmPooltemplates and creatingSandboxClaimresources when a compatible pool exists. The PRalso moves Kubernetes supervisor pod bootstrap and claim activation into the Kubernetes driver path so warm pods can register before assignment and be activated once a claim selects them.
Related Issue
Closes#2157
Changes
IssueSandboxTokento the streamingRegisterSupervisorPodRPC so warm pods can establish a gateway-held registration before they are assigned to a sandbox. This means supervisor path is the same for warm or cold initialisation.SandboxWarmPoolandSandboxTemplatefingerprints.SandboxClaimresources instead of directSandboxresources when exactly one compatible warm pool exists.SandboxClaimactivation handling that validates the selectedSandboxand pod before issuing a sandbox token.SandboxWarmPooland associatedSandboxTemplatein response to presence of labeledConfigMapwith toml describing the sandbox parameters it should match. Included some exampleConfigMaps.Examples
The easiest way to try it out is to use one (or more) of the examples under
examples/kubernetes-warm-pool-config/:default-configmap.yamlwill create a warm pool that will be matched by sandbox create requests with no explicit arguments,env-foo-configmap.yamlby requests with--env FOO=barandcpu-0-2-configmap.yamlby requests with--cpu 0.2. Obviously you can also modify these for other settings. The examples set the pool size to 1; if you want to test rapid creation of multiple sandboxes you will need to increase that.Testing
mise run pre-commitpassesChecklist