Skip to content

Commit d919c3d

Browse files
elezarelezar
committed
fix(server): validate portable resource requirements
Validate typed CPU and memory quantities and reject legacy CPU or memory entries in the historical inline template resource passthrough with migration guidance. Part of #2838 Signed-off-by: Evan Lezar <[email protected]>
1 parent 0f3a009 commit d919c3d

1 file changed

Lines changed: 227 additions & 3 deletions

File tree

‎crates/openshell-server/src/grpc/validation.rs‎

Lines changed: 227 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,8 +197,8 @@ pub(super) fn validate_sandbox_spec(name: &str, spec: &SandboxSpec) -> Result<()
197197
validate_env_entries(&tmpl.environment, "spec.template.environment")?;
198198
}
199199

200-
// --- spec.resource_requirements.gpu ---
201-
validate_gpu_request_fields(spec)?;
200+
// --- spec.resource_requirements ---
201+
validate_resource_requirement_fields(spec)?;
202202

203203
if !spec.command.is_empty() {
204204
validate_main_process_command(&spec.command)?;
@@ -296,17 +296,55 @@ fn validate_main_process_command(command: &[String]) -> Result<(), Status> {
296296
Ok(())
297297
}
298298

299-
fn validate_gpu_request_fields(spec: &SandboxSpec) -> Result<(), Status> {
299+
fn validate_resource_requirement_fields(spec: &SandboxSpec) -> Result<(), Status> {
300300
if openshell_core::gpu::sandbox_gpu_count(spec.resource_requirements.as_ref()) == Some(0) {
301301
return Err(invalid_argument(
302302
"spec.resource_requirements.gpu.count",
303303
"gpu count must be greater than 0",
304304
));
305305
}
306306

307+
if let Some(cpu) = spec
308+
.resource_requirements
309+
.as_ref()
310+
.and_then(|requirements| requirements.cpu.as_ref())
311+
{
312+
if let Some(quantity) = cpu.quantity.as_deref() {
313+
validate_cpu_quantity(quantity, "spec.resource_requirements.cpu.quantity")?;
314+
} else {
315+
return Err(Status::invalid_argument(
316+
"spec.resource_requirements.cpu must set quantity",
317+
));
318+
}
319+
}
320+
321+
if let Some(memory) = spec
322+
.resource_requirements
323+
.as_ref()
324+
.and_then(|requirements| requirements.memory.as_ref())
325+
{
326+
if let Some(quantity) = memory.quantity.as_deref() {
327+
validate_memory_quantity(quantity, "spec.resource_requirements.memory.quantity")?;
328+
} else {
329+
return Err(Status::invalid_argument(
330+
"spec.resource_requirements.memory must set quantity",
331+
));
332+
}
333+
}
334+
307335
Ok(())
308336
}
309337

338+
fn validate_cpu_quantity(value: &str, field_name: &str) -> Result<(), Status> {
339+
openshell_core::quantity::validate_cpu_quantity(value, field_name)
340+
.map_err(Status::invalid_argument)
341+
}
342+
343+
fn validate_memory_quantity(value: &str, field_name: &str) -> Result<(), Status> {
344+
openshell_core::quantity::validate_memory_quantity(value, field_name)
345+
.map_err(Status::invalid_argument)
346+
}
347+
310348
/// Validate template-level field sizes.
311349
fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> {
312350
// String fields.
@@ -360,6 +398,7 @@ fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> {
360398
),
361399
));
362400
}
401+
reject_legacy_template_cpu_memory_resources(s)?;
363402
}
364403
if let Some(ref s) = tmpl.driver_config {
365404
let size = s.encoded_len();
@@ -401,6 +440,31 @@ fn reject_gateway_owned_driver_config_keys(config: &prost_types::Struct) -> Resu
401440
}
402441
}
403442
}
443+
444+
Ok(())
445+
}
446+
447+
fn reject_legacy_template_cpu_memory_resources(
448+
resources: &prost_types::Struct,
449+
) -> Result<(), Status> {
450+
for section_name in ["limits", "requests"] {
451+
let Some(value) = resources.fields.get(section_name) else {
452+
continue;
453+
};
454+
let Some(prost_types::value::Kind::StructValue(section)) = value.kind.as_ref() else {
455+
return Err(Status::invalid_argument(format!(
456+
"template.resources.{section_name} must be an object"
457+
)));
458+
};
459+
460+
for resource_name in ["cpu", "memory"] {
461+
if section.fields.contains_key(resource_name) {
462+
return Err(Status::invalid_argument(format!(
463+
"template.resources.{section_name}.{resource_name} is no longer supported; use spec.resource_requirements.{resource_name}.quantity"
464+
)));
465+
}
466+
}
467+
}
404468
Ok(())
405469
}
406470

