Skip to content

Commit 7565f76

Browse files
committed
refactor(server): colocate Docker driver config construction with Podman/K8s
Move Docker config construction from cli.rs into lib.rs to match the pattern used by Podman and Kubernetes drivers. Docker config is now built inside build_compute_runtime() via docker_config_from_file() and apply_docker_local_tls_defaults(), eliminating the docker_config parameter from run_server() and build_compute_runtime(). The colocated test docker_config_reads_bind_mount_opt_in_from_driver_table moves from cli.rs to lib.rs next to its constructor. Refs: RHAIENG-5899 Signed-off-by: Ian Miller <ianmiller@nvidia.com> Signed-off-by: Ian Miller <milleryan2003@gmail.com>
1 parent c7202af commit 7565f76

2 files changed

Lines changed: 72 additions & 67 deletions

File tree

crates/openshell-server/src/cli.rs

Lines changed: 4 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use tracing::{info, warn};
1414
use tracing_subscriber::EnvFilter;
1515

1616
use crate::certgen;
17-
use crate::compute::{DockerComputeConfig, VmComputeConfig};
17+
use crate::compute::VmComputeConfig;
1818
use crate::config_file::{self, ConfigFile, GatewayFileSection};
1919
use crate::defaults::{self, LocalTlsPaths};
2020
use crate::{run_server, tracing_bus::TracingLogBus};
@@ -428,8 +428,6 @@ async fn run_from_args(mut args: RunArgs, matches: ArgMatches) -> Result<()> {
428428
args.disable_tls,
429429
args.port,
430430
)?;
431-
let docker_config = build_docker_config(file.as_ref(), local_tls.as_ref())?;
432-
433431
if args.disable_tls {
434432
warn!("TLS disabled — listening on plaintext HTTP");
435433
} else {
@@ -464,15 +462,9 @@ async fn run_from_args(mut args: RunArgs, matches: ArgMatches) -> Result<()> {
464462

465463
info!(bind = %config.bind_address, "Starting OpenShell server");
466464

467-
Box::pin(run_server(
468-
config,
469-
vm_config,
470-
docker_config,
471-
file,
472-
tracing_log_bus,
473-
))
474-
.await
475-
.into_diagnostic()
465+
Box::pin(run_server(config, vm_config, file, tracing_log_bus))
466+
.await
467+
.into_diagnostic()
476468
}
477469

478470
fn parse_compute_driver(value: &str) -> std::result::Result<String, String> {
@@ -788,33 +780,6 @@ fn build_vm_config(
788780
Ok(cfg)
789781
}
790782

791-
/// Build [`DockerComputeConfig`] using the same inheritance pattern as
792-
/// [`build_vm_config`].
793-
fn build_docker_config(
794-
file: Option<&ConfigFile>,
795-
local_tls: Option<&LocalTlsPaths>,
796-
) -> Result<DockerComputeConfig> {
797-
let mut cfg = if let Some(file) = file {
798-
let merged = config_file::driver_table(
799-
ComputeDriverKind::Docker,
800-
&file.openshell.gateway,
801-
file.openshell.drivers.get("docker"),
802-
);
803-
merged
804-
.try_into::<DockerComputeConfig>()
805-
.map_err(|e| miette::miette!("invalid [openshell.drivers.docker] table: {e}"))?
806-
} else {
807-
DockerComputeConfig::default()
808-
};
809-
apply_guest_tls_defaults(
810-
&mut cfg.guest_tls_ca,
811-
&mut cfg.guest_tls_cert,
812-
&mut cfg.guest_tls_key,
813-
local_tls,
814-
);
815-
Ok(cfg)
816-
}
817-
818783
fn apply_guest_tls_defaults(
819784
ca: &mut Option<PathBuf>,
820785
cert: &mut Option<PathBuf>,
@@ -1839,18 +1804,4 @@ default_image = "k8s-specific:1.0"
18391804
.expect("deserializes");
18401805
assert_eq!(parsed.default_image, "k8s-specific:1.0");
18411806
}
1842-
1843-
#[test]
1844-
fn docker_config_reads_bind_mount_opt_in_from_driver_table() {
1845-
let file = config_file_from_toml(
1846-
r"
1847-
[openshell.drivers.docker]
1848-
enable_bind_mounts = true
1849-
",
1850-
);
1851-
1852-
let cfg = super::build_docker_config(Some(&file), None).expect("docker config");
1853-
1854-
assert!(cfg.enable_bind_mounts);
1855-
}
18561807
}

crates/openshell-server/src/lib.rs

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,6 @@ impl ServerState {
208208
pub async fn run_server(
209209
config: Config,
210210
vm_config: VmComputeConfig,
211-
docker_config: DockerComputeConfig,
212211
config_file: Option<config_file::ConfigFile>,
213212
tracing_log_bus: TracingLogBus,
214213
) -> Result<()> {
@@ -243,7 +242,6 @@ pub async fn run_server(
243242
let compute = build_compute_runtime(
244243
&config,
245244
&vm_config,
246-
&docker_config,
247245
config_file.as_ref(),
248246
store.clone(),
249247
sandbox_index.clone(),
@@ -720,7 +718,6 @@ async fn terminate_signal() {
720718
async fn build_compute_runtime(
721719
config: &Config,
722720
vm_config: &VmComputeConfig,
723-
docker_config: &DockerComputeConfig,
724721
file: Option<&config_file::ConfigFile>,
725722
store: Arc<Store>,
726723
sandbox_index: SandboxIndex,
@@ -751,17 +748,21 @@ async fn build_compute_runtime(
751748
.await
752749
.map_err(|e| Error::execution(format!("failed to create compute runtime: {e}")))
753750
}
754-
ConfiguredComputeDriver::Builtin(ComputeDriverKind::Docker) => ComputeRuntime::new_docker(
755-
config.clone(),
756-
docker_config.clone(),
757-
store,
758-
sandbox_index,
759-
sandbox_watch_bus,
760-
tracing_log_bus,
761-
supervisor_sessions,
762-
)
763-
.await
764-
.map_err(|e| Error::execution(format!("failed to create compute runtime: {e}"))),
751+
ConfiguredComputeDriver::Builtin(ComputeDriverKind::Docker) => {
752+
let mut docker = docker_config_from_file(file)?;
753+
apply_docker_local_tls_defaults(config, &mut docker)?;
754+
ComputeRuntime::new_docker(
755+
config.clone(),
756+
docker,
757+
store,
758+
sandbox_index,
759+
sandbox_watch_bus,
760+
tracing_log_bus,
761+
supervisor_sessions,
762+
)
763+
.await
764+
.map_err(|e| Error::execution(format!("failed to create compute runtime: {e}")))
765+
}
765766
ConfiguredComputeDriver::Builtin(ComputeDriverKind::Podman) => {
766767
let mut podman = podman_config_from_file(file)?;
767768
podman.gateway_port = config.bind_address.port();
@@ -897,6 +898,44 @@ fn apply_podman_local_tls_defaults(
897898
Ok(())
898899
}
899900

901+
/// Same pattern as [`kubernetes_config_from_file`] but for Docker.
902+
fn docker_config_from_file(file: Option<&config_file::ConfigFile>) -> Result<DockerComputeConfig> {
903+
let Some(file) = file else {
904+
return Ok(DockerComputeConfig::default());
905+
};
906+
let merged = config_file::driver_table(
907+
ComputeDriverKind::Docker,
908+
&file.openshell.gateway,
909+
file.openshell.drivers.get("docker"),
910+
);
911+
merged
912+
.try_into()
913+
.map_err(|e| Error::config(format!("invalid [openshell.drivers.docker] table: {e}")))
914+
}
915+
916+
fn apply_docker_local_tls_defaults(
917+
config: &Config,
918+
docker: &mut DockerComputeConfig,
919+
) -> Result<()> {
920+
if config.tls.is_none()
921+
|| docker.guest_tls_ca.is_some()
922+
|| docker.guest_tls_cert.is_some()
923+
|| docker.guest_tls_key.is_some()
924+
{
925+
return Ok(());
926+
}
927+
928+
let Some(paths) = defaults::complete_local_tls_paths()
929+
.map_err(|e| Error::config(format!("failed to resolve local TLS defaults: {e}")))?
930+
else {
931+
return Ok(());
932+
};
933+
docker.guest_tls_ca = Some(paths.ca);
934+
docker.guest_tls_cert = Some(paths.client_cert);
935+
docker.guest_tls_key = Some(paths.client_key);
936+
Ok(())
937+
}
938+
900939
#[derive(Debug, Clone)]
901940
enum ConfiguredComputeDriver {
902941
Builtin(ComputeDriverKind),
@@ -1549,6 +1588,21 @@ enable_bind_mounts = true
15491588
assert!(cfg.enable_bind_mounts);
15501589
}
15511590

1591+
#[test]
1592+
fn docker_config_reads_bind_mount_opt_in_from_driver_table() {
1593+
let file: crate::config_file::ConfigFile = toml::from_str(
1594+
r"
1595+
[openshell.drivers.docker]
1596+
enable_bind_mounts = true
1597+
",
1598+
)
1599+
.expect("valid config");
1600+
1601+
let cfg = crate::docker_config_from_file(Some(&file)).expect("docker config");
1602+
1603+
assert!(cfg.enable_bind_mounts);
1604+
}
1605+
15521606
#[test]
15531607
fn gateway_listener_addresses_skip_driver_address_covered_by_wildcard() {
15541608
let primary: SocketAddr = "0.0.0.0:8080".parse().unwrap();

0 commit comments

Comments
 (0)