mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-21 06:11:31 +08:00
fix(sandbox): fix create ordering race, dual-registry credentials, and policy identity clearing (#176)
This commit is contained in:
@@ -355,6 +355,26 @@ pub async fn ensure_container(
|
||||
env_vars.push(format!("REGISTRY_USERNAME={username}"));
|
||||
env_vars.push(format!("REGISTRY_PASSWORD={password}"));
|
||||
}
|
||||
|
||||
// When the primary registry is NOT the distribution registry (e.g. a
|
||||
// local registry in push-mode), we still need containerd credentials for
|
||||
// the distribution registry so that community sandbox images
|
||||
// (d1i0nduu2f6qxk.cloudfront.net/nemoclaw-community/sandboxes/*) can be
|
||||
// pulled at runtime. Pass the distribution registry credentials as a
|
||||
// separate set of env vars so the entrypoint can add a second block to
|
||||
// registries.yaml.
|
||||
if registry_host != pull_registry() {
|
||||
env_vars.push(format!("DIST_REGISTRY_HOST={}", pull_registry()));
|
||||
env_vars.push(format!(
|
||||
"DIST_REGISTRY_USERNAME={}",
|
||||
pull_registry_username()
|
||||
));
|
||||
env_vars.push(format!(
|
||||
"DIST_REGISTRY_PASSWORD={}",
|
||||
pull_registry_password()
|
||||
));
|
||||
}
|
||||
|
||||
if !extra_sans.is_empty() {
|
||||
env_vars.push(format!("EXTRA_SANS={}", extra_sans.join(",")));
|
||||
}
|
||||
|
||||
@@ -1090,10 +1090,13 @@ pub async fn sandbox_create(
|
||||
|
||||
// When a custom image is specified, clear the default run_as_user/group
|
||||
// to prevent failures on images that lack the "sandbox" user/group.
|
||||
// If no explicit policy was provided, start from the restrictive default
|
||||
// so the server stores a policy with cleared identity — otherwise the
|
||||
// sandbox falls back to disk discovery which may re-introduce
|
||||
// run_as_user: sandbox for images that don't have that user.
|
||||
if image.is_some() {
|
||||
if let Some(ref mut p) = policy {
|
||||
navigator_policy::clear_process_identity(p);
|
||||
}
|
||||
let p = policy.get_or_insert_with(navigator_policy::restrictive_default_policy);
|
||||
navigator_policy::clear_process_identity(p);
|
||||
}
|
||||
|
||||
let template = image.map(|img| SandboxTemplate {
|
||||
|
||||
@@ -186,6 +186,11 @@ impl Navigator for NavigatorService {
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
// Persist to the store FIRST so the sandbox watcher always finds
|
||||
// the record with `spec` populated. If we created the k8s
|
||||
// resource first, the watcher could race us and write a fallback
|
||||
// record with `spec: None`, causing the supervisor to fail with
|
||||
// "sandbox has no spec".
|
||||
self.state.sandbox_index.update_from_sandbox(&sandbox);
|
||||
|
||||
self.state
|
||||
@@ -194,49 +199,47 @@ impl Navigator for NavigatorService {
|
||||
.await
|
||||
.map_err(|e| Status::internal(format!("persist sandbox failed: {e}")))?;
|
||||
|
||||
self.state.sandbox_watch_bus.notify(&id);
|
||||
|
||||
// Now create the Kubernetes resource. If this fails, clean up
|
||||
// the store entry to avoid orphans.
|
||||
match self.state.sandbox_client.create(&sandbox).await {
|
||||
Ok(_) => {
|
||||
info!(
|
||||
sandbox_id = %id,
|
||||
sandbox_name = %name,
|
||||
"CreateSandbox request completed successfully"
|
||||
);
|
||||
Ok(Response::new(SandboxResponse {
|
||||
sandbox: Some(sandbox),
|
||||
}))
|
||||
}
|
||||
Ok(_) => {}
|
||||
Err(kube::Error::Api(err)) if err.code == 409 => {
|
||||
// Clean up the store entry we just wrote.
|
||||
let _ = self.state.store.delete("sandbox", &id).await;
|
||||
self.state.sandbox_index.remove_sandbox(&id);
|
||||
warn!(
|
||||
sandbox_id = %id,
|
||||
sandbox_name = %name,
|
||||
"Sandbox already exists in Kubernetes"
|
||||
);
|
||||
if let Err(e) = self.state.store.delete(Sandbox::object_type(), &id).await {
|
||||
warn!(sandbox_id = %id, error = %e, "Failed to clean up store after conflict");
|
||||
}
|
||||
self.state.sandbox_index.remove_sandbox(&id);
|
||||
self.state.sandbox_watch_bus.notify(&id);
|
||||
Err(Status::already_exists("sandbox already exists"))
|
||||
return Err(Status::already_exists("sandbox already exists"));
|
||||
}
|
||||
Err(err) => {
|
||||
// Clean up the store entry we just wrote.
|
||||
let _ = self.state.store.delete("sandbox", &id).await;
|
||||
self.state.sandbox_index.remove_sandbox(&id);
|
||||
warn!(
|
||||
sandbox_id = %id,
|
||||
sandbox_name = %name,
|
||||
error = %err,
|
||||
"CreateSandbox request failed"
|
||||
);
|
||||
if let Err(e) = self.state.store.delete(Sandbox::object_type(), &id).await {
|
||||
warn!(sandbox_id = %id, error = %e, "Failed to clean up store after creation failure");
|
||||
}
|
||||
self.state.sandbox_index.remove_sandbox(&id);
|
||||
self.state.sandbox_watch_bus.notify(&id);
|
||||
Err(Status::internal(format!(
|
||||
return Err(Status::internal(format!(
|
||||
"create sandbox in kubernetes failed: {err}"
|
||||
)))
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
self.state.sandbox_watch_bus.notify(&id);
|
||||
|
||||
info!(
|
||||
sandbox_id = %id,
|
||||
sandbox_name = %name,
|
||||
"CreateSandbox request completed successfully"
|
||||
);
|
||||
Ok(Response::new(SandboxResponse {
|
||||
sandbox: Some(sandbox),
|
||||
}))
|
||||
}
|
||||
|
||||
type WatchSandboxStream = ReceiverStream<Result<SandboxStreamEvent, Status>>;
|
||||
|
||||
@@ -30,7 +30,7 @@ pub use grpc::NavigatorService;
|
||||
pub use http::{health_router, http_router};
|
||||
pub use multiplex::{MultiplexService, MultiplexedService};
|
||||
use persistence::Store;
|
||||
use sandbox::{SandboxClient, spawn_sandbox_watcher};
|
||||
use sandbox::{SandboxClient, spawn_sandbox_watcher, spawn_store_reconciler};
|
||||
use sandbox_index::SandboxIndex;
|
||||
use sandbox_watch::{SandboxWatchBus, spawn_kube_event_tailer};
|
||||
pub use tls::TlsAcceptor;
|
||||
@@ -130,6 +130,13 @@ pub async fn run_server(config: Config, tracing_log_bus: TracingLogBus) -> Resul
|
||||
state.sandbox_watch_bus.clone(),
|
||||
state.tracing_log_bus.clone(),
|
||||
);
|
||||
spawn_store_reconciler(
|
||||
store.clone(),
|
||||
state.sandbox_client.clone(),
|
||||
state.sandbox_index.clone(),
|
||||
state.sandbox_watch_bus.clone(),
|
||||
state.tracing_log_bus.clone(),
|
||||
);
|
||||
spawn_kube_event_tailer(state.clone());
|
||||
|
||||
// Create the multiplexed service
|
||||
|
||||
@@ -113,6 +113,18 @@ mirrors:
|
||||
|
||||
REGEOF
|
||||
|
||||
# If the distribution registry is a separate host (e.g. we're using a
|
||||
# local registry for component images), add it as an additional mirror
|
||||
# so community sandbox images can be pulled at runtime.
|
||||
if [ -n "${DIST_REGISTRY_HOST:-}" ] && [ "${DIST_REGISTRY_HOST}" != "${REGISTRY_HOST}" ]; then
|
||||
echo "Adding distribution registry mirror for ${DIST_REGISTRY_HOST}"
|
||||
cat >> "$REGISTRIES_YAML" <<REGEOF
|
||||
"${DIST_REGISTRY_HOST}":
|
||||
endpoint:
|
||||
- "https://${DIST_REGISTRY_HOST}"
|
||||
REGEOF
|
||||
fi
|
||||
|
||||
if [ -n "${REGISTRY_USERNAME:-}" ] && [ -n "${REGISTRY_PASSWORD:-}" ]; then
|
||||
cat >> "$REGISTRIES_YAML" <<REGEOF
|
||||
|
||||
@@ -123,6 +135,31 @@ configs:
|
||||
password: ${REGISTRY_PASSWORD}
|
||||
REGEOF
|
||||
fi
|
||||
|
||||
# Add auth for the distribution registry when it differs from the
|
||||
# primary registry (community sandbox images live there).
|
||||
if [ -n "${DIST_REGISTRY_HOST:-}" ] && [ "${DIST_REGISTRY_HOST}" != "${REGISTRY_HOST}" ] \
|
||||
&& [ -n "${DIST_REGISTRY_USERNAME:-}" ] && [ -n "${DIST_REGISTRY_PASSWORD:-}" ]; then
|
||||
# Append to existing configs block or start a new one.
|
||||
if [ -n "${REGISTRY_USERNAME:-}" ] && [ -n "${REGISTRY_PASSWORD:-}" ]; then
|
||||
# configs: block already started above — just append the entry.
|
||||
cat >> "$REGISTRIES_YAML" <<REGEOF
|
||||
"${DIST_REGISTRY_HOST}":
|
||||
auth:
|
||||
username: ${DIST_REGISTRY_USERNAME}
|
||||
password: ${DIST_REGISTRY_PASSWORD}
|
||||
REGEOF
|
||||
else
|
||||
cat >> "$REGISTRIES_YAML" <<REGEOF
|
||||
|
||||
configs:
|
||||
"${DIST_REGISTRY_HOST}":
|
||||
auth:
|
||||
username: ${DIST_REGISTRY_USERNAME}
|
||||
password: ${DIST_REGISTRY_PASSWORD}
|
||||
REGEOF
|
||||
fi
|
||||
fi
|
||||
else
|
||||
echo "Warning: REGISTRY_HOST not set; skipping registry config"
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user