feat(drivers): support docker and podman config mounts#1785
Conversation
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
drew
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
|
🌿 Preview your docs: https://nvidia-preview-pr-1785.docs.buildwithfern.com/openshell |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
|
/ok to test 113bbea |
|
Label |
|
/ok to test f37da23 |
| #[test] | ||
| fn docker_config_reads_bind_mount_opt_in_from_driver_table() { | ||
| let file = config_file_from_toml( | ||
| r" | ||
| [openshell.drivers.docker] | ||
| enable_bind_mounts = true | ||
| ", | ||
| ); | ||
|
|
||
| let cfg = super::build_docker_config(Some(&file), None).expect("docker config"); | ||
|
|
||
| assert!(cfg.enable_bind_mounts); | ||
| } |
There was a problem hiding this comment.
I wouldn't call this a blocker, but it is strange that this test is in this file while the equivalent podman test is in lib.rs.
| for reserved in [ | ||
| "/opt/openshell", | ||
| "/etc/openshell/auth", | ||
| "/etc/openshell/tls", |
There was a problem hiding this comment.
Codex mentions that /etc/openshell-tls is also used at crates/openshell-sandbox/src/lib.rs:499. Should that also be added to the list? Does it make sense to reserve a wildcard like /etc/openshell*?
| for reserved in [ | ||
| "/opt/openshell", | ||
| "/etc/openshell/auth", | ||
| "/etc/openshell/tls", | ||
| "/run/netns", |
There was a problem hiding this comment.
I know that we said we didn't want to share mount defintions between Docker and Podman, but it does seem as if the list of reserved sandbox paths is not driver-specific and would likely be needed for any driver that supports mounts in some form. Would pulling the normalisation and path validation into a common location make sense?
| .is_some_and(|rest| rest.starts_with('/')) | ||
| } | ||
|
|
||
| fn proto_struct_to_json_object( |
| pids_limit: docker_pids_limit(config.sandbox_pids_limit)?, | ||
| device_requests: docker_gpu_device_requests(spec.gpu, &spec.gpu_device), | ||
| binds: Some(build_binds(sandbox, config)?), | ||
| mounts: (!user_mounts.is_empty()).then_some(user_mounts), |
There was a problem hiding this comment.
Question: Is the !is_empty() check strictly required here? Could this just be Some(user_mounts)? Alternatively, could one just use docker_driver_mounts(template, config.enable_bind_mounts)? directly here? (Or is the intent to allow errors to be triggered earlier?)
| source: String, | ||
| target: String, | ||
| #[serde(default)] | ||
| read_only: bool, |
There was a problem hiding this comment.
I see that read_only is false by default. While I understand that this is the behaviour for Docker and Podman, does it make sense to invert this for the sandboxing use case so that users need to explicitly add rw?
| if target.is_empty() { | ||
| return Err(Status::failed_precondition( | ||
| "mount target must not be empty", | ||
| )); | ||
| } | ||
| if target.as_bytes().contains(&0) { | ||
| return Err(Status::failed_precondition( | ||
| "mount target must not contain NUL bytes", | ||
| )); | ||
| } | ||
| if !target.starts_with('/') { | ||
| return Err(Status::failed_precondition( | ||
| "mount target must be an absolute container path", | ||
| )); | ||
| } |
There was a problem hiding this comment.
This check is the same as validate_absolute_mount_source does reusing the logic make things simpler?
| if target == "/sandbox" { | ||
| return Err(Status::failed_precondition( | ||
| "mount target '/sandbox' is reserved for the OpenShell workspace", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Question: Why not just fold /sandbox into the reserved list below? Is it because we allow mounts to paths below /sandbox?
| host_gateway_ip: String::new(), | ||
| ssh_socket_path: "/run/openshell/ssh.sock".to_string(), | ||
| sandbox_pids_limit: DEFAULT_SANDBOX_PIDS_LIMIT, | ||
| enable_bind_mounts: false, |
There was a problem hiding this comment.
We have added this flag to prevent users from adding direct host mounts by default. These are, however, still possible when creating a docker volume mapping to a local path. Should those also be gated on this flag?
2 participants
Summary
Adds driver-config mount support for local Docker and Podman sandboxes. Docker accepts existing named volumes and tmpfs mounts; Podman accepts existing named volumes, tmpfs mounts, and image mounts. Host bind mounts remain out of the driver-config schema, and NFS is supported through pre-created runtime-managed named volumes.
Related Issue
N/A. Follow-up to #1744.
Changes
--driver-config-json.Testing
mise run pre-commitpassesChecklist