@@ -1188,6 +1252,8 @@ mod tests {
11881252
let spec = SandboxSpec {
11891253
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
11901254
gpu: Some(openshell_core::proto::GpuResourceRequirements { count: None }),
1255+
cpu: None,
1256+
memory: None,
11911257
}),
11921258
..Default::default()
11931259
};
@@ -1199,6 +1265,8 @@ mod tests {
11991265
let spec = SandboxSpec {
12001266
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
12011267
gpu: Some(openshell_core::proto::GpuResourceRequirements { count: Some(2) }),
1268+
cpu: None,
1269+
memory: None,
12021270
}),
12031271
..Default::default()
12041272
};
@@ -1210,6 +1278,8 @@ mod tests {
12101278
let spec = SandboxSpec {
12111279
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
12121280
gpu: Some(openshell_core::proto::GpuResourceRequirements { count: Some(0) }),
1281+
cpu: None,
1282+
memory: None,
12131283
}),
12141284
..Default::default()
12151285
};
@@ -1218,6 +1288,160 @@ mod tests {
12181288
assert!(err.message().contains("gpu count must be greater than 0"));
12191289
}
12201290

1291+
#[test]
1292+
fn validate_sandbox_spec_accepts_cpu_and_memory_requirements() {
1293+
let spec = SandboxSpec {
1294+
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
1295+
gpu: None,
1296+
cpu: Some(openshell_core::proto::CpuResourceRequirements {
1297+
quantity: Some("500m".to_string()),
1298+
}),
1299+
memory: Some(openshell_core::proto::MemoryResourceRequirements {
1300+
quantity: Some("2Gi".to_string()),
1301+
}),
1302+
}),
1303+
..Default::default()
1304+
};
1305+
1306+
assert!(validate_sandbox_spec("compute-sandbox", &spec).is_ok());
1307+
}
1308+
1309+
#[test]
1310+
fn validate_sandbox_spec_rejects_invalid_cpu_requirements() {
1311+
let spec = SandboxSpec {
1312+
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
1313+
gpu: None,
1314+
cpu: Some(openshell_core::proto::CpuResourceRequirements {
1315+
quantity: Some("0".to_string()),
1316+
}),
1317+
memory: Some(openshell_core::proto::MemoryResourceRequirements {
1318+
quantity: Some("2Gi".to_string()),
1319+
}),
1320+
}),
1321+
..Default::default()
1322+
};
1323+
1324+
let err = validate_sandbox_spec("compute-sandbox", &spec).unwrap_err();
1325+
assert_eq!(err.code(), Code::InvalidArgument);
1326+
assert!(
1327+
err.message()
1328+
.contains("spec.resource_requirements.cpu.quantity")
1329+
);
1330+
}
1331+
1332+
#[test]
1333+
fn validate_sandbox_spec_accepts_cpu_and_memory_quantities() {
1334+
let spec = SandboxSpec {
1335+
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
1336+
gpu: None,
1337+
cpu: Some(openshell_core::proto::CpuResourceRequirements {
1338+
quantity: Some("250m".to_string()),
1339+
}),
1340+
memory: Some(openshell_core::proto::MemoryResourceRequirements {
1341+
quantity: Some("512Mi".to_string()),
1342+
}),
1343+
}),
1344+
..Default::default()
1345+
};
1346+
1347+
assert!(validate_sandbox_spec("compute-sandbox", &spec).is_ok());
1348+
}
1349+
1350+
#[test]
1351+
fn validate_sandbox_spec_rejects_invalid_memory_quantity() {
1352+
let spec = SandboxSpec {
1353+
resource_requirements: Some(openshell_core::proto::ResourceRequirements {
1354+
gpu: None,
1355+
cpu: None,
1356+
memory: Some(openshell_core::proto::MemoryResourceRequirements {
1357+
quantity: Some("0".to_string()),
1358+
}),
1359+
}),
1360+
..Default::default()
1361+
};
1362+
1363+
let err = validate_sandbox_spec("compute-sandbox", &spec).unwrap_err();
1364+
assert_eq!(err.code(), Code::InvalidArgument);
1365+
assert!(
1366+
err.message()
1367+
.contains("spec.resource_requirements.memory.quantity")
1368+
);
1369+
}
1370+
1371+
#[test]
1372+
fn validate_sandbox_spec_rejects_legacy_template_cpu_memory_resources() {
1373+
use prost_types::{Struct, Value, value::Kind};
1374+
1375+
let mut limits = std::collections::BTreeMap::new();
1376+
limits.insert(
1377+
"cpu".to_string(),
1378+
Value {
1379+
kind: Some(Kind::StringValue("500m".to_string())),
1380+
},
1381+
);
1382+
let mut fields = std::collections::BTreeMap::new();
1383+
fields.insert(
1384+
"limits".to_string(),
1385+
Value {
1386+
kind: Some(Kind::StructValue(Struct { fields: limits })),
1387+
},
1388+
);
1389+
let spec = SandboxSpec {
1390+
template: Some(SandboxTemplate {
1391+
resources: Some(Struct { fields }),
1392+
..Default::default()
1393+
}),
1394+
..Default::default()
1395+
};
1396+
1397+
let err = validate_sandbox_spec("legacy-resources", &spec).unwrap_err();
1398+
assert_eq!(err.code(), Code::InvalidArgument);
1399+
assert!(err.message().contains("template.resources.limits.cpu"));
1400+
assert!(
1401+
err.message()
1402+
.contains("spec.resource_requirements.cpu.quantity")
1403+
);
1404+
}
1405+
1406+
#[test]
1407+
fn validate_sandbox_spec_rejects_non_object_legacy_resource_sections() {
1408+
use prost_types::{ListValue, Struct, Value, value::Kind};
1409+
1410+
let invalid_sections = [
1411+
(
1412+
"limits",
1413+
"string",
1414+
Some(Kind::StringValue("500m".to_string())),
1415+
),
1416+
(
1417+
"requests",
1418+
"list",
1419+
Some(Kind::ListValue(ListValue { values: Vec::new() })),
1420+
),
1421+
("limits", "null", None),
1422+
];
1423+
1424+
for (section_name, name, kind) in invalid_sections {
1425+
let mut fields = std::collections::BTreeMap::new();
1426+
fields.insert(section_name.to_string(), Value { kind });
1427+
let spec = SandboxSpec {
1428+
template: Some(SandboxTemplate {
1429+
resources: Some(Struct { fields }),
1430+
..Default::default()
1431+
}),
1432+
..Default::default()
1433+
};
1434+
1435+
let message = format!("{name} {section_name} section should be rejected");
1436+
let err = validate_sandbox_spec("legacy-resources", &spec).expect_err(&message);
1437+
assert_eq!(err.code(), Code::InvalidArgument, "{name}");
1438+
assert_eq!(
1439+
err.message(),
1440+
format!("template.resources.{section_name} must be an object")
1441+
);
1442+
}
1443+
}
1444+
12211445
#[test]
12221446
fn validate_sandbox_spec_accepts_empty_defaults() {
12231447
assert!(validate_sandbox_spec("", &default_spec()).is_ok());

0 commit comments

Comments
 (0)