Skip to content

Commit 9f19b02

Browse files
committed
refactor(gateway): drop image_pull_policy from shared inheritance
Kubernetes and Podman use mutually-incompatible vocabularies for the same TOML key: - Kubernetes: `Always | IfNotPresent | Never` (free-form string passed verbatim to the K8s API). - Podman: `always | missing | never | newer` (strict lowercase enum deserialised into `ImagePullPolicy`). No value means the same thing in both drivers. Sharing the key at `[openshell.gateway]` scope and inheriting it into every active driver's table meant any value safe for one driver was either wrong or silently dropped for the other (`IfNotPresent` → `ImagePullPolicy::Missing` after `.unwrap_or_default()`). Operators run one driver per gateway, so the "shared default" never pays for itself. Make `image_pull_policy` driver-local: - Remove the field from `GatewayFileSection` and from `inheritable_keys()` for both Kubernetes and Podman. - Drop the file→`RunArgs` merge for the gateway-scope key. - Stop unconditionally clobbering the driver value with `config.sandbox_image_pull_policy` in the runtime wiring — only apply the CLI/env override when it was set (and, for Podman, only when it parses into the lowercase enum). - Move the key under `[openshell.drivers.kubernetes]` and `[openshell.drivers.podman]` in every example, the RFC, the architecture doc, and the Helm-rendered gateway ConfigMap. The supervisor pull policy follows the same shape: it is K8s-only and moves into `[openshell.drivers.kubernetes]` alongside `image_pull_policy` in the Helm template.
1 parent e671405 commit 9f19b02

9 files changed

Lines changed: 48 additions & 34 deletions

File tree

architecture/gateway.md

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -213,12 +213,18 @@ worked example and RFC 0003 for the full schema.
213213
### Driver inheritance
214214

215215
`[openshell.gateway]` carries a small set of values (`default_image`,
216-
`supervisor_image`, `image_pull_policy`, `guest_tls_ca/cert/key`,
217-
`client_tls_secret_name`, `host_gateway_ip`, `enable_user_namespaces`) that
218-
are inherited into each driver's `[openshell.drivers.<name>]` table when
219-
the driver-specific table does not override them. The allowlist is
220-
per-driver so a gateway-wide default cannot land in a driver that does not
221-
understand it (e.g. `client_tls_secret_name` is K8s-only).
216+
`supervisor_image`, `guest_tls_ca/cert/key`, `client_tls_secret_name`,
217+
`host_gateway_ip`, `enable_user_namespaces`) that are inherited into each
218+
driver's `[openshell.drivers.<name>]` table when the driver-specific table
219+
does not override them. The allowlist is per-driver so a gateway-wide
220+
default cannot land in a driver that does not understand it (e.g.
221+
`client_tls_secret_name` is K8s-only).
222+
223+
`image_pull_policy` is intentionally **not** inheritable: Kubernetes uses
224+
`Always | IfNotPresent | Never` (passed verbatim to the K8s API) while
225+
Podman uses the lowercase enum `always | missing | never | newer`. No
226+
value means the same thing in both, so the key lives only under each
227+
driver's own table.
222228

223229
Driver-specific values that are not part of the inheritance allowlist
224230
(e.g. K8s `namespace`, Podman `socket_path`, VM `vcpus`) only come from

