feat(kubernetes): support PVC subPath driver config#2034
Conversation
Signed-off-by: mjamiv <michael.commack@gmail.com>
|
Thanks for this — the caller-owned-PVC-by-
Happy to help validate — we're deploying this exact pattern and can share e2e findings. |
Signed-off-by: mjamiv <michael.commack@gmail.com>
|
Thanks for the test-scope review. I added positive I agree the missing-PVC and durability claims need cluster-level coverage: kubelet |
mjamiv
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
| WORKSPACE_VOLUME_NAME, | ||
| ]; | ||
|
|
||
| const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[ |
There was a problem hiding this comment.
Why are we redefining what should be shared across drivers (see driver_mounts::RESERVED_MOUNT_TARGETS)? Note that the openshell_core::driver_utils::SUPERVISOR_CONTAINER_* constants added to the list later would already be covered by /opt/openshell which is included in the shared list.
Review: PVC subPath driver configThanks for this — the direction is good and it correctly routes subpath/source/target validation through the shared Code reuse / overlap with other driversAll container drivers already converge on
Correctly kept driver-local (don't hoist): the reserved volume-name list (PVC names are K8s-only), the PVC config structs, and the SPIFFE runtime-conflict check.
Implementation issuesGate on these:
Design — worth discussing:
Minor:
Checked and not issues: the reserved volume-name list does currently cover all five managed volume names; protected-path coverage of managed mounts (TLS, sa-token, supervisor) is complete via the protected list + shared Net: approve the direction; I'd like 1–3 (and ideally reuse items 1–4) addressed before merge. |
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Review: PVC subPath driver config
Thanks for this — the direction is good and it correctly routes subpath/source/target validation through the shared openshell_core::driver_mounts module. Two themes below: code reuse (so the K8s driver doesn't become a fourth divergent copy of mount logic) and implementation issues (a few I'd gate merge on).
Code reuse / overlap with other drivers
All container drivers already converge on openshell_core::driver_mounts (Docker lib.rs:1779-1909, Podman container.rs:546-688), so subpath rules, NUL/empty checks, .. rejection, and the /sandbox + reserved-target base list are already unified. This PR reuses most of that — good. Remaining duplication worth consolidating:
path_is_or_underis an exact copy of the private fn indriver_mounts.rs:99-104. Make the shared onepuband import it instead of re-declaring.default_true()is now defined a third time (already indocker/lib.rs:342andpodman/container.rs:136). Along with the#[serde(default = "default_true")] read_only: boolidiom, this belongs in the shared module.- Duplicate mount-target detection is identical in three places — Docker (
lib.rs:1857-1917), Podman (container.rs:625-681), and now here. A sharedvalidate_unique_mount_targets(&[...], driver_name)would collapse all three. - Protected-path list overlaps and risks drift.
KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHSrepeats/etc/openshell,/etc/openshell-tls,/run/netns(already indriver_mounts::RESERVED_MOUNT_TARGETS) and redundantly listsSUPERVISOR_CONTAINER_DIR/SUPERVISOR_CONTAINER_BINARY, both already under/opt/openshellwhich the shared list covers. Sincevalidate_kubernetes_mount_pathcallsvalidate_container_mount_targetfirst, keep only the K8s-unique entries (/var/run/secrets/openshell,/spiffe-workload-api) locally and let the shared list cover the rest — otherwise the two lists will silently diverge. - Consider promoting the stricter path checks into the shared validator.
validate_kubernetes_mount_pathadds rejection of internal empty segments (/a//b),.segments, and surrounding whitespace — checksvalidate_container_mount_targetlacks today (Docker/Podman currently accept/a//b). If these are correctness improvements, hoist them so every driver benefits and the K8s wrapper shrinks to just the protected-list check.
Correctly kept driver-local (don't hoist): the reserved volume-name list (PVC names are K8s-only), the PVC config structs, and the SPIFFE runtime-conflict check.
Note for reviewers: subpath support is intentionally inconsistent across drivers — Docker supports it on volume mounts, Podman rejects it (
reject_subpath). This PR adding subpath for PVC mounts matches Docker; it's deliberate, not an oversight.
Implementation issues
Gate on these:
-
Validation is split across three sites with inconsistent handling of the same check.
validate_kubernetes_mount_pathis used invalidate()(returnsErr), inkubernetes_driver_volume_mount_to_k8s(.expect()— panics), and inhas_explicit_sandbox_data_mount(.ok()— silently ignores). In the normal flowcreate_sandboxvalidates before rendering, so the panic is currently unreachable — but this PR widens the panic surface, sincefrom_template()now fails on semantic checks (duplicate/reserved names, bad subpath, protected paths, read-only weakening), not just serde errors. Any future render path not preceded byvalidate_driver_config_for_sandboxwill panic the driver instead of returningInvalidArgument. Render functions should take an already-validated, normalized config rather than re-parsing and.expect()-ing. -
Re-validation at render exists only to recompute the normalized string.
kubernetes_driver_volume_mount_to_k8sre-runsvalidate_kubernetes_mount_path/validate_mount_subpathpurely to get the normalizedmountPath/subPath. Validate once, store the normalized values on the struct, render from those — removes the redundant work and the panic in (1). -
Reserved volume-name list duplicates pod-spec string literals.
KUBERNETES_DRIVER_RESERVED_VOLUME_NAMEShardcodes"openshell-client-tls"and"openshell-sa-token"; the pod spec hardcodes the same literals (driver.rs:1465,1478). They match today, but if either managed volume is renamed the guard silently stops matching and a user volume could collide. Extract both to named constants used in both places (the other three already use constants).
Design — worth discussing:
-
Any subpath mount under
/sandboxdisables all default workspace persistence.has_explicit_sandbox_data_mountflipsinject_workspaceoff for any mount at-or-under/sandbox, so the documented example (/sandbox/.openshell/workspace,/sandbox/.openshell/memory) makes/sandboxitself ephemeral image content while only the two leaf subpaths persist. Kubernetes supports nested volumeMounts, so disabling the whole workspace isn't strictly required — it's a coarse simplification that silently drops persistence for everything else under/sandbox. At minimum this needs a prominent warning in the docs/README; better, reconsider whether the default workspace should coexist with narrow user subpath mounts. -
Double read-only opt-in is easy to trip. Both the PVC volume and the mount default to
read_only: true, and the validator rejects a RW mount on a RO volume — so RW requiresread_only: falsein two places. Consider defaulting the mount'sread_onlyfrom the volume's value to halve the opt-in.
Minor:
validate_kubernetes_reference_nameand theclaim_namecheck don't enforce DNS-1123 label syntax, so invalid-but-printable names pass driver validation and fail later at the API server.validate_kubernetes_driver_runtime_mountsre-runsvalidate_kubernetes_mount_pathon every mount already validated byvalidate()(~3× per create) — folds away once (1)/(2) land.
Checked and not issues: the reserved volume-name list does currently cover all five managed volume names; protected-path coverage of managed mounts (TLS, sa-token, supervisor) is complete via the protected list + shared RESERVED_MOUNT_TARGETS; mount_path_conflicts_with_protected_path correctly also blocks mounting a parent of a reserved path.
Net: approve the direction; I'd like 1–3 (and ideally reuse items 1–4) addressed before merge.
Signed-off-by: mjamiv <michael.commack@gmail.com>
|
@austin-shih PR updated and ready for review. |
|
Re-reviewed Remaining gap is the cluster-level coverage we discussed: missing-PVC |
| &self.volumes, | ||
| &mut self.containers.agent.volume_mounts, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Looking at the normalize_ functions, I think that they can all actually be validate_* functions. Note that I've created #2086 to make the driver_mounts::validate_* functions consistent with this.
There was a problem hiding this comment.
2086 is merged. Let's rebase this one.
| Ok(normalized) | ||
| } | ||
|
|
||
| fn validate_kubernetes_mount_path(mount_path: &str) -> Result<String, String> { |
There was a problem hiding this comment.
We should not be modifying the path in a validate function.
There was a problem hiding this comment.
Done in 96ac311. The mount_path and sub_path validation helpers now return validation success only; non-normalized values are rejected instead of written back into the config.
| let spiffe_mount_path = spiffe_socket_mount_path(socket_path); | ||
| for mount in &config.containers.agent.volume_mounts { | ||
| let mount_path = mount.mount_path.as_str(); | ||
| if mount_path_conflicts_with_protected_path(mount_path, &spiffe_mount_path) { |
There was a problem hiding this comment.
Could you comment on why the spiffe path is handled separately from other mounts?
There was a problem hiding this comment.
Added a comment in 96ac311. The static OpenShell-managed paths are validated during from_template, while the SPIFFE socket mount path is derived from gateway runtime config and can only be checked once that runtime path is available.
There was a problem hiding this comment.
That's a fair explanation of why the split exists, but there's a related issue worth addressing: /spiffe-workload-api currently appears in both KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS and the dynamic check in validate_kubernetes_driver_runtime_mounts. The static entry is a hard-coded approximation of what is actually a runtime-derived path — if a gateway configures a non-default SPIFFE socket path, the static entry blocks the wrong directory while the dynamic check is doing the real work.
A cleaner approach would be to split the validation into two explicit concerns:
- Structural correctness (absolute path, normalized, no
.., no NUL bytes) — stays invalidate_kubernetes_mount_path, runs atfrom_templatetime, no change needed there. - Protected path conflicts — moves entirely to
kubernetes_driver_config_for_spec, where the complete set of protected paths (static + runtime-derived) is assembled once and passed to a single conflict-check pass.
This eliminates validate_kubernetes_driver_runtime_mounts as a separate function, removes /spiffe-workload-api from the static constant (leaving only paths that are truly always protected regardless of runtime config), and makes KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS an accurate invariant rather than a conservative approximation. It also scales cleanly to additional dynamic paths: kubernetes_driver_config_for_spec simply pushes each one onto protected_paths before the single conflict-check call, or accepts a list as input, rather than requiring a new validation function and loop per path.
Roughly:
-const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] =
- &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH, "/spiffe-workload-api"];
+const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH];
fn validate_kubernetes_mount_path(mount_path: &str) -> Result<(), String> {
// structural checks only: whitespace, normalization, absolute path, etc.
- for protected_path in KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS {
- if mount_path_conflicts_with_protected_path(mount_path, protected_path) {
- return Err(format!(
- "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'"
- ));
- }
- }
Ok(())
}
+fn validate_kubernetes_protected_path_conflicts(
+ volume_mounts: &[KubernetesDriverVolumeMountConfig],
+ protected_paths: &[&str],
+) -> Result<(), String> {
+ for mount in volume_mounts {
+ let mount_path = mount.mount_path.as_str();
+ for protected_path in protected_paths {
+ if mount_path_conflicts_with_protected_path(mount_path, protected_path) {
+ return Err(format!(
+ "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'"
+ ));
+ }
+ }
+ }
+ Ok(())
+}
+
-fn validate_kubernetes_driver_runtime_mounts(
- config: &KubernetesSandboxDriverConfig,
- provider_spiffe_workload_api_socket_path: Option<&str>,
-) -> Result<(), String> {
- let Some(socket_path) = provider_spiffe_workload_api_socket_path else {
- return Ok(());
- };
- let spiffe_mount_path = spiffe_socket_mount_path(socket_path);
- for mount in &config.containers.agent.volume_mounts {
- let mount_path = mount.mount_path.as_str();
- if mount_path_conflicts_with_protected_path(mount_path, &spiffe_mount_path) {
- return Err(format!(
- "mount path '{mount_path}' conflicts with reserved OpenShell path '{spiffe_mount_path}'"
- ));
- }
- }
- Ok(())
-}
-
fn kubernetes_driver_config_for_spec(
spec: Option<&SandboxSpec>,
provider_spiffe_workload_api_socket_path: Option<&str>,
) -> Result<KubernetesSandboxDriverConfig, String> {
let config = ...?;
- validate_kubernetes_driver_runtime_mounts(&config, provider_spiffe_workload_api_socket_path)?;
+ let mut protected_paths: Vec<&str> = KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS.to_vec();
+ let spiffe_mount_path;
+ if let Some(socket_path) = provider_spiffe_workload_api_socket_path {
+ spiffe_mount_path = spiffe_socket_mount_path(socket_path);
+ protected_paths.push(&spiffe_mount_path);
+ }
+ validate_kubernetes_protected_path_conflicts(
+ &config.containers.agent.volume_mounts,
+ &protected_paths,
+ )?;
Ok(config)
}Happy to open a follow-up issue or take a crack at the patch if useful.
There was a problem hiding this comment.
Done in a9cf61c. Removed /spiffe-workload-api from the static Kubernetes protected-path list, moved protected-path conflict checking into kubernetes_driver_config_for_spec, and now assemble static plus runtime-derived protected paths in one pass before rendering.
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result<String, String> { |
There was a problem hiding this comment.
Question: Are we validating general resource names here?
There was a problem hiding this comment.
Done in 96ac311. Volume names and volume_mount references now use DNS-1123 label validation, while PVC claim_name uses DNS-1123 subdomain validation. Docs and tests were updated for that split.
There was a problem hiding this comment.
One cleanup worth considering here: validate_kubernetes_name_text creates an indirect dependency on driver_mounts::validate_mount_source (a mount-specific helper) just to check whitespace and NUL bytes — neither of which belongs in DNS name validation. The whitespace check is also redundant, since whitespace isn't in the allowed character set and is_kubernetes_dns1123_label would reject it anyway with an equally descriptive error.
is_kubernetes_dns1123_label is also a local reimplementation of is_dns_label from openshell-core/src/config.rs (and openshell-server/src/grpc/service.rs). Moving these to openshell-core as a follow-up would be easier if the local versions are already self-contained and structurally identical to what they'll replace.
Proposed cleanup (all within this PR's scope since these functions were introduced here):
-fn validate_kubernetes_name_text(value: &str, field: &str) -> Result<(), String> {
- if value != value.trim() {
- return Err(format!(
- "{field} must not contain leading or trailing whitespace"
- ));
- }
- driver_mounts::validate_mount_source(value, field).map(|_| ())
+// TODO: replace with openshell_core::k8s_names::is_dns_label once available
+fn is_dns_label(label: &str) -> bool {
+ if label.is_empty() || label.len() > 63 || label.starts_with('-') || label.ends_with('-') {
+ return false;
+ }
+ label
+ .bytes()
+ .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-')
}
-fn is_kubernetes_dns1123_label(value: &str) -> bool { ... }
+// TODO: replace with openshell_core::k8s_names::is_dns_subdomain once available
+fn is_dns_subdomain(value: &str) -> bool {
+ value.len() <= 253 && value.split('.').all(is_dns_label)
+}
fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result<(), String> {
- validate_kubernetes_name_text(value, field)?;
- if !is_kubernetes_dns1123_label(value) {
+ if !is_dns_label(value) {
return Err(format!("..."));
}
Ok(())
}
fn validate_kubernetes_dns1123_subdomain(value: &str, field: &str) -> Result<(), String> {
- validate_kubernetes_name_text(value, field)?;
- let is_dns1123_subdomain = value.len() <= 253 && value.split('.').all(is_kubernetes_dns1123_label);
- if !is_dns1123_subdomain {
+ if !is_dns_subdomain(value) {
return Err(format!("..."));
}
Ok(())
}The is_dns_label implementation here matches openshell-core/src/config.rs exactly, so the follow-up extraction becomes a one-line import swap.
| let normalized = driver_mounts::validate_mount_source(value, field)?; | ||
| if normalized != value { | ||
| return Err(format!( | ||
| "{field} must not contain leading or trailing whitespace" | ||
| )); | ||
| } |
There was a problem hiding this comment.
What about just: if value != value.trim()?
There was a problem hiding this comment.
Done in 96ac311. The whitespace check is now the direct value != value.trim() guard before the common source validation runs.
| validate_kubernetes_driver_runtime_mounts( | ||
| &config, | ||
| self.config.provider_spiffe_enabled().then_some( | ||
| self.config | ||
| .provider_spiffe_workload_api_socket_path | ||
| .as_str(), | ||
| ), | ||
| )?; |
There was a problem hiding this comment.
Quuestion: Since we call validate / normalize in KubernetesSandboxDriverConfig::from_template, does it not make sense to also check this there?
There was a problem hiding this comment.
Done/clarified in 96ac311. Template-owned protected paths are checked during from_template; the remaining SPIFFE conflict depends on runtime gateway config. The new spec-level helper handles that runtime-dependent validation before rendering.
| let _ = KubernetesSandboxDriverConfig::from_sandbox(sandbox) | ||
| let driver_config = self | ||
| .validate_driver_config_for_sandbox(sandbox) |
There was a problem hiding this comment.
Given that neither sandbox nor driver_config can change after this point, does it make sense to rather only construct driver_config in sandbox_to_k8s_spec instead of passing it as an argument?
There was a problem hiding this comment.
Done in 96ac311. sandbox_to_k8s_spec now constructs and validates the Kubernetes driver_config itself and returns a Result. create_sandbox renders the spec before Kubernetes API discovery so invalid driver_config still returns InvalidArgument before any API call.
Signed-off-by: mjamiv <michael.commack@gmail.com>
|
@elezar Updated in 96ac311 for the July 1 review pass. What changed:
Validation run locally:
|
| || driver_mounts::path_is_or_under(protected_path, mount_path) | ||
| } | ||
|
|
||
| fn kubernetes_driver_volume_to_k8s(volume: &KubernetesDriverVolumeConfig) -> serde_json::Value { |
There was a problem hiding this comment.
Could we use the existing k8s-openapi types here instead of assembling loose JSON? We could implement From<&KubernetesDriverVolumeConfig> for Volume and the
equivalent for VolumeMount, then serialize those at the dynamic JSON boundary. This would keep Kubernetes field naming and optional-field handling schema-
aware without broadening the PR into refactoring the rest of the pod-template builder.
| } | ||
|
|
||
| fn validate_kubernetes_mount_path(mount_path: &str) -> Result<(), String> { | ||
| if mount_path != mount_path.trim() { |
There was a problem hiding this comment.
Most of the structural checks in this function are already provided by driver_mounts::validate_container_mount_target on main (which rejects whitespace, NUL bytes, non-absolute paths, .., /sandbox, and the shared reserved paths without normalizing). The only checks here that aren't already shared are the empty-segment (//) and .-segment cases.
If those two checks were added to validate_container_mount_target in driver_mounts, this function could be removed entirely — the structural validation would be shared across all drivers automatically. Combined with the suggestion on the SPIFFE comment to move protected-path conflict checking into kubernetes_driver_config_for_spec, there would be no Kubernetes-specific mount path validation function needed at all:
- Structural correctness →
driver_mounts::validate_container_mount_target(shared, all drivers) - Protected path conflicts →
kubernetes_driver_config_for_spec(Kubernetes-specific, assembled at runtime)
There was a problem hiding this comment.
Done in a9cf61c. Added the internal empty-segment and dot-segment checks to driver_mounts::validate_container_mount_target, removed the Kubernetes-specific mount path/subpath validation helpers, and now use the shared target/subpath validators from Kubernetes, Docker, and Podman paths.
There was a problem hiding this comment.
Thanks. That's much cleaner now. Does the implementation also handle .. segments?
…c-subpath Signed-off-by: mjamiv <michael.commack@gmail.com> # Conflicts: # crates/openshell-core/src/driver_mounts.rs # crates/openshell-driver-docker/src/lib.rs # crates/openshell-driver-kubernetes/src/driver.rs # crates/openshell-driver-podman/src/container.rs
| "mount target must be normalized and must not contain empty path segments or '.'" | ||
| ); | ||
| assert_eq!( | ||
| validate_container_mount_target("/sandbox/work/./tmp").unwrap_err(), |
There was a problem hiding this comment.
What bout /sandbox/work/../../tmp?
| path_is_or_under(mount_path, protected_path) || path_is_or_under(protected_path, mount_path) | ||
| } | ||
|
|
||
| fn path_is_or_under(path: &str, parent: &str) -> bool { |
There was a problem hiding this comment.
This duplicates driver_mounts::path_is_or_under, which is already public. The local copy can be removed and all three call sites updated to use the shared function instead.
|
Hi @elezar, I pushed
Local validation passed: The required |
3 participants
Summary
Add Kubernetes driver-config support for mounting existing PVCs into the agent container with optional
sub_pathvalues. This gives deployments a narrow driver-owned storage topology for durable per-user PVC data without broad PodSpec overrides.Related Issue
Fixes #2033
Changes
driver_config.kubernetes.volumes[]for PVC-backed pod volumes.driver_config.kubernetes.containers.agent.volume_mounts[]for agent PVC mounts with optionalsub_path.read_only: falseis explicit.sub_pathvalues, and attempts to weaken read-only PVC volumes./sandboxworkspace PVC injection when an explicit Kubernetes driver-config mount targets a path below/sandbox/.Testing
mise run pre-commitpassesmise; after installing it,mise run pre-commitrefused to run until the repo config was trusted. I did not runmise trustbecause it persistently changes local trust settings.cargo fmt --all -- --checkcargo test -p openshell-driver-kubernetescargo clippy -p openshell-driver-kubernetes --all-targets -- -D warningsnpx --yes markdownlint-cli2@0.22.0 crates/openshell-driver-kubernetes/README.md docs/reference/sandbox-compute-drivers.mdxUV_CACHE_DIR=.cache/uv uv run python scripts/update_license_headers.py --checkChecklist