From 0e821add1ebe810095f32b56c5e1053cbe7ad216 Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 10 Jul 2026 15:26:48 -0700 Subject: [PATCH 1/2] fix(pxe): seed NoCloud network-config for tenant-provided network settings NICo seeds NoCloud's user-data and meta-data files, but never seeds the separate network-config file. A top-level 'network:' key inside user-data is not a valid user-data format (confirmed via cloud-init's own schema validator) and is silently ignored, regardless of the change in #2814 that correctly moved user-data to the canonical NoCloud seed location. Adds a new /network-config PXE route that extracts an existing 'network:' key from the tenant's custom_cloud_init (if present) and serves it as its own document, plus one line in disk_imaging.sh to fetch it into the seed directory. No API changes needed; existing 'network:' key placement keeps working as-is. Verified end-to-end against real cloud-init: a populated network-config applies correctly, and an empty one (the common case, no custom network settings) falls back cleanly to default DHCP with no errors. Addresses #3303. Signed-off-by: Raj --- Cargo.lock | 1 + crates/pxe/Cargo.toml | 1 + crates/pxe/src/routes/cloud_init.rs | 57 +++++++++++++++++++++++++++++ pxe/common_files/disk_imaging.sh | 1 + pxe/templates/network-config | 1 + 5 files changed, 61 insertions(+) create mode 100644 pxe/templates/network-config diff --git a/Cargo.lock b/Cargo.lock index b3f447ad36..34571fdb2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2637,6 +2637,7 @@ dependencies = [ "mime", "pin-project-lite", "serde", + "serde_yaml", "tempfile", "tera", "tokio", diff --git a/crates/pxe/Cargo.toml b/crates/pxe/Cargo.toml index f5b8219529..e8de0e9e58 100644 --- a/crates/pxe/Cargo.toml +++ b/crates/pxe/Cargo.toml @@ -48,6 +48,7 @@ metrics-exporter-prometheus = { workspace = true } mime = { workspace = true } pin-project-lite = { workspace = true } serde = { features = ["derive"], workspace = true } +serde_yaml = { workspace = true } tera = { workspace = true } tokio = { workspace = true } toml = { workspace = true } diff --git a/crates/pxe/src/routes/cloud_init.rs b/crates/pxe/src/routes/cloud_init.rs index 0c8af5c5a5..de806c462c 100644 --- a/crates/pxe/src/routes/cloud_init.rs +++ b/crates/pxe/src/routes/cloud_init.rs @@ -267,6 +267,30 @@ pub async fn meta_data(machine: Machine, state: State) -> impl IntoRes axum_template::Render(template_key, state.engine.clone(), template_data) } +fn extract_network_config(custom_cloud_init: &str) -> Option { + let value: serde_yaml::Value = serde_yaml::from_str(custom_cloud_init).ok()?; + let network = value.get("network")?.clone(); + serde_yaml::to_string(&network).ok() +} + +pub async fn network_config(machine: Machine, state: State) -> impl IntoResponse { + let network_config_yaml = machine + .instructions + .custom_cloud_init + .as_deref() + .and_then(extract_network_config) + .unwrap_or_default(); + + let mut template_data: HashMap = HashMap::new(); + template_data.insert("network_config".to_string(), network_config_yaml); + + axum_template::Render( + "network-config".to_string(), + state.engine.clone(), + template_data, + ) +} + pub async fn vendor_data(state: State) -> impl IntoResponse { axum_template::Render( "printcontext", @@ -289,6 +313,10 @@ pub fn get_router(path_prefix: &str) -> Router { format!("{}/{}", path_prefix, "vendor-data").as_str(), get(vendor_data), ) + .route( + format!("{}/{}", path_prefix, "network-config").as_str(), + get(network_config), + ) } #[cfg(test)] @@ -593,3 +621,32 @@ mod tests { ); } } + +#[test] +fn extract_network_config_returns_network_key_as_yaml() { + let cloud_init = "#cloud-config\nnetwork:\n version: 2\n ethernets:\n eth0:\n addresses:\n - 10.10.10.50/24\nwrite_files:\n - path: /tmp/foo\n content: bar\n"; + + let extracted = extract_network_config(cloud_init).expect("network key should be extracted"); + let parsed: serde_yaml::Value = serde_yaml::from_str(&extracted).unwrap(); + + assert_eq!(parsed.get("version").unwrap().as_u64().unwrap(), 2); + assert!( + parsed + .get("ethernets") + .and_then(|e| e.get("eth0")) + .is_some(), + "expected eth0 config to be present in extracted network-config" + ); +} + +#[test] +fn extract_network_config_returns_none_when_no_network_key() { + let cloud_init = "#cloud-config\nwrite_files:\n - path: /tmp/foo\n content: bar\n"; + assert!(extract_network_config(cloud_init).is_none()); +} + +#[test] +fn extract_network_config_returns_none_on_invalid_yaml() { + let cloud_init = "not: valid: yaml: at: all: :::"; + assert!(extract_network_config(cloud_init).is_none()); +} \ No newline at end of file diff --git a/pxe/common_files/disk_imaging.sh b/pxe/common_files/disk_imaging.sh index f13d6bd611..b49d4d5883 100755 --- a/pxe/common_files/disk_imaging.sh +++ b/pxe/common_files/disk_imaging.sh @@ -149,6 +149,7 @@ function add_cloud_init() { mkdir -p "$seed_dir" curl --fail --retry 5 --retry-all-errors -k "$cloud_init_url/user-data" --output "$seed_dir/user-data" 2>&1 | tee "$log_output" curl --fail --retry 5 --retry-all-errors -k "$cloud_init_url/meta-data" --output "$seed_dir/meta-data" 2>&1 | tee "$log_output" + curl --fail --retry 5 --retry-all-errors -k "$cloud_init_url/network-config" --output "$seed_dir/network-config" 2>&1 | tee "$log_output" } function expand_root_fs() { diff --git a/pxe/templates/network-config b/pxe/templates/network-config new file mode 100644 index 0000000000..8acac3e681 --- /dev/null +++ b/pxe/templates/network-config @@ -0,0 +1 @@ +{{ network_config }} \ No newline at end of file From 0a115be7ac62c384a155964bb72bc739e64a990d Mon Sep 17 00:00:00 2001 From: Raj Date: Fri, 10 Jul 2026 17:01:17 -0700 Subject: [PATCH 2/2] refactor: address CodeRabbit review feedback - Avoid cloning the extracted network YAML value before serializing. - Consolidate the three extract_network_config tests into a single table-driven test, moved into the existing tests module. Signed-off-by: Raj --- crates/pxe/src/routes/cloud_init.rs | 105 +++++++++++++++------------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/crates/pxe/src/routes/cloud_init.rs b/crates/pxe/src/routes/cloud_init.rs index de806c462c..d23d680f45 100644 --- a/crates/pxe/src/routes/cloud_init.rs +++ b/crates/pxe/src/routes/cloud_init.rs @@ -101,8 +101,6 @@ fn user_data_handler( ); } context.insert("interface_id".to_string(), machine_interface_id.to_string()); - // Use URL overrides for external clients (static-assignments segment), - // falling back to global config. context.insert( "api_url".to_string(), api_url_override.unwrap_or(config.client_facing_api_url), @@ -234,10 +232,6 @@ pub async fn user_data(machine: Machine, state: State) -> impl IntoRes )), } } - // discovery_instructions can not be None for a non-assigned machine. - // This means that the machine is assigned to tenant. - // custom_cloud_init None means user has not configured any user-data. Send a empty - // response. (None, None) => { let mut template_data: HashMap = HashMap::new(); template_data.insert("user_data".to_string(), "{}".to_string()); @@ -267,10 +261,14 @@ pub async fn meta_data(machine: Machine, state: State) -> impl IntoRes axum_template::Render(template_key, state.engine.clone(), template_data) } +/// Extracts the top-level `network:` key (if present) from a tenant's +/// custom cloud-init document and returns it as its own standalone YAML +/// document, suitable for seeding NoCloud's separate `network-config` +/// file. A `network:` key inside `user-data` itself is not a recognized +/// user-data format and is silently ignored by cloud-init. fn extract_network_config(custom_cloud_init: &str) -> Option { let value: serde_yaml::Value = serde_yaml::from_str(custom_cloud_init).ok()?; - let network = value.get("network")?.clone(); - serde_yaml::to_string(&network).ok() + serde_yaml::to_string(value.get("network")?).ok() } pub async fn network_config(machine: Machine, state: State) -> impl IntoResponse { @@ -356,12 +354,6 @@ mod tests { let interface_id = "91609f10-c91d-470d-a260-6293ea0c1234".parse().unwrap(); let config = generate_forge_agent_config(interface_id, None); - // The intent here is to actually test what the written - // configuration file looks like, so we can visualize to - // make sure it's going to look like what we think it's - // supposed to look like. Obviously as various new fields - // get added to AgentConfig, then our test config will also - // need to be updated accordingly, but that should be ok. let test_config = fs::read_to_string(format!("{TEST_DATA_DIR}/agent_config.toml")).unwrap(); assert_eq!(config, test_config); @@ -377,11 +369,8 @@ mod tests { interface_id.to_string().as_str(), ); - // No forge-system section when no override is provided. assert!(data.get("forge-system").is_none()); - // Check to make sure is_fake_dpu gets skipped - // from the serialized output. let skipped = match data.get("machine").unwrap().get("is_fake_dpu") { Some(_val) => false, None => true, @@ -427,7 +416,6 @@ mod tests { let template_glob = concat!(env!("CARGO_MANIFEST_DIR"), "/../../pxe/templates/**/*"); let tera = tera::Tera::new(template_glob).unwrap(); - // Use the same string-valued context shape the route handler passes to Tera. let context = HashMap::from([ ( "api_url".to_string(), @@ -466,7 +454,6 @@ mod tests { ) .unwrap(); - // The mlxconfig value and DHCP drop rules should use the configured count. assert!(rendered.contains("NUM_OF_VFS=3")); assert!(!rendered.contains("NUM_OF_VFS=16")); assert_eq!(rendered.matches("--physdev-in pf0vf").count(), 3); @@ -482,7 +469,6 @@ mod tests { let template_glob = concat!(env!("CARGO_MANIFEST_DIR"), "/../../pxe/templates/**/*"); let tera = tera::Tera::new(template_glob).unwrap(); - // Use a non-empty provisioning string so the host representor bridge loop renders. let context = HashMap::from([ ( "api_url".to_string(), @@ -521,7 +507,6 @@ mod tests { ) .unwrap(); - // The loop should emit one assignment and invocation per bridge entry. assert!(rendered.contains("ovs-vsctl get bridge br-sfc external_ids")); assert!(rendered.contains("ovs-vsctl --may-exist add-port br-sfc")); assert!(rendered.contains( @@ -620,33 +605,57 @@ mod tests { Some("node-02.new.forge.example.com"), ); } -} - -#[test] -fn extract_network_config_returns_network_key_as_yaml() { - let cloud_init = "#cloud-config\nnetwork:\n version: 2\n ethernets:\n eth0:\n addresses:\n - 10.10.10.50/24\nwrite_files:\n - path: /tmp/foo\n content: bar\n"; - - let extracted = extract_network_config(cloud_init).expect("network key should be extracted"); - let parsed: serde_yaml::Value = serde_yaml::from_str(&extracted).unwrap(); - - assert_eq!(parsed.get("version").unwrap().as_u64().unwrap(), 2); - assert!( - parsed - .get("ethernets") - .and_then(|e| e.get("eth0")) - .is_some(), - "expected eth0 config to be present in extracted network-config" - ); -} -#[test] -fn extract_network_config_returns_none_when_no_network_key() { - let cloud_init = "#cloud-config\nwrite_files:\n - path: /tmp/foo\n content: bar\n"; - assert!(extract_network_config(cloud_init).is_none()); -} + /// Table-driven coverage for `extract_network_config` across its three + /// input variants: a present `network:` key, a missing one, and + /// malformed YAML. + #[test] + fn extract_network_config_handles_various_inputs() { + struct Case { + name: &'static str, + input: &'static str, + expect_some: bool, + } -#[test] -fn extract_network_config_returns_none_on_invalid_yaml() { - let cloud_init = "not: valid: yaml: at: all: :::"; - assert!(extract_network_config(cloud_init).is_none()); + let cases = [ + Case { + name: "network key present", + input: "#cloud-config\nnetwork:\n version: 2\n ethernets:\n eth0:\n addresses:\n - 10.10.10.50/24\nwrite_files:\n - path: /tmp/foo\n content: bar\n", + expect_some: true, + }, + Case { + name: "no network key", + input: "#cloud-config\nwrite_files:\n - path: /tmp/foo\n content: bar\n", + expect_some: false, + }, + Case { + name: "invalid yaml", + input: "not: valid: yaml: at: all: :::", + expect_some: false, + }, + ]; + + for case in cases { + let result = extract_network_config(case.input); + assert_eq!( + result.is_some(), + case.expect_some, + "case '{}' failed", + case.name + ); + + if case.expect_some { + let parsed: serde_yaml::Value = serde_yaml::from_str(&result.unwrap()).unwrap(); + assert_eq!(parsed.get("version").unwrap().as_u64().unwrap(), 2); + assert!( + parsed + .get("ethernets") + .and_then(|e| e.get("eth0")) + .is_some(), + "case '{}': expected eth0 config present", + case.name + ); + } + } + } } \ No newline at end of file