crates/openshell-server/src/cli.rs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -580,12 +580,6 @@ fn merge_file_into_args(args: &mut RunArgs, file: &GatewayFileSection, matches:
580580
{
581581
args.sandbox_image = Some(image.clone());
582582
}
583-
if let Some(policy) = &file.image_pull_policy
584-
&& args.sandbox_image_pull_policy.is_none()
585-
&& arg_defaulted(matches, "sandbox_image_pull_policy")
586-
{
587-
args.sandbox_image_pull_policy = Some(policy.clone());
588-
}
589583
if let Some(secret) = &file.client_tls_secret_name
590584
&& args.client_tls_secret_name.is_none()
591585
&& arg_defaulted(matches, "client_tls_secret_name")

crates/openshell-server/src/config_file.rs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,6 @@ pub struct GatewayFileSection {
116116
#[serde(default)]
117117
pub supervisor_image: Option<String>,
118118
#[serde(default)]
119-
pub image_pull_policy: Option<String>,
120-
#[serde(default)]
121119
pub client_tls_secret_name: Option<String>,
122120
#[serde(default)]
123121
pub host_gateway_ip: Option<String>,
@@ -254,7 +252,6 @@ fn inheritable_keys(driver: ComputeDriverKind) -> &'static [&'static str] {
254252
ComputeDriverKind::Kubernetes => &[
255253
"default_image",
256254
"supervisor_image",
257-
"image_pull_policy",
258255
"client_tls_secret_name",
259256
"host_gateway_ip",
260257
"ssh_handshake_skew_secs",
@@ -269,7 +266,6 @@ fn inheritable_keys(driver: ComputeDriverKind) -> &'static [&'static str] {
269266
ComputeDriverKind::Podman => &[
270267
"default_image",
271268
"supervisor_image",
272-
"image_pull_policy",
273269
"guest_tls_ca",
274270
"guest_tls_cert",
275271
"guest_tls_key",
@@ -288,7 +284,6 @@ fn gateway_inherited_value(g: &GatewayFileSection, key: &str) -> Option<toml::Va
288284
match key {
289285
"default_image" => g.default_image.as_deref().map(string_value),
290286
"supervisor_image" => g.supervisor_image.as_deref().map(string_value),
291-
"image_pull_policy" => g.image_pull_policy.as_deref().map(string_value),
292287
"client_tls_secret_name" => g.client_tls_secret_name.as_deref().map(string_value),
293288
"host_gateway_ip" => g.host_gateway_ip.as_deref().map(string_value),
294289
"ssh_handshake_skew_secs" => g.ssh_handshake_skew_secs.and_then(skew_value),
@@ -349,7 +344,6 @@ compute_drivers = ["kubernetes"]
349344
sandbox_namespace = "agents"
350345
default_image = "ghcr.io/nvidia/openshell/sandbox:latest"
351346
supervisor_image = "ghcr.io/nvidia/openshell/supervisor:latest"
352-
image_pull_policy = "IfNotPresent"
353347
client_tls_secret_name = "openshell-sandbox-tls"
354348
355349
[openshell.gateway.tls]
@@ -442,7 +436,6 @@ version = 2
442436
let gateway = GatewayFileSection {
443437
default_image: Some("ghcr.io/nvidia/openshell/sandbox:0.9".to_string()),
444438
supervisor_image: Some("ghcr.io/nvidia/openshell/supervisor:0.9".to_string()),
445-
image_pull_policy: Some("IfNotPresent".to_string()),
446439
..Default::default()
447440
};
448441
let raw = toml::toml! {

crates/openshell-server/src/lib.rs

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -588,8 +588,14 @@ async fn build_compute_runtime(
588588
// file + CLI/env at startup.
589589
k8s.namespace.clone_from(&config.sandbox_namespace);
590590
k8s.default_image.clone_from(&config.sandbox_image);
591-
k8s.image_pull_policy
592-
.clone_from(&config.sandbox_image_pull_policy);
591+
// Only let the gateway-wide CLI/env value overwrite the per-driver
592+
// file value when it was actually set — otherwise the empty CLI
593+
// default would silently clobber `image_pull_policy` configured
594+
// under `[openshell.drivers.kubernetes]`.
595+
if !config.sandbox_image_pull_policy.is_empty() {
596+
k8s.image_pull_policy
597+
.clone_from(&config.sandbox_image_pull_policy);
598+
}
593599
k8s.grpc_endpoint.clone_from(&config.grpc_endpoint);
594600
k8s.ssh_socket_path
595601
.clone_from(&config.sandbox_ssh_socket_path);
@@ -677,7 +683,16 @@ async fn build_compute_runtime(
677683
// Shared fields are sourced from Config (which already merged
678684
// file + CLI/env at startup).
679685
podman.default_image.clone_from(&config.sandbox_image);
680-
podman.image_pull_policy = config.sandbox_image_pull_policy.parse().unwrap_or_default();
686+
// The CLI/env `image_pull_policy` is K8s-shaped
687+
// (e.g. `IfNotPresent`) and won't parse into Podman's lowercase
688+
// enum. Only apply it when the operator set a Podman-shaped value
689+
// explicitly; otherwise keep whatever `[openshell.drivers.podman]`
690+
// (or the struct default) provided.
691+
if !config.sandbox_image_pull_policy.is_empty()
692+
&& let Ok(policy) = config.sandbox_image_pull_policy.parse()
693+
{
694+
podman.image_pull_policy = policy;
695+
}
681696
podman.grpc_endpoint.clone_from(&config.grpc_endpoint);
682697
podman.gateway_port = config.bind_address.port();
683698
podman

deploy/helm/openshell/templates/gateway-config.yaml

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,6 @@ data:
3333
sandbox_namespace = {{ include "openshell.sandboxNamespace" . | quote }}
3434
default_image = {{ .Values.server.sandboxImage | quote }}
3535
supervisor_image = {{ include "openshell.supervisorImage" . | quote }}
36-
{{- if .Values.server.sandboxImagePullPolicy }}
37-
image_pull_policy = {{ .Values.server.sandboxImagePullPolicy | quote }}
38-
{{- end }}
39-
{{- if .Values.supervisor.image.pullPolicy }}
40-
supervisor_image_pull_policy = {{ .Values.supervisor.image.pullPolicy | quote }}
41-
{{- end }}
4236
grpc_endpoint = {{ include "openshell.grpcEndpoint" . | quote }}
4337
{{- if .Values.server.sshGatewayHost }}
4438
ssh_gateway_host = {{ .Values.server.sshGatewayHost | quote }}
@@ -99,3 +93,9 @@ data:
9993
10094
[openshell.drivers.kubernetes]
10195
supervisor_sideload_method = {{ include "openshell.supervisorSideloadMethod" . | quote }}
96+
{{- if .Values.server.sandboxImagePullPolicy }}
97+
image_pull_policy = {{ .Values.server.sandboxImagePullPolicy | quote }}
98+
{{- end }}
99+
{{- if .Values.supervisor.image.pullPolicy }}
100+
supervisor_image_pull_policy = {{ .Values.supervisor.image.pullPolicy | quote }}
101+
{{- end }}

examples/gateway/gateway.example.toml

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,12 @@ enable_loopback_service_http = true
4040
# when the driver-specific table does not override them.
4141
default_image = "ghcr.io/nvidia/openshell/sandbox:latest"
4242
supervisor_image = "ghcr.io/nvidia/openshell/supervisor:latest"
43-
image_pull_policy = "IfNotPresent"
4443
client_tls_secret_name = "openshell-client-tls"
44+
# `image_pull_policy` is intentionally NOT a shared key: Kubernetes uses
45+
# `Always | IfNotPresent | Never` (passed as a string to the K8s API) while
46+
# Podman uses the lowercase `always | missing | never | newer` enum. There is
47+
# no value that means the same thing in both, so set it under the relevant
48+
# `[openshell.drivers.<name>]` table instead.
4549

4650
# Gateway listener TLS (distinct from the per-driver guest_tls_*).
4751
[openshell.gateway.tls]
@@ -63,8 +67,9 @@ user_role = "openshell-user"
6367
# over the inherited value.
6468

6569
[openshell.drivers.kubernetes]
66-
namespace = "agents"
67-
grpc_endpoint = "https://openshell-gateway.agents.svc:8080"
70+
namespace = "agents"
71+
grpc_endpoint = "https://openshell-gateway.agents.svc:8080"
72+
image_pull_policy = "IfNotPresent" # K8s vocabulary: Always | IfNotPresent | Never
6873

6974
[openshell.drivers.docker]
7075
network_name = "openshell-docker"
@@ -74,6 +79,7 @@ network_name = "openshell-docker"
7479
socket_path = "/run/podman/podman.sock"
7580
network_name = "openshell"
7681
stop_timeout_secs = 10
82+
image_pull_policy = "missing" # Podman vocabulary: always | missing | never | newer
7783

7884
[openshell.drivers.vm]
7985
state_dir = "/var/lib/openshell/vm"

examples/gateway/kubernetes.example.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ compute_drivers = ["kubernetes"]
2020
# Shared defaults inherited into [openshell.drivers.kubernetes].
2121
default_image = "ghcr.io/nvidia/openshell/sandbox:latest"
2222
supervisor_image = "ghcr.io/nvidia/openshell/supervisor:latest"
23-
image_pull_policy = "IfNotPresent"
2423
client_tls_secret_name = "openshell-client-tls"
2524

2625
[openshell.gateway.tls]
@@ -31,6 +30,7 @@ client_ca_path = "/etc/openshell-tls/client-ca/ca.crt"
3130
[openshell.drivers.kubernetes]
3231
namespace = "agents"
3332
grpc_endpoint = "https://openshell-gateway.agents.svc:8080"
33+
image_pull_policy = "IfNotPresent"
3434
# Use the image volume on K8s >= 1.35 (GA in 1.36); switch to "init-container"
3535
# on older clusters or where the ImageVolume feature gate is off.
3636
supervisor_sideload_method = "image-volume"

examples/gateway/podman.example.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ compute_drivers = ["podman"]
1818
# Shared defaults inherited into [openshell.drivers.podman].
1919
default_image = "ghcr.io/nvidia/openshell/sandbox:latest"
2020
supervisor_image = "ghcr.io/nvidia/openshell/supervisor:latest"
21-
image_pull_policy = "missing" # Podman pull policy: always | missing | never | newer
2221
guest_tls_ca = "/etc/openshell/certs/ca.pem"
2322
guest_tls_cert = "/etc/openshell/certs/client.pem"
2423
guest_tls_key = "/etc/openshell/certs/client-key.pem"
@@ -28,3 +27,4 @@ guest_tls_key = "/etc/openshell/certs/client-key.pem"
2827
socket_path = "/run/user/1000/podman/podman.sock"
2928
network_name = "openshell"
3029
stop_timeout_secs = 10
30+
image_pull_policy = "missing" # Podman vocabulary: always | missing | never | newer

rfc/0003-gateway-configuration/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ guest_tls_key = "/etc/openshell/certs/client-key.pem"
142142
[openshell.drivers.podman]
143143
socket_path = "/run/podman/podman.sock"
144144
default_image = "ghcr.io/nvidia/openshell/sandbox:latest"
145-
image_pull_policy = "IfNotPresent"
145+
image_pull_policy = "missing" # Podman vocabulary: always | missing | never | newer
146146
supervisor_image = "ghcr.io/nvidia/openshell/supervisor:latest"
147147
network_name = "openshell"
148148
stop_timeout_secs = 10

0 commit comments

Comments
 (0)