From d24ea23413503f26777e0f39971f31cb2f44be76 Mon Sep 17 00:00:00 2001 From: Jean-Gabriel Gill-Couture Date: Tue, 2 Sep 2025 11:26:07 -0400 Subject: [PATCH] fix: Dhcp static hostname has to have domain specified with dnsmasq, also progress on okd installation automation --- harmony/src/modules/dhcp.rs | 9 ++- harmony/src/modules/okd/installation.rs | 79 +++++++++++------------ opnsense-config/src/modules/dhcp.rs | 2 +- opnsense-config/src/modules/dnsmasq.rs | 84 ++++++++++++++++--------- 4 files changed, 102 insertions(+), 72 deletions(-) diff --git a/harmony/src/modules/dhcp.rs b/harmony/src/modules/dhcp.rs index 1270fa6..24d697a 100644 --- a/harmony/src/modules/dhcp.rs +++ b/harmony/src/modules/dhcp.rs @@ -130,6 +130,7 @@ impl Interpret for DhcpInterpret { #[derive(Debug, new, Clone, Serialize)] pub struct DhcpHostBindingScore { pub host_binding: Vec, + pub domain: Option, } impl Score for DhcpHostBindingScore { @@ -168,8 +169,14 @@ impl DhcpHostBindingInterpret { } }; + let name = if let Some(domain) = self.score.domain.as_ref() { + format!("{}.{}", binding.logical_host.name, domain) + } else { + binding.logical_host.name.clone() + }; + DHCPStaticEntry { - name: binding.logical_host.name.clone(), + name, mac: binding.physical_host.cluster_mac(), ip, } diff --git a/harmony/src/modules/okd/installation.rs b/harmony/src/modules/okd/installation.rs index 1087a24..9ed1898 100644 --- a/harmony/src/modules/okd/installation.rs +++ b/harmony/src/modules/okd/installation.rs @@ -61,8 +61,10 @@ use crate::{ interpret::{Interpret, InterpretError, InterpretName, InterpretStatus, Outcome}, inventory::{HostRole, Inventory}, modules::{ - dhcp::DhcpHostBindingScore, http::IPxeMacBootFileScore, + dhcp::DhcpHostBindingScore, + http::{IPxeMacBootFileScore, StaticFilesHttpScore}, inventory::LaunchDiscoverInventoryAgentScore, + okd::dns::OKDDnsScore, }, score::Score, topology::{HAClusterTopology, HostBinding}, @@ -280,6 +282,11 @@ impl Interpret for OKDSetup01InventoryInterpret { inventory: &Inventory, topology: &HAClusterTopology, ) -> Result { + info!("Setting up base DNS config for OKD"); + OKDDnsScore::new(topology) + .interpret(inventory, topology) + .await?; + info!( "Launching discovery agent, make sure that your nodes are successfully PXE booted and running inventory agent. They should answer on `http://:8080/inventory`" ); @@ -298,7 +305,7 @@ impl Interpret for OKDSetup01InventoryInterpret { if all_hosts.is_empty() { warn!("No discovered hosts found yet. Waiting for hosts to appear..."); // Sleep to avoid spamming the user and logs while waiting for nodes. - tokio::time::sleep(std::time::Duration::from_secs(5)); + tokio::time::sleep(std::time::Duration::from_secs(3)).await; continue; } @@ -339,37 +346,6 @@ impl Interpret for OKDSetup01InventoryInterpret { bootstrap_host.summary() ), )) - // info!( - // "Launching discovery agent, make sure that your nodes are successfully PXE booted and running inventory agent. They should answer on `http://:8080/inventory`" - // ); - // LaunchDiscoverInventoryAgentScore { - // discovery_timeout: Some(60), - // } - // .interpret(inventory, topology) - // .await?; - // - // // TODO write a loop - // let bootstrap_host: PhysicalHost; - // let mut found_bootstrap_host = false; - // let host_repo = InventoryRepositoryFactory::build().await?; - // while !found_bootstrap_host { - // let all_hosts = host_repo.get_all_hosts().await?; - // // TODO use inquire to select among the current hosts, tell the user to cancel if he - // // wants to update the list. I believe inquire::Select is the correct option here - // // - // // The options are all_hosts, all_hosts is of type Vec and PhysicalHost - // // has a human friendly `summary() -> String` method that is perfect to have the user - // // choose - // // - // // once the user has chosen one, call host_repo.save_role_mapping(Role::Bootstrap, - // // host.id).await?; - // bootstrap_host = todo!(); - // } - // - // Ok(Outcome::new( - // InterpretStatus::SUCCESS, - // format!("Found bootstrap node : {}", bootstrap_host.summary()), - // )) } } @@ -411,10 +387,7 @@ impl OKDSetup02BootstrapInterpret { } } - async fn get_bootstrap_node( - &self, - _inventory: &Inventory, - ) -> Result { + async fn get_bootstrap_node(&self) -> Result { let repo = InventoryRepositoryFactory::build().await?; match repo .get_host_for_role(HostRole::Bootstrap) @@ -429,6 +402,19 @@ impl OKDSetup02BootstrapInterpret { } } + async fn prepare_ignition_files( + &self, + inventory: &Inventory, + topology: &HAClusterTopology, + ) -> Result<(), InterpretError> { + StaticFilesHttpScore { + folder_to_serve: None, + files: todo!(), + } + .interpret(inventory, topology) + .await?; + } + async fn configure_host_binding( &self, inventory: &Inventory, @@ -436,12 +422,13 @@ impl OKDSetup02BootstrapInterpret { ) -> Result<(), InterpretError> { let binding = HostBinding { logical_host: topology.bootstrap_host.clone(), - physical_host: self.get_bootstrap_node(inventory).await?, + physical_host: self.get_bootstrap_node().await?, }; info!("Configuring host binding for bootstrap node {binding:?}"); DhcpHostBindingScore { host_binding: vec![binding], + domain: Some(topology.domain_name), } .interpret(inventory, topology) .await?; @@ -455,7 +442,7 @@ impl OKDSetup02BootstrapInterpret { ) -> Result<(), InterpretError> { // Placeholder: use Harmony templates to emit {MAC}.ipxe selecting SCOS live + bootstrap ignition. info!("[Bootstrap] Rendering per-MAC PXE for bootstrap node"); - let bootstrap_node = self.get_bootstrap_node(inventory).await?; + let bootstrap_node = self.get_bootstrap_node().await?; IPxeMacBootFileScore { mac_address: bootstrap_node.get_mac_address(), content: todo!("templace for bootstrap node"), @@ -465,6 +452,14 @@ impl OKDSetup02BootstrapInterpret { Ok(()) } + async fn setup_bootstrap_load_balancer( + &self, + inventory: &Inventory, + topology: &HAClusterTopology, + ) -> Result<(), InterpretError> { + todo!("OKD loadbalancer score already exists, just call it here probably? 6443 22623, 80 and 443 \n\nhttps://docs.okd.io/latest/installing/installing_bare_metal/upi/installing-bare-metal.html#installation-load-balancing-user-infra_installing-bare-metal"); + } + async fn reboot_target(&self) -> Result<(), InterpretError> { // Placeholder: ssh reboot using the inventory ephemeral key info!("[Bootstrap] Rebooting bootstrap node via SSH"); @@ -502,7 +497,13 @@ impl Interpret for OKDSetup02BootstrapInterpret { topology: &HAClusterTopology, ) -> Result { self.configure_host_binding(inventory, topology).await?; + self.prepare_ignition_files(inventory, topology).await?; self.render_per_mac_pxe(inventory, topology).await?; + self.setup_bootstrap_load_balancer(inventory, topology).await?; + + // TODO https://docs.okd.io/latest/installing/installing_bare_metal/upi/installing-bare-metal.html#installation-user-provisioned-validating-dns_installing-bare-metal + // self.validate_dns_config(inventory, topology).await?; + self.reboot_target().await?; self.wait_for_bootstrap_complete().await?; diff --git a/opnsense-config/src/modules/dhcp.rs b/opnsense-config/src/modules/dhcp.rs index 8ec3519..a59b1a9 100644 --- a/opnsense-config/src/modules/dhcp.rs +++ b/opnsense-config/src/modules/dhcp.rs @@ -1,4 +1,4 @@ -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum DhcpError { InvalidMacAddress(String), InvalidIpAddress(String), diff --git a/opnsense-config/src/modules/dnsmasq.rs b/opnsense-config/src/modules/dnsmasq.rs index e6417a8..001f442 100644 --- a/opnsense-config/src/modules/dnsmasq.rs +++ b/opnsense-config/src/modules/dnsmasq.rs @@ -69,9 +69,9 @@ impl<'a> DhcpConfigDnsMasq<'a> { /// This function implements specific logic to handle existing entries: /// - If no host exists for the given IP or hostname, a new entry is created. /// - If exactly one host exists for the IP and/or hostname, the new MAC is appended to it. - /// - It will panic if the IP and hostname exist but point to two different host entries, + /// - It will error if the IP and hostname exist but point to two different host entries, /// as this represents an unresolvable conflict. - /// - It will also panic if multiple entries are found for the IP or hostname, indicating an + /// - It will also error if multiple entries are found for the IP or hostname, indicating an /// ambiguous state. pub fn add_static_mapping( &mut self, @@ -79,6 +79,10 @@ impl<'a> DhcpConfigDnsMasq<'a> { ipaddr: Ipv4Addr, hostname: &str, ) -> Result<(), DhcpError> { + let mut hostname_split = hostname.split("."); + let hostname = hostname_split.next().expect("hostname cannot be empty"); + let domain_name = hostname_split.collect::>().join("."); + if !Self::is_valid_mac(mac) { return Err(DhcpError::InvalidMacAddress(mac.to_string())); } @@ -107,10 +111,10 @@ impl<'a> DhcpConfigDnsMasq<'a> { && !hostname_indices.is_empty() && ip_set.intersection(&hostname_set).count() == 0 { - panic!( + return Err(DhcpError::Configuration(format!( "Configuration conflict: IP {} and hostname '{}' exist, but in different static host entries.", ipaddr, hostname - ); + ))); } let mut all_indices: Vec<&usize> = ip_set.union(&hostname_set).collect(); @@ -129,6 +133,7 @@ impl<'a> DhcpConfigDnsMasq<'a> { hwaddr: mac.to_string().into(), local: MaybeString::from("1"), ignore: Some(0), + domain: domain_name.into(), ..Default::default() }; hosts.push(new_host); @@ -162,7 +167,7 @@ impl<'a> DhcpConfigDnsMasq<'a> { let mut updated_macs = host_to_modify.hwaddr.content_string().to_string(); updated_macs.push(','); updated_macs.push_str(mac); - host_to_modify.hwaddr = updated_macs.into(); + host_to_modify.hwaddr.content = updated_macs.into(); } else { info!( "MAC {} already present in static host entry for {} ({}). No changes made.", @@ -171,10 +176,10 @@ impl<'a> DhcpConfigDnsMasq<'a> { } } _ => { - panic!( + return Err(DhcpError::Configuration(format!( "Configuration conflict: Found multiple host entries matching IP {} and/or hostname '{}'. Cannot resolve automatically.", ipaddr, hostname - ); + ))); } } @@ -202,8 +207,11 @@ impl<'a> DhcpConfigDnsMasq<'a> { .exec("configctl dhcpd list static") .await?; - let value: serde_json::Value = serde_json::from_str(&list_static_output) - .unwrap_or_else(|_| panic!("Got invalid json from configctl {list_static_output}")); + let value: serde_json::Value = serde_json::from_str(&list_static_output).map_err(|e| { + Error::Command(format!( + "Got invalid json from configctl {list_static_output} : {e}" + )) + })?; // The JSON output key might be 'dhcpd' even when dnsmasq is the backend. let static_maps = value["dhcpd"] @@ -358,6 +366,28 @@ mod test { assert!(Uuid::parse_str(&host.uuid).is_ok()); } + #[test] + fn test_hostname_split_into_host_domain() { + let mut dhcp_config = setup_test_env(vec![]); + let ip = Ipv4Addr::new(192, 168, 1, 10); + let mac = "00:11:22:33:44:55"; + let hostname = "new-host"; + let domain = "some.domain"; + + dhcp_config + .add_static_mapping(mac, ip, &format!("{hostname}.{domain}")) + .unwrap(); + + let hosts = &dhcp_config.opnsense.dnsmasq.as_ref().unwrap().hosts; + assert_eq!(hosts.len(), 1); + let host = &hosts[0]; + assert_eq!(host.host, hostname); + assert_eq!(host.domain.content_string(), domain); + assert_eq!(host.ip, ip.to_string().into()); + assert_eq!(host.hwaddr.content_string(), mac); + assert!(Uuid::parse_str(&host.uuid).is_ok()); + } + #[test] fn test_add_mac_to_existing_host_by_ip_and_hostname() { let initial_host = create_host( @@ -462,41 +492,33 @@ mod test { } #[test] - #[should_panic( - expected = "Configuration conflict: IP 192.168.1.10 and hostname 'host-b' exist, but in different static host entries." - )] - fn test_panic_on_conflicting_ip_and_hostname() { + fn test_error_on_conflicting_ip_and_hostname() { let host_a = create_host("uuid-a", "host-a", "192.168.1.10", "AA:AA:AA:AA:AA:AA"); let host_b = create_host("uuid-b", "host-b", "192.168.1.20", "BB:BB:BB:BB:BB:BB"); let mut dhcp_config = setup_test_env(vec![host_a, host_b]); + let result = dhcp_config.add_static_mapping( + "CC:CC:CC:CC:CC:CC", + Ipv4Addr::new(192, 168, 1, 10), + "host-b", + ); // This IP belongs to host-a, but the hostname belongs to host-b. - dhcp_config - .add_static_mapping( - "CC:CC:CC:CC:CC:CC", - Ipv4Addr::new(192, 168, 1, 10), - "host-b", - ) - .unwrap(); + assert_eq!(result, Err(DhcpError::Configuration("Configuration conflict: IP 192.168.1.10 and hostname 'host-b' exist, but in different static host entries.".to_string()))); } #[test] - #[should_panic( - expected = "Configuration conflict: Found multiple host entries matching IP 192.168.1.30 and/or hostname 'new-host'." - )] - fn test_panic_on_multiple_ip_matches() { + fn test_error_on_multiple_ip_matches() { let host_a = create_host("uuid-a", "host-a", "192.168.1.30", "AA:AA:AA:AA:AA:AA"); let host_b = create_host("uuid-b", "host-b", "192.168.1.30", "BB:BB:BB:BB:BB:BB"); let mut dhcp_config = setup_test_env(vec![host_a, host_b]); // This IP is ambiguous. - dhcp_config - .add_static_mapping( - "CC:CC:CC:CC:CC:CC", - Ipv4Addr::new(192, 168, 1, 30), - "new-host", - ) - .unwrap(); + let result = dhcp_config.add_static_mapping( + "CC:CC:CC:CC:CC:CC", + Ipv4Addr::new(192, 168, 1, 30), + "new-host", + ); + assert_eq!(result, Err(DhcpError::Configuration("Configuration conflict: Found multiple host entries matching IP 192.168.1.30 and/or hostname 'new-host'. Cannot resolve automatically.".to_string()))); } #[test]