Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 37 additions & 32 deletions crates/openshell-bootstrap/src/docker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const REGISTRY_NAMESPACE_DEFAULT: &str = "openshell";
/// `connect_with_local_defaults()` ceiling is 120s, which is far too short for
/// multi-GB image exports — a 7 GB image on a laptop SSD takes ~4–5 minutes.
/// One hour is a safe upper bound; override with `OPENSHELL_DOCKER_TIMEOUT_SECS`.
pub(crate) const DEFAULT_LARGE_TRANSFER_TIMEOUT_SECS: u64 = 3600;
pub const DEFAULT_LARGE_TRANSFER_TIMEOUT_SECS: u64 = 3600;

/// Build a local-Docker client suitable for large streaming transfers.
/// Respects `OPENSHELL_DOCKER_TIMEOUT_SECS` (in seconds); falls back to
Expand All @@ -50,7 +50,7 @@ pub fn connect_local_for_large_transfers() -> std::result::Result<Docker, Bollar
/// | `["legacy"]` | `["legacy"]` — pass through to the non-CDI fallback path |
/// | `["auto"]` | `["nvidia.com/gpu=all"]` if CDI enabled, else `["legacy"]` |
/// | `[cdi-ids…]` | unchanged |
pub(crate) fn resolve_gpu_device_ids(gpu: &[String], cdi_enabled: bool) -> Vec<String> {
pub fn resolve_gpu_device_ids(gpu: &[String], cdi_enabled: bool) -> Vec<String> {
match gpu {
[] => vec![],
[v] if v == "auto" => {
Expand Down Expand Up @@ -346,22 +346,25 @@ pub async fn find_gateway_container(docker: &Docker, port: Option<u16>) -> Resul

let matches: Vec<String> = containers
.iter()
.filter(|c| is_gateway_image(c) && port.map_or(true, |p| has_port(c, p)))
.filter(|c| is_gateway_image(c) && port.is_none_or(|p| has_port(c, p)))
.filter_map(container_name)
.collect();

match matches.len() {
0 => {
let hint = if let Some(p) = port {
format!(
"No openshell gateway container found listening on port {p}.\n\
let hint = port.map_or_else(
|| {
"No openshell gateway container found.\n\
Is the gateway running? Check with: docker ps"
)
} else {
"No openshell gateway container found.\n\
Is the gateway running? Check with: docker ps"
.to_string()
};
.to_string()
},
|p| {
format!(
"No openshell gateway container found listening on port {p}.\n\
Is the gateway running? Check with: docker ps"
)
},
);
Err(miette::miette!("{hint}"))
}
1 => Ok(matches.into_iter().next().unwrap()),
Expand Down Expand Up @@ -488,6 +491,8 @@ pub async fn ensure_image(
/// Returns the actual host port the container is using. When an existing
/// container is reused (same image), this may differ from `gateway_port`
/// because the container was originally created with a different port.
// Refactoring this signature would touch many call sites across the workspace.
#[allow(clippy::too_many_arguments)]
pub async fn ensure_container(
docker: &Docker,
name: &str,
Expand Down Expand Up @@ -744,10 +749,7 @@ pub async fn ensure_container(
// When OPENSHELL_PUSH_IMAGES is set the entrypoint overrides the baked-in
// HelmChart manifest so k3s uses the locally-pushed images with
// IfNotPresent pull policy instead of pulling from the remote registry.
let push_mode = std::env::var("OPENSHELL_PUSH_IMAGES")
.ok()
.filter(|v| !v.trim().is_empty())
.is_some();
let push_mode = std::env::var("OPENSHELL_PUSH_IMAGES").is_ok_and(|v| !v.trim().is_empty());
let effective_tag = std::env::var("IMAGE_TAG")
.ok()
.filter(|v| !v.trim().is_empty())
Expand Down Expand Up @@ -863,22 +865,22 @@ pub async fn check_port_conflicts(

let ports = container.ports.as_deref().unwrap_or_default();
for port in ports {
if let Some(public) = port.public_port {
if needed_ports.contains(&public) {
let cname = names
.first()
.map(|n| n.trim_start_matches('/').to_string())
.unwrap_or_else(|| {
container
.id
.clone()
.unwrap_or_else(|| "<unknown>".to_string())
});
conflicts.push(PortConflict {
container_name: cname,
host_port: public,
});
}
if let Some(public) = port.public_port
&& needed_ports.contains(&public)
{
let cname = names.first().map_or_else(
|| {
container
.id
.clone()
.unwrap_or_else(|| "<unknown>".to_string())
},
|n| n.trim_start_matches('/').to_string(),
);
conflicts.push(PortConflict {
container_name: cname,
host_port: public,
});
}
}
}
Expand Down Expand Up @@ -1371,6 +1373,9 @@ mod tests {
);
}

// Test-only: mutates DOCKER_HOST env var via std::env::set_var/remove_var,
// which require unsafe in the 2024 edition.
#[allow(unsafe_code)]
#[test]
fn docker_not_reachable_error_with_docker_host() {
// Simulate: DOCKER_HOST is set but daemon unresponsive.
Expand Down
17 changes: 8 additions & 9 deletions crates/openshell-bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ pub struct DeployOptions {
/// - `[]` — no GPU passthrough (default)
/// - `["legacy"]` — internal non-CDI fallback path (`driver="nvidia"`, `count=-1`)
/// - `["auto"]` — resolved at deploy time: CDI if enabled on the daemon, else the non-CDI fallback
/// - `[cdi-ids…]` — CDI DeviceRequest with the given device IDs
/// - `[cdi-ids…]` — CDI `DeviceRequest` with the given device IDs
pub gpu: Vec<String>,
/// When true, destroy any existing gateway resources before deploying.
/// When false, an existing gateway is left as-is and deployment is
Expand Down Expand Up @@ -340,8 +340,8 @@ where
// the image to recreate the container, so the pull must happen.
let need_image = !resume || !resume_container_exists;
if need_image {
log("[status] Downloading gateway".to_string());
if remote_opts.is_some() {
log("[status] Downloading gateway".to_string());
let on_log_clone = Arc::clone(&on_log);
let progress_cb = move |msg: String| {
if let Ok(mut f) = on_log_clone.lock() {
Expand All @@ -358,7 +358,6 @@ where
.await?;
} else {
// Local deployment: ensure image exists (pull if needed)
log("[status] Downloading gateway".to_string());
ensure_image(
&target_docker,
&image_ref,
Expand Down Expand Up @@ -732,16 +731,16 @@ pub async fn gateway_container_logs<W: std::io::Write>(
Ok(())
}

/// Fetch the last `n` lines of container logs for a local gateway as a
/// `String`. This is a convenience wrapper for diagnostic call sites (e.g.
/// failure diagnosis in the CLI) that do not hold a Docker client handle.
/// Fetch the last `n` lines of container logs for a local gateway as a `String`.
///
/// This is a convenience wrapper for diagnostic call sites (e.g. failure
/// diagnosis in the CLI) that do not hold a Docker client handle.
///
/// Returns an empty string on any Docker/connection error so callers don't
/// need to worry about error handling.
pub async fn fetch_gateway_logs(name: &str, n: usize) -> String {
let docker = match Docker::connect_with_local_defaults() {
Ok(d) => d,
Err(_) => return String::new(),
let Ok(docker) = Docker::connect_with_local_defaults() else {
return String::new();
};
let container = container_name(name);
fetch_recent_logs(&docker, &container, n).await
Expand Down
11 changes: 5 additions & 6 deletions crates/openshell-bootstrap/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,11 @@ pub fn load_last_sandbox(gateway: &str) -> Option<String> {
/// This should be called after a sandbox is deleted so that subsequent commands
/// don't try to connect to a sandbox that no longer exists.
pub fn clear_last_sandbox_if_matches(gateway: &str, sandbox: &str) {
if let Some(current) = load_last_sandbox(gateway) {
if current == sandbox {
if let Ok(path) = last_sandbox_path(gateway) {
let _ = std::fs::remove_file(path);
}
}
if let Some(current) = load_last_sandbox(gateway)
&& current == sandbox
&& let Ok(path) = last_sandbox_path(gateway)
{
let _ = std::fs::remove_file(path);
}
}

Expand Down
11 changes: 7 additions & 4 deletions crates/openshell-cli/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ fn generate_confirmation_code() -> String {
let hash_b = hasher_b.finish();

prev_hash = hash_b;
// hash_b is `u64`; truncation to `usize` is acceptable here since we mod
// by charset.len() (small) and only use it as an index.
#[allow(clippy::cast_possible_truncation)]
let idx = (hash_b as usize) % charset.len();
code.push(charset[idx] as char);
}
Expand All @@ -91,8 +94,7 @@ pub async fn browser_auth_flow(gateway_endpoint: &str) -> Result<String> {
// listener, spawns a callback server, and waits the full AUTH_TIMEOUT
// (120 s) for a POST that will never arrive.
let no_browser = std::env::var("OPENSHELL_NO_BROWSER")
.map(|v| v == "1" || v.eq_ignore_ascii_case("true"))
.unwrap_or(false);
.is_ok_and(|v| v == "1" || v.eq_ignore_ascii_case("true"));
if no_browser {
return Err(miette::miette!(
"authentication skipped (OPENSHELL_NO_BROWSER is set).\n\
Expand Down Expand Up @@ -129,8 +131,9 @@ pub async fn browser_auth_flow(gateway_endpoint: &str) -> Result<String> {

eprint!("Press Enter to open the browser for authentication...");
std::io::stderr().flush().ok();
let mut _input = String::new();
std::io::stdin().read_line(&mut _input).ok();
let mut input = String::new();
std::io::stdin().read_line(&mut input).ok();
drop(input);

if let Err(e) = open_browser(&auth_url) {
debug!(error = %e, "failed to open browser");
Expand Down
2 changes: 1 addition & 1 deletion crates/openshell-cli/src/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ fn is_connectivity_error(error: &miette::Report) -> bool {
/// `false` to skip bootstrap. Otherwise returns `true` — a gateway is created
/// automatically without prompting the user.
pub fn confirm_bootstrap(override_value: Option<bool>) -> Result<bool> {
if let Some(false) = override_value {
if override_value == Some(false) {
return Ok(false);
}
Ok(true)
Expand Down
10 changes: 5 additions & 5 deletions crates/openshell-cli/src/completers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,11 @@ mod tests {
.unwrap();

let result = complete_gateway_names(OsStr::new("a"));
let names: Vec<String> = result
.iter()
.map(|candidate| candidate.get_value().to_string_lossy().into_owned())
.collect();
assert!(names.contains(&"alpha".to_string()));
assert!(
result
.iter()
.any(|candidate| candidate.get_value().to_string_lossy() == "alpha")
);
});
}
}
34 changes: 17 additions & 17 deletions crates/openshell-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ fn resolve_sandbox_name(name: Option<String>, gateway: &str) -> Result<String> {
Specify a sandbox name or connect to one first: nav sandbox connect <name>"
)
})?;
eprintln!("{} Using sandbox '{}' (last used)", "→".bold(), last.bold(),);
eprintln!("{} Using sandbox '{}' (last used)", "→".bold(), last.bold());
Ok(last)
}

Expand Down Expand Up @@ -816,7 +816,7 @@ enum GatewayCommands {
/// `nvidia.com/gpu` resources. Requires NVIDIA drivers and the
/// NVIDIA Container Toolkit on the host.
///
/// When enabled, OpenShell auto-selects CDI when the Docker daemon has
/// When enabled, `OpenShell` auto-selects CDI when the Docker daemon has
/// CDI enabled and falls back to Docker's NVIDIA GPU request path
/// (`--gpus all`) otherwise.
#[arg(long)]
Expand Down Expand Up @@ -1158,7 +1158,7 @@ enum SandboxCommands {
policy: Option<String>,

/// Forward a local port to the sandbox before the initial command or shell starts.
/// Accepts [bind_address:]port (e.g. 8080, 0.0.0.0:8080). Keeps the sandbox alive.
/// Accepts [`bind_address`:]port (e.g. 8080, 0.0.0.0:8080). Keeps the sandbox alive.
#[arg(long, conflicts_with = "no_keep")]
forward: Option<String>,

Expand Down Expand Up @@ -1467,11 +1467,11 @@ enum PolicyCommands {
#[arg(long = "remove-endpoint")]
remove_endpoints: Vec<String>,

/// Add a REST allow rule: host:port:METHOD:path_glob.
/// Add a REST allow rule: `host:port:METHOD:path_glob`.
#[arg(long = "add-allow")]
add_allow: Vec<String>,

/// Add a REST deny rule: host:port:METHOD:path_glob.
/// Add a REST deny rule: `host:port:METHOD:path_glob`.
#[arg(long = "add-deny")]
add_deny: Vec<String>,

Expand Down Expand Up @@ -1551,7 +1551,7 @@ enum PolicyCommands {
/// Prove properties of a sandbox policy — or find counterexamples.
#[command(help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")]
Prove {
/// Path to OpenShell sandbox policy YAML.
/// Path to `OpenShell` sandbox policy YAML.
#[arg(long, value_hint = ValueHint::FilePath)]
policy: String,

Expand Down Expand Up @@ -1641,7 +1641,7 @@ enum ForwardCommands {
/// Start forwarding a local port to a sandbox.
#[command(help_template = LEAF_HELP_TEMPLATE, next_help_heading = "FLAGS")]
Start {
/// Port to forward: [bind_address:]port (e.g. 8080, 0.0.0.0:8080).
/// Port to forward: [`bind_address`:]port (e.g. 8080, 0.0.0.0:8080).
port: String,

/// Sandbox name (defaults to last-used sandbox).
Expand Down Expand Up @@ -1670,6 +1670,7 @@ enum ForwardCommands {
}

#[tokio::main]
#[allow(clippy::large_stack_frames)] // CLI dispatch holds many futures; OK at top level.
async fn main() -> Result<()> {
// Install the rustls crypto provider before completion runs — completers may
// establish TLS connections to the gateway.
Expand Down Expand Up @@ -1739,7 +1740,7 @@ async fn main() -> Result<()> {
} else {
vec![]
};
run::gateway_admin_deploy(
Box::pin(run::gateway_admin_deploy(
&name,
remote.as_deref(),
ssh_key.as_deref(),
Expand All @@ -1751,7 +1752,7 @@ async fn main() -> Result<()> {
registry_username.as_deref(),
registry_token.as_deref(),
gpu,
)
))
.await?;
}
GatewayCommands::Stop {
Expand Down Expand Up @@ -1868,7 +1869,7 @@ async fn main() -> Result<()> {
} else {
println!("{}", "Gateway Status".cyan().bold());
println!();
println!(" {} No gateway configured.", "Status:".dimmed(),);
println!(" {} No gateway configured.", "Status:".dimmed());
println!();
println!(
"Deploy a gateway with: {}",
Expand All @@ -1886,16 +1887,15 @@ async fn main() -> Result<()> {
ForwardCommands::Stop { port, name } => {
let name = match name {
Some(n) => n,
None => match run::find_forward_by_port(port)? {
Some(n) => {
None => {
if let Some(n) = run::find_forward_by_port(port)? {
eprintln!("→ Found forward on sandbox '{n}'");
n
}
None => {
eprintln!("{} No active forward found for port {port}", "!".yellow(),);
} else {
eprintln!("{} No active forward found for port {port}", "!".yellow());
return Ok(());
}
},
}
};
if run::stop_forward(&name, port)? {
eprintln!(
Expand Down Expand Up @@ -3272,7 +3272,7 @@ mod tests {
});
}

/// Verify the flag names the TUI uses to build its ProxyCommand are
/// Verify the flag names the TUI uses to build its `ProxyCommand` are
/// accepted by the `SshProxy` subcommand and land in the right fields.
/// This catches drift when CLI flags are renamed or restructured.
#[test]
Expand Down
Loading
Loading