From 069bb5963d46298f0e6092c0aa0f634ecdf80ff5 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 14:36:00 +0100 Subject: [PATCH 01/10] feat(config): rename for consistency Signed-off-by: Fredi Raspall --- config/src/external/overlay/vpc.rs | 2 +- config/src/external/overlay/vpcpeering.rs | 17 ++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/config/src/external/overlay/vpc.rs b/config/src/external/overlay/vpc.rs index cde45ad0e..dced6da1d 100644 --- a/config/src/external/overlay/vpc.rs +++ b/config/src/external/overlay/vpc.rs @@ -148,7 +148,7 @@ impl Vpc { local: local.clone(), remote: remote.clone(), remote_id: remote_id.clone(), - gwgroup: p.gw_group.clone(), + gwgroup: p.gwgroup.clone(), adv_communities: vec![], } }) diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index 1da3018ae..184e169c0 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -587,24 +587,19 @@ impl VpcManifest { #[derive(Clone, Debug)] pub struct VpcPeering { - pub name: String, /* name of peering (key in table) */ - pub left: VpcManifest, /* manifest for one side of the peering */ - pub right: VpcManifest, /* manifest for the other side */ - pub gw_group: Option, /* name of gateway group */ + pub name: String, /* name of peering (key in table) */ + pub left: VpcManifest, /* manifest for one side of the peering */ + pub right: VpcManifest, /* manifest for the other side */ + pub gwgroup: Option, /* name of gateway group */ } impl VpcPeering { #[must_use] - pub fn new( - name: &str, - left: VpcManifest, - right: VpcManifest, - gw_group: Option, - ) -> Self { + pub fn new(name: &str, left: VpcManifest, right: VpcManifest, gwgroup: Option) -> Self { Self { name: name.to_owned(), left, right, - gw_group, + gwgroup, } } #[cfg(test)] From 25bce49f61b7082726f7a8ad88a24ab1e5339cf1 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 14:42:39 +0100 Subject: [PATCH 02/10] feat(config): make VpcManifest::add_expose infallible For some reason, the method was made to return a ConfigResult, which is unnecessary because it is infallible. Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/peering.rs | 6 +-- config/src/external/overlay/tests.rs | 43 ++++++++++----------- config/src/external/overlay/vpcpeering.rs | 3 +- config/src/utils/collapse.rs | 2 +- mgmt/src/tests/mgmt.rs | 18 ++++----- nat/src/stateful/test.rs | 8 ++-- nat/src/stateless/setup/mod.rs | 8 ++-- nat/src/stateless/test.rs | 4 +- 8 files changed, 42 insertions(+), 50 deletions(-) diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index b510c4054..646661d0a 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -15,11 +15,7 @@ impl TryFrom<(&SubnetMap, &str, &GatewayAgentPeeringsPeering)> for VpcManifest { ) -> Result { let mut manifest = VpcManifest::new(vpc_name); for expose in peering.expose.as_ref().unwrap_or(&vec![]) { - manifest - .add_expose(VpcExpose::try_from((subnets, expose))?) - .map_err(|e| { - FromK8sConversionError::Invalid(format!("Failed to add expose: {e}")) - })?; + manifest.add_expose(VpcExpose::try_from((subnets, expose))?); } Ok(manifest) } diff --git a/config/src/external/overlay/tests.rs b/config/src/external/overlay/tests.rs index 292999518..5374de9ca 100644 --- a/config/src/external/overlay/tests.rs +++ b/config/src/external/overlay/tests.rs @@ -30,7 +30,7 @@ pub mod test { .as_range(Prefix::expect_from(("100.64.1.0", 24)).into()) .not_as(Prefix::expect_from(("100.64.1.13", 32)).into()) .not_as(Prefix::expect_from(("100.64.1.14", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn build_manifest_vpc2() -> VpcManifest { @@ -39,7 +39,7 @@ pub mod test { .ip(Prefix::expect_from(("10.0.0.0", 24)).into()) .as_range(Prefix::expect_from(("100.64.2.0", 24)).into()); - m2.add_expose(expose).expect("Should succeed"); + m2.add_expose(expose); m2 } @@ -244,15 +244,14 @@ pub mod test { .ip("3.0.3.0/24".into()); // overlaps with expose1.ips (even without as_range) let mut manifest = VpcManifest::new("VPC-1"); - manifest.add_expose(expose1).expect("Should succeed"); - manifest.add_expose(expose2).expect("Should succeed"); + manifest.add_expose(expose1); + manifest.add_expose(expose2); assert_eq!(manifest.validate(), Ok(())); // Overlap between a manifest's exposes prefixes is not allowed let mut invalid_manifest = manifest.clone(); - invalid_manifest - .add_expose(expose3) - .expect("Should succeed"); + invalid_manifest.add_expose(expose3); + assert_eq!( invalid_manifest.validate(), Err(ConfigError::OverlappingPrefixes( @@ -263,9 +262,8 @@ pub mod test { // Overlap between a manifest's exposes prefixes is not allowed (ips / as_range collision) let mut invalid_manifest = manifest.clone(); - invalid_manifest - .add_expose(expose4) - .expect("Should succeed"); + invalid_manifest.add_expose(expose4); + assert_eq!( invalid_manifest.validate(), Err(ConfigError::OverlappingPrefixes( @@ -276,9 +274,8 @@ pub mod test { // Overlap between a manifest's exposes prefixes is not allowed (ips / ips collision) let mut invalid_manifest = manifest.clone(); - invalid_manifest - .add_expose(expose5) - .expect("Should succeed"); + invalid_manifest.add_expose(expose5); + assert_eq!( invalid_manifest.validate(), Err(ConfigError::OverlappingPrefixes( @@ -510,7 +507,7 @@ pub mod test { let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.50.0", 24)).into()) .not(Prefix::expect_from(("192.168.50.13", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.111.0", 24)).into()) @@ -518,37 +515,37 @@ pub mod test { .not(Prefix::expect_from(("192.168.111.254", 32)).into()) .as_range(Prefix::expect_from(("100.64.200.0", 24)).into()) .not_as(Prefix::expect_from(("100.64.200.12", 31)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc1_with_vpc3() -> VpcManifest { let mut m1 = VpcManifest::new("VPC-1"); let expose = VpcExpose::empty().ip(Prefix::expect_from(("192.168.60.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc1_with_vpc4() -> VpcManifest { let mut m1 = VpcManifest::new("VPC-1"); let expose = VpcExpose::empty().ip(Prefix::expect_from(("192.168.60.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc2() -> VpcManifest { let mut m1 = VpcManifest::new("VPC-2"); let expose = VpcExpose::empty().ip(Prefix::expect_from(("192.168.80.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc2_with_vpc3() -> VpcManifest { let mut m1 = VpcManifest::new("VPC-2"); let expose = VpcExpose::empty().ip(Prefix::expect_from(("192.168.80.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc3() -> VpcManifest { let mut m1 = VpcManifest::new("VPC-3"); let expose = VpcExpose::empty().ip(Prefix::expect_from(("192.168.128.0", 27)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc4() -> VpcManifest { @@ -557,17 +554,17 @@ pub mod test { .ip(Prefix::expect_from(("192.168.201.1", 32)).into()) .ip(Prefix::expect_from(("192.168.202.2", 32)).into()) .ip(Prefix::expect_from(("192.168.203.3", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.204.4", 32)).into()) .as_range(Prefix::expect_from(("100.64.204.4", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.210.0", 29)).into()) .not(Prefix::expect_from(("192.168.210.1", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } diff --git a/config/src/external/overlay/vpcpeering.rs b/config/src/external/overlay/vpcpeering.rs index 184e169c0..97cf31d6e 100644 --- a/config/src/external/overlay/vpcpeering.rs +++ b/config/src/external/overlay/vpcpeering.rs @@ -540,9 +540,8 @@ impl VpcManifest { } Ok(()) } - pub fn add_expose(&mut self, expose: VpcExpose) -> ConfigResult { + pub fn add_expose(&mut self, expose: VpcExpose) { self.exposes.push(expose); - Ok(()) } pub fn validate(&self) -> ConfigResult { if self.name.is_empty() { diff --git a/config/src/utils/collapse.rs b/config/src/utils/collapse.rs index 36ecb9cc4..7a9191b4d 100644 --- a/config/src/utils/collapse.rs +++ b/config/src/utils/collapse.rs @@ -245,7 +245,7 @@ mod tests { .not("2.0.2.128/25".into()) .not("3.0.128.0/17".into()); let mut manifest = VpcManifest::new("VPC-1"); - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); let manifest_empty = VpcManifest::new("VPC-2"); let peering = Peering { name: "test_peering".into(), diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 8a9662ee8..844484eaf 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -70,17 +70,17 @@ pub mod test { let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.60.0", 24)).into()) .not(Prefix::expect_from(("192.168.60.13", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.50.0", 24)).into()) .as_range(Prefix::expect_from(("100.100.50.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.30.0", 24)).into()) .as_range(Prefix::expect_from(("100.100.30.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc2_with_vpc1() -> VpcManifest { @@ -88,17 +88,17 @@ pub mod test { let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.80.0", 24)).into()) .not(Prefix::expect_from(("192.168.80.2", 32)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.70.0", 24)).into()) .as_range(Prefix::expect_from(("200.200.70.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.90.0", 24)).into()) .as_range(Prefix::expect_from(("200.200.90.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc1_with_vpc3() -> VpcManifest { @@ -106,7 +106,7 @@ pub mod test { let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.60.0", 24)).into()) .as_range(Prefix::expect_from(("100.100.60.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn man_vpc3_with_vpc1() -> VpcManifest { @@ -114,12 +114,12 @@ pub mod test { let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.128.0", 27)).into()) .as_range(Prefix::expect_from(("100.30.128.0", 27)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); let expose = VpcExpose::empty() .ip(Prefix::expect_from(("192.168.100.0", 24)).into()) .as_range(Prefix::expect_from(("192.168.100.0", 24)).into()); - m1.add_expose(expose).expect("Should succeed"); + m1.add_expose(expose); m1 } fn sample_vpc_peering_table() -> VpcPeeringTable { diff --git a/nat/src/stateful/test.rs b/nat/src/stateful/test.rs index 7b726e31c..1d2feb7b3 100644 --- a/nat/src/stateful/test.rs +++ b/nat/src/stateful/test.rs @@ -95,7 +95,7 @@ mod tests { #[allow(clippy::too_many_lines)] fn build_overlay_4vpcs() -> Overlay { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); @@ -231,7 +231,7 @@ mod tests { fn build_overlay_2vpcs() -> Overlay { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); @@ -462,7 +462,7 @@ mod tests { fn build_overlay_2vpcs_no_nat() -> Overlay { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); @@ -822,7 +822,7 @@ mod tests { fn build_overlay_2vpcs_with_default() -> Overlay { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); diff --git a/nat/src/stateless/setup/mod.rs b/nat/src/stateless/setup/mod.rs index 59931038c..8a61924dd 100644 --- a/nat/src/stateless/setup/mod.rs +++ b/nat/src/stateless/setup/mod.rs @@ -150,8 +150,8 @@ mod tests { .as_range("4.0.0.0/16".into()); let mut manifest1 = VpcManifest::new("VPC-1"); - manifest1.add_expose(expose1).expect("Failed to add expose"); - manifest1.add_expose(expose2).expect("Failed to add expose"); + manifest1.add_expose(expose1); + manifest1.add_expose(expose2); let expose3 = VpcExpose::empty() .ip("1::/64".into()) @@ -165,8 +165,8 @@ mod tests { .not_as("2:4::/128".into()); let mut manifest2 = VpcManifest::new("VPC-2"); - manifest2.add_expose(expose3).expect("Failed to add expose"); - manifest2.add_expose(expose4).expect("Failed to add expose"); + manifest2.add_expose(expose3); + manifest2.add_expose(expose4); let peering: Peering = Peering { name: "test_peering".into(), diff --git a/nat/src/stateless/test.rs b/nat/src/stateless/test.rs index dd723c131..551b69a4e 100644 --- a/nat/src/stateless/test.rs +++ b/nat/src/stateless/test.rs @@ -362,7 +362,7 @@ mod tests { #[allow(clippy::too_many_lines)] fn build_sample_config() -> GwConfig { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); @@ -755,7 +755,7 @@ mod tests { exposes_right: Vec, ) -> GwConfig { fn add_expose(manifest: &mut VpcManifest, expose: VpcExpose) { - manifest.add_expose(expose).expect("Failed to add expose"); + manifest.add_expose(expose); } let mut vpc_table = VpcTable::new(); From f40ee4a6895beda9daec166589987de34a0c4756 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 15:27:51 +0100 Subject: [PATCH 03/10] feat(config): polish error messages & add error codes Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/communities.rs | 2 +- config/src/converters/k8s/config/expose.rs | 9 +++++---- config/src/converters/k8s/config/interface.rs | 9 +++++---- config/src/errors.rs | 6 ++++-- config/src/external/gwgroup.rs | 4 ++-- 5 files changed, 17 insertions(+), 13 deletions(-) diff --git a/config/src/converters/k8s/config/communities.rs b/config/src/converters/k8s/config/communities.rs index 5988c7b72..5c546b1c8 100644 --- a/config/src/converters/k8s/config/communities.rs +++ b/config/src/converters/k8s/config/communities.rs @@ -16,7 +16,7 @@ impl TryFrom<&GatewayAgentSpec> for PriorityCommunityTable { Some(map) => { for (prio, community) in map { let priority: u32 = prio.parse().map_err(|e| { - Self::Error::ParseError(format!("Community priority '{prio}': {e}")) + Self::Error::ParseError(format!("Community priority {prio}: {e}")) })?; comtable.insert(priority, community)?; } diff --git a/config/src/converters/k8s/config/expose.rs b/config/src/converters/k8s/config/expose.rs index 47c7d5ba4..7d2d4c34a 100644 --- a/config/src/converters/k8s/config/expose.rs +++ b/config/src/converters/k8s/config/expose.rs @@ -18,10 +18,11 @@ fn parse_port_ranges(ports_str: &str) -> Result, FromK8sConversio // Split port ranges for prefix on ',' .split(',') .map(|port_range_str| { - port_range_str - .trim() - .parse::() - .map_err(|e| FromK8sConversionError::ParseError(format!("Invalid port range: {e}"))) + port_range_str.trim().parse::().map_err(|e| { + FromK8sConversionError::ParseError(format!( + "Invalid port range {port_range_str}: {e}" + )) + }) }) .collect() } diff --git a/config/src/converters/k8s/config/interface.rs b/config/src/converters/k8s/config/interface.rs index 900e12226..d6b07c1ae 100644 --- a/config/src/converters/k8s/config/interface.rs +++ b/config/src/converters/k8s/config/interface.rs @@ -29,7 +29,7 @@ impl TryFrom<(&str, &GatewayAgentGatewayInterfaces)> for InterfaceConfig { for ip in ips { let ifaddr = ip.parse::().map_err(|e| { FromK8sConversionError::ParseError(format!( - "Invalid interface address \"{ip}\": {e}" + "Invalid interface address {ip}: {e}" )) })?; interface_config = interface_config.add_address(ifaddr.address, ifaddr.mask_len); @@ -37,14 +37,15 @@ impl TryFrom<(&str, &GatewayAgentGatewayInterfaces)> for InterfaceConfig { } if let Some(iface_mtu) = iface.mtu { - let mtu = Mtu::try_from(iface_mtu) - .map_err(|e| FromK8sConversionError::ParseError(format!("Invalid MTU: {e}")))?; + let mtu = Mtu::try_from(iface_mtu).map_err(|e| { + FromK8sConversionError::ParseError(format!("Invalid MTU {iface_mtu}: {e}")) + })?; interface_config = interface_config.set_mtu(mtu); } if let Some(pci) = &iface.pci { let pci = PciAddress::try_from(pci.as_str()).map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid PCI address: {e}")) + FromK8sConversionError::ParseError(format!("Invalid PCI address {pci}: {e}")) })?; interface_config = interface_config.set_pci(pci); } diff --git a/config/src/errors.rs b/config/src/errors.rs index 6538117a4..6b958ca65 100644 --- a/config/src/errors.rs +++ b/config/src/errors.rs @@ -28,10 +28,12 @@ pub enum ConfigError { DuplicateVpcPeeringId(String), #[error("Peering '{0}' refers to a VPC for which a peering already exists")] DuplicateVpcPeerings(String), - #[error("Duplicated gateway group '{0}'")] + #[error("Duplicated gateway group {0}")] DuplicateGroup(String), - #[error("Duplicated gateway group member '{0}'")] + #[error("Duplicated gateway group member {0}")] DuplicateMember(String), + #[error("Duplicated gateway group member address {0}")] + DuplicateMemberAddress(IpAddr), #[error("A VPC peering object refers to non-existent VPC '{0}'")] NoSuchVpc(String), #[error("A VPC peering object refers to non-existent group '{0}'")] diff --git a/config/src/external/gwgroup.rs b/config/src/external/gwgroup.rs index 5044dbbcb..de8584423 100644 --- a/config/src/external/gwgroup.rs +++ b/config/src/external/gwgroup.rs @@ -80,7 +80,7 @@ impl GwGroup { return Err(ConfigError::DuplicateMember(member.name.clone())); } if self.get_member_by_addr(member.ipaddress).is_some() { - return Err(ConfigError::DuplicateMember(member.ipaddress.to_string())); + return Err(ConfigError::DuplicateMemberAddress(member.ipaddress)); } self.members.push(member); Ok(()) @@ -198,7 +198,7 @@ mod test { let r = group.add_member(GwGroupMember::new("gw1", 99, IpAddr::from_str("172.128.0.4").unwrap())); assert!(r.is_err_and(|e| matches!(e, ConfigError::DuplicateMember(_)))); let r = group.add_member(GwGroupMember::new("gw4", 99, IpAddr::from_str("172.128.0.1").unwrap())); - assert!(r.is_err_and(|e| matches!(e, ConfigError::DuplicateMember(_)))); + assert!(r.is_err_and(|e| matches!(e, ConfigError::DuplicateMemberAddress(_)))); gwtable.add_group(group).unwrap(); // err on duplicate group From 9b986a27400495e8a067345b466e435ce02aa334 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 15:33:41 +0100 Subject: [PATCH 04/10] feat(config): allow empty gateway groups Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/gwgroups.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/config/src/converters/k8s/config/gwgroups.rs b/config/src/converters/k8s/config/gwgroups.rs index 849133c38..2e5f031c8 100644 --- a/config/src/converters/k8s/config/gwgroups.rs +++ b/config/src/converters/k8s/config/gwgroups.rs @@ -34,12 +34,14 @@ impl TryFrom<&GatewayAgentSpec> for GwGroupTable { Some(map) => { for (name, gagroups) in map { let mut group = GwGroup::new(name); - let members = gagroups.members.as_ref().ok_or_else(|| { - Self::Error::MissingData(format!("Gateway group members for group {name}")) - })?; - for m in members { - let member = GwGroupMember::try_from(m)?; - group.add_member(member)?; + if let Some(members) = gagroups.members.as_ref() { + for m in members { + let member = GwGroupMember::try_from(m)?; + group.add_member(member)?; + } + } else { + // we don't complain on empty groups, which are shared resources + // we may want to complain later if a peering refers to an empty group } group_table.add_group(group)?; } From fa99c3a91670d0bb61d0fd57f935c5c4faeb4996 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 16:22:29 +0100 Subject: [PATCH 05/10] feat(config): polish error codes and stringifiers This patch: - renames some result codes to better indicate their nature. - remaps some of the results to better-suited variants. - the goal is to make it easier to know what type or category of error to use in each case and unify the stringification. Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/bgp.rs | 2 +- .../src/converters/k8s/config/communities.rs | 2 +- config/src/converters/k8s/config/expose.rs | 39 ++++++++++--------- .../converters/k8s/config/gateway_config.rs | 6 +-- config/src/converters/k8s/config/gwgroups.rs | 2 +- config/src/converters/k8s/config/interface.rs | 8 ++-- config/src/converters/k8s/config/overlay.rs | 10 ++--- config/src/converters/k8s/config/peering.rs | 4 +- config/src/converters/k8s/config/tracecfg.rs | 4 +- config/src/converters/k8s/config/underlay.rs | 21 +++++----- config/src/converters/k8s/config/vpc.rs | 5 ++- config/src/converters/k8s/mod.rs | 23 ++++++++--- 12 files changed, 69 insertions(+), 57 deletions(-) diff --git a/config/src/converters/k8s/config/bgp.rs b/config/src/converters/k8s/config/bgp.rs index 1d4893338..f0ff87387 100644 --- a/config/src/converters/k8s/config/bgp.rs +++ b/config/src/converters/k8s/config/bgp.rs @@ -16,7 +16,7 @@ impl TryFrom<&GatewayAgentGatewayNeighbors> for BgpNeighbor { fn try_from(neighbor: &GatewayAgentGatewayNeighbors) -> Result { let neighbor_addr = match neighbor.ip.as_ref() { Some(ip) => ip.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid neighbor address {ip}: {e}")) + FromK8sConversionError::InvalidData(format!("neighbor address {ip}: {e}")) })?, None => { return Err(FromK8sConversionError::MissingData(format!( diff --git a/config/src/converters/k8s/config/communities.rs b/config/src/converters/k8s/config/communities.rs index 5c546b1c8..0a3d95b75 100644 --- a/config/src/converters/k8s/config/communities.rs +++ b/config/src/converters/k8s/config/communities.rs @@ -16,7 +16,7 @@ impl TryFrom<&GatewayAgentSpec> for PriorityCommunityTable { Some(map) => { for (prio, community) in map { let priority: u32 = prio.parse().map_err(|e| { - Self::Error::ParseError(format!("Community priority {prio}: {e}")) + Self::Error::InvalidData(format!("community priority {prio}: {e}")) })?; comtable.insert(priority, community)?; } diff --git a/config/src/converters/k8s/config/expose.rs b/config/src/converters/k8s/config/expose.rs index 7d2d4c34a..cad56da1c 100644 --- a/config/src/converters/k8s/config/expose.rs +++ b/config/src/converters/k8s/config/expose.rs @@ -19,9 +19,7 @@ fn parse_port_ranges(ports_str: &str) -> Result, FromK8sConversio .split(',') .map(|port_range_str| { port_range_str.trim().parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!( - "Invalid port range {port_range_str}: {e}" - )) + FromK8sConversionError::InvalidData(format!("port range {port_range_str}: {e}")) }) }) .collect() @@ -57,29 +55,29 @@ fn process_ip_block( )); } (Some(_), None, Some(_)) => { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "Expose ip block must specify either cidr or not, not both".to_string(), )); } (None, Some(_), Some(_)) => { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "Expose ip block must specify either subnet or not, not both".to_string(), )); } (Some(_), Some(_), None) => { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "Expose ip block must specify either subnet or cidr, not both".to_string(), )); } (Some(_), Some(_), Some(_)) => { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "Expose ip block must specify either subnet, cidr, or not, not all three" .to_string(), )); } (None, Some(subnet_name), None) => { let prefix = subnets.get(subnet_name.as_str()).ok_or_else(|| { - FromK8sConversionError::Invalid(format!( + FromK8sConversionError::NotAllowed(format!( "Expose references unknown VPC subnet {subnet_name}" )) })?; @@ -89,7 +87,7 @@ fn process_ip_block( } (Some(cidr), None, None) => { let prefix = cidr.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid CIDR format: {cidr}: {e}")) + FromK8sConversionError::InvalidData(format!("CIDR format: {cidr}: {e}")) })?; for prefix in map_ports(prefix, ip.ports.as_deref())? { vpc_expose = vpc_expose.ip(prefix); @@ -97,7 +95,7 @@ fn process_ip_block( } (None, None, Some(not)) => { let prefix = Prefix::try_from(PrefixString(not.as_str())).map_err(|e| { - FromK8sConversionError::Invalid(format!("Invalid CIDR format: {not}: {e}")) + FromK8sConversionError::InvalidData(format!("CIDR format: {not}: {e}")) })?; for prefix in map_ports(prefix, ip.ports.as_deref())? { vpc_expose = vpc_expose.not(prefix); @@ -118,13 +116,13 @@ fn process_as_block( )); } (Some(_), Some(_)) => { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "Expose as block must specify either cidr or not, not both".to_string(), )); } (Some(cidr), None) => { let prefix = cidr.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid CIDR format: {cidr}: {e}")) + FromK8sConversionError::InvalidData(format!("CIDR format: {cidr}: {e}")) })?; for prefix in map_ports(prefix, ip.ports.as_deref())? { vpc_expose = vpc_expose.as_range(prefix); @@ -132,7 +130,7 @@ fn process_as_block( } (None, Some(not)) => { let prefix = Prefix::try_from(PrefixString(not.as_str())).map_err(|e| { - FromK8sConversionError::Invalid(format!("Invalid CIDR format: {not}: {e}")) + FromK8sConversionError::InvalidData(format!("CIDR format: {not}: {e}")) })?; for prefix in map_ports(prefix, ip.ports.as_deref())? { vpc_expose = vpc_expose.not_as(prefix); @@ -148,7 +146,7 @@ fn process_nat_block( ) -> Result { match nat { Some(nat) => match (&nat.stateful, &nat.stateless) { - (Some(_), Some(_)) => Err(FromK8sConversionError::Invalid( + (Some(_), Some(_)) => Err(FromK8sConversionError::NotAllowed( "Cannot have both stateful and stateless nat configured on the same expose block" .to_string(), )), @@ -156,11 +154,11 @@ fn process_nat_block( let idle_timeout = stateful.idle_timeout.map(std::time::Duration::from); vpc_expose .make_stateful_nat(idle_timeout) - .map_err(|e| FromK8sConversionError::Invalid(e.to_string())) + .map_err(|e| FromK8sConversionError::NotAllowed(e.to_string())) } (None, Some(_)) => vpc_expose .make_stateless_nat() - .map_err(|e| FromK8sConversionError::Invalid(e.to_string())), + .map_err(|e| FromK8sConversionError::NotAllowed(e.to_string())), (None, None) => Ok(vpc_expose), // Rely on default behavior for NAT }, None => Ok(vpc_expose), @@ -178,12 +176,12 @@ impl TryFrom<(&SubnetMap, &GatewayAgentPeeringsPeeringExpose)> for VpcExpose { vpc_expose.default = expose.default.unwrap_or(false); if vpc_expose.default { if expose.ips.as_ref().is_some_and(|ips| !ips.is_empty()) { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "A Default expose can't contain prefixes".to_string(), )); } if expose.r#as.as_ref().is_some_and(|r#as| !r#as.is_empty()) { - return Err(FromK8sConversionError::Invalid( + return Err(FromK8sConversionError::NotAllowed( "A Default expose can't contain 'as' prefixes".to_string(), )); } @@ -300,7 +298,10 @@ mod test { let result = map_ports(prefix, Some("80,invalid,443")); assert!(result.is_err()); - assert!(matches!(result, Err(FromK8sConversionError::ParseError(_)))); + assert!(matches!( + result, + Err(FromK8sConversionError::InvalidData(_)) + )); } #[test] diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index 50bdc33f1..f365a865d 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -20,13 +20,13 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { .metadata .name .as_ref() - .ok_or(FromK8sConversionError::MissingData( + .ok_or(FromK8sConversionError::K8sInfra( "metadata.name not found".to_string(), ))? .as_str(); let Some(gen_id) = ga.metadata.generation else { - return Err(FromK8sConversionError::Invalid(format!( + return Err(FromK8sConversionError::K8sInfra(format!( "metadata.generation not found for {name}" ))); }; @@ -34,7 +34,7 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { let device = DeviceConfig::try_from(ga)?; let mut underlay = Underlay::try_from(ga.spec.gateway.as_ref().ok_or( - FromK8sConversionError::MissingData(format!( + FromK8sConversionError::K8sInfra(format!( "gateway section not found in spec for {name}" )), )?)?; diff --git a/config/src/converters/k8s/config/gwgroups.rs b/config/src/converters/k8s/config/gwgroups.rs index 2e5f031c8..a65c16be1 100644 --- a/config/src/converters/k8s/config/gwgroups.rs +++ b/config/src/converters/k8s/config/gwgroups.rs @@ -13,7 +13,7 @@ impl TryFrom<&GatewayAgentGroupsMembers> for GwGroupMember { fn try_from(value: &GatewayAgentGroupsMembers) -> Result { let address = value.vtep_ip.as_str(); let ipaddress = parse_address(address) - .map_err(|e| Self::Error::ParseError(format!("Invalid ip address {address}: {e}")))?; + .map_err(|e| Self::Error::InvalidData(format!("ip address {address}: {e}")))?; Ok(Self { name: value.name.clone(), diff --git a/config/src/converters/k8s/config/interface.rs b/config/src/converters/k8s/config/interface.rs index d6b07c1ae..c029ffd89 100644 --- a/config/src/converters/k8s/config/interface.rs +++ b/config/src/converters/k8s/config/interface.rs @@ -28,9 +28,7 @@ impl TryFrom<(&str, &GatewayAgentGatewayInterfaces)> for InterfaceConfig { if let Some(ips) = iface.ips.as_ref() { for ip in ips { let ifaddr = ip.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!( - "Invalid interface address {ip}: {e}" - )) + FromK8sConversionError::InvalidData(format!("interface address {ip}: {e}")) })?; interface_config = interface_config.add_address(ifaddr.address, ifaddr.mask_len); } @@ -38,14 +36,14 @@ impl TryFrom<(&str, &GatewayAgentGatewayInterfaces)> for InterfaceConfig { if let Some(iface_mtu) = iface.mtu { let mtu = Mtu::try_from(iface_mtu).map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid MTU {iface_mtu}: {e}")) + FromK8sConversionError::InvalidData(format!("MTU {iface_mtu}: {e}")) })?; interface_config = interface_config.set_mtu(mtu); } if let Some(pci) = &iface.pci { let pci = PciAddress::try_from(pci.as_str()).map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid PCI address {pci}: {e}")) + FromK8sConversionError::InvalidData(format!("PCI address {pci}: {e}")) })?; interface_config = interface_config.set_pci(pci); } diff --git a/config/src/converters/k8s/config/overlay.rs b/config/src/converters/k8s/config/overlay.rs index d7f37bd3d..2ea5a520a 100644 --- a/config/src/converters/k8s/config/overlay.rs +++ b/config/src/converters/k8s/config/overlay.rs @@ -23,8 +23,8 @@ fn extract_subnets( continue; }; let prefix = cidr.parse::().map_err(|e| { - FromK8sConversionError::Invalid(format!( - "Invalid vpc subnet CIDR {cidr} for vpc {vpc_name}: {e}" + FromK8sConversionError::InvalidData(format!( + "vpc subnet CIDR {cidr} for vpc {vpc_name}: {e}" )) })?; subnets.insert(subnet_name.clone(), prefix); @@ -41,7 +41,7 @@ fn make_vpc_table( for (vpc_name, k8s_vpc) in vpcs { let vpc = Vpc::try_from((vpc_name.as_str(), k8s_vpc))?; vpc_table.add(vpc).map_err(|e| { - FromK8sConversionError::Invalid(format!("Cannot add vpc {vpc_name}: {e}")) + FromK8sConversionError::InternalError(format!("Cannot add vpc {vpc_name}: {e}")) })?; } Ok(vpc_table) @@ -55,7 +55,7 @@ fn make_peering_table( for (peering_name, k8s_peering) in peerings { let peering = VpcPeering::try_from((vpc_subnets, peering_name.as_str(), k8s_peering))?; peering_table.add(peering).map_err(|e| { - FromK8sConversionError::Invalid(format!("Cannot add peering {peering_name}: {e}")) + FromK8sConversionError::InternalError(format!("Cannot add peering {peering_name}: {e}")) })?; } Ok(peering_table) @@ -67,7 +67,7 @@ impl TryFrom<&GatewayAgentSpec> for Overlay { fn try_from(spec: &GatewayAgentSpec) -> Result { match (spec.vpcs.as_ref(), spec.peerings.as_ref()) { (None, None) => Ok(Overlay::new(VpcTable::new(), VpcPeeringTable::new())), - (None, Some(peerings)) => Err(FromK8sConversionError::Invalid(format!( + (None, Some(peerings)) => Err(FromK8sConversionError::NotAllowed(format!( "Found 0 vpcs but {} peerings", peerings.len() ))), diff --git a/config/src/converters/k8s/config/peering.rs b/config/src/converters/k8s/config/peering.rs index 646661d0a..5dad23f08 100644 --- a/config/src/converters/k8s/config/peering.rs +++ b/config/src/converters/k8s/config/peering.rs @@ -50,8 +50,8 @@ impl TryFrom<(&VpcSubnetMap, &str, &GatewayAgentPeerings)> for VpcPeering { Ok(VpcPeering::new(peering_name, left, right, gwgroup)) } else { - Err(FromK8sConversionError::Invalid( - "Missing peering".to_string(), + Err(FromK8sConversionError::MissingData( + "Vpc reference in peering".to_string(), )) } } diff --git a/config/src/converters/k8s/config/tracecfg.rs b/config/src/converters/k8s/config/tracecfg.rs index b96f9f540..19f893eb2 100644 --- a/config/src/converters/k8s/config/tracecfg.rs +++ b/config/src/converters/k8s/config/tracecfg.rs @@ -15,8 +15,8 @@ fn levelstring_to_levelfilter(value: Option<&str>) -> Result Ok(LevelFilter::INFO), Some("debug") => Ok(LevelFilter::DEBUG), Some("trace") => Ok(LevelFilter::TRACE), - Some(val) => Err(FromK8sConversionError::ParseError(format!( - "Invalid log level value: {val}" + Some(val) => Err(FromK8sConversionError::InvalidData(format!( + "log-level value: {val}" ))), } } diff --git a/config/src/converters/k8s/config/underlay.rs b/config/src/converters/k8s/config/underlay.rs index 84fc8e667..1f094958a 100644 --- a/config/src/converters/k8s/config/underlay.rs +++ b/config/src/converters/k8s/config/underlay.rs @@ -28,9 +28,9 @@ fn add_hardcoded_interfaces( .ok_or(FromK8sConversionError::MissingData( "Gateway VTEP IP not specified".to_string(), ))?; - let vtep_ip = vtep_ip_raw.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid VTEP IP {vtep_ip_raw}: {e}")) - })?; + let vtep_ip = vtep_ip_raw + .parse::() + .map_err(|e| FromK8sConversionError::InvalidData(format!("VTEP IP {vtep_ip_raw}: {e}")))?; // Loopback let mut lo = InterfaceConfig::new("lo", InterfaceType::Loopback, false); @@ -40,7 +40,7 @@ fn add_hardcoded_interfaces( // VTEP let vtep_mac = if let Some(vtep_mac_raw) = gateway.vtep_mac.as_ref() { let vtep_mac = vtep_mac_raw.parse::().map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid VTEP MAC {vtep_mac_raw}: {e}")) + FromK8sConversionError::InvalidData(format!("VTEP MAC {vtep_mac_raw}: {e}")) })?; Some(vtep_mac) } else { @@ -50,7 +50,7 @@ fn add_hardcoded_interfaces( let vtep_ipv4 = match vtep_ip.address { IpAddr::V4(v4) => v4, IpAddr::V6(v6) => { - return Err(FromK8sConversionError::Invalid(format!( + return Err(FromK8sConversionError::InvalidData(format!( "VTEP IP {v6} is not IPv4" ))); } @@ -99,7 +99,7 @@ fn add_bgp_config( ))?; let router_id = parse_address_v4(protocol_ip).map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid IPv4 protocol IP {protocol_ip}: {e}")) + FromK8sConversionError::InvalidData(format!("IPv4 protocol IP {protocol_ip}: {e}")) })?; let vtep_ip_raw = gateway @@ -109,12 +109,11 @@ fn add_bgp_config( "Gateway VTEP IP not specified".to_string(), ))?; - let vtep_prefix = Prefix::try_from(PrefixString(vtep_ip_raw)).map_err(|e| { - FromK8sConversionError::ParseError(format!("Invalid VTEP IP {vtep_ip_raw}: {e}")) - })?; + let vtep_prefix = Prefix::try_from(PrefixString(vtep_ip_raw)) + .map_err(|e| FromK8sConversionError::InvalidData(format!("VTEP IP {vtep_ip_raw}: {e}")))?; if !vtep_prefix.is_ipv4() { - return Err(FromK8sConversionError::Invalid(format!( - "Invalid VTEP IP {vtep_ip_raw}: not an IPv4 prefix" + return Err(FromK8sConversionError::InvalidData(format!( + "VTEP IP {vtep_ip_raw}: not an IPv4 prefix" ))); } diff --git a/config/src/converters/k8s/config/vpc.rs b/config/src/converters/k8s/config/vpc.rs index efab73819..7efb2c57f 100644 --- a/config/src/converters/k8s/config/vpc.rs +++ b/config/src/converters/k8s/config/vpc.rs @@ -28,8 +28,9 @@ impl TryFrom<(&str, &GatewayAgentVpcs)> for Vpc { ))?; // Create a new VPC with name and VNI - Vpc::new(vpc_name, internal_id, *vni) - .map_err(|e| FromK8sConversionError::Invalid(format!("Could not create VPC: {e}"))) + Vpc::new(vpc_name, internal_id, *vni).map_err(|e| { + FromK8sConversionError::InternalError(format!("Could not create VPC: {e}")) + }) } } diff --git a/config/src/converters/k8s/mod.rs b/config/src/converters/k8s/mod.rs index 6734e991e..2167dc5f8 100644 --- a/config/src/converters/k8s/mod.rs +++ b/config/src/converters/k8s/mod.rs @@ -11,14 +11,27 @@ use thiserror::Error; #[derive(Debug, Error)] pub enum FromK8sConversionError { - #[error("Invalid Gateway Agent object: {0}")] - Invalid(String), + /// Error that occurs when required, expected (meta)data is missing + #[error("k8s infra: {0}")] + K8sInfra(String), + + /// An object within the configuration lacks a value for the object to make any sense #[error("Missing required data: {0}")] MissingData(String), - #[error("Could not parse value: {0}")] - ParseError(String), - #[error("Internal CRD object conversion error: {0}")] + + /// A value that is invalid such as a malformed IP or MAC address, an out of range integer, etc. + #[error("Invalid {0}")] + InvalidData(String), + + /// Something that we don't allow because the system would malfunction or because it's not supported atm + #[error("Not allowed: {0}")] + NotAllowed(String), + + /// An error that occurs while processing the configuration which prevents processing or validating it + #[error("Internal failure: {0}")] InternalError(String), + + /// A validation error, generally. #[error("Configuration error: {0}")] ConfigError(#[from] ConfigError), } From ee9bd79553fd4ccfa464db16bf2672408e22d0f6 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 18:01:56 +0100 Subject: [PATCH 06/10] feat(config): simplify conversion from k8s Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/device.rs | 32 ++++++++----------- .../converters/k8s/config/gateway_config.rs | 17 ++++++---- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/config/src/converters/k8s/config/device.rs b/config/src/converters/k8s/config/device.rs index 6b9820248..1be92a9bc 100644 --- a/config/src/converters/k8s/config/device.rs +++ b/config/src/converters/k8s/config/device.rs @@ -1,27 +1,18 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -use k8s_intf::gateway_agent_crd::GatewayAgent; +use k8s_intf::gateway_agent_crd::GatewayAgentGateway; use crate::converters::k8s::FromK8sConversionError; use crate::internal::device::DeviceConfig; use crate::internal::device::tracecfg::TracingConfig; -impl TryFrom<&GatewayAgent> for DeviceConfig { +impl TryFrom<&GatewayAgentGateway> for DeviceConfig { type Error = FromK8sConversionError; - fn try_from(ga: &GatewayAgent) -> Result { + fn try_from(gagw: &GatewayAgentGateway) -> Result { let mut device_config = DeviceConfig::new(); - - if let Some(logs) = &ga - .spec - .gateway - .as_ref() - .ok_or(FromK8sConversionError::MissingData( - "gateway section is required".to_string(), - ))? - .logs - { + if let Some(logs) = &gagw.logs { device_config.set_tracing(TracingConfig::try_from(logs)?); } Ok(device_config) @@ -33,6 +24,7 @@ mod test { use super::*; use k8s_intf::bolero::LegalValue; + use k8s_intf::gateway_agent_crd::GatewayAgent; #[test] fn test_simple_hostname() { @@ -40,12 +32,14 @@ mod test { .with_type::>() .for_each(|ga| { let ga = ga.as_ref(); - let dev = DeviceConfig::try_from(ga).unwrap(); - // Make sure we set tracing, the conversion is tested as part of the `TraceConfig` conversion - assert_eq!( - ga.spec.gateway.as_ref().unwrap().logs.is_some(), - dev.tracing.is_some() - ); + if let Some(gw_agent_gw) = &ga.spec.gateway { + let dev = DeviceConfig::try_from(gw_agent_gw).unwrap(); + // Make sure we set tracing, the conversion is tested as part of the `TraceConfig` conversion + assert_eq!( + ga.spec.gateway.as_ref().unwrap().logs.is_some(), + dev.tracing.is_some() + ); + } }); } } diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index f365a865d..565c7eea9 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -27,17 +27,20 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { let Some(gen_id) = ga.metadata.generation else { return Err(FromK8sConversionError::K8sInfra(format!( - "metadata.generation not found for {name}" + "metadata.generation not found for gateway {name}" ))); }; - let device = DeviceConfig::try_from(ga)?; + let ga_spec_gw = ga + .spec + .gateway + .as_ref() + .ok_or(FromK8sConversionError::K8sInfra(format!( + "gateway section not found in spec for gateway {name}" + )))?; - let mut underlay = Underlay::try_from(ga.spec.gateway.as_ref().ok_or( - FromK8sConversionError::K8sInfra(format!( - "gateway section not found in spec for {name}" - )), - )?)?; + let device = DeviceConfig::try_from(ga_spec_gw)?; + let mut underlay = Underlay::try_from(ga_spec_gw)?; // fabricBFD variable check: enable BFD on fabric-facing BGP neighbors let fabric_bfd_enabled = ga From 8dca31d1cb050f7b66c45accf852d952f28b597f Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 21:59:14 +0100 Subject: [PATCH 07/10] feat(config,validator): ensure that validator and ...dataplane use the same validation logic. This patch will be ferther simplified later. Signed-off-by: Fredi Raspall --- .../converters/k8s/config/gateway_config.rs | 100 ++++++++++++++---- validator/src/main.rs | 43 +------- 2 files changed, 80 insertions(+), 63 deletions(-) diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index 565c7eea9..d45240ec4 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -1,43 +1,96 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -use k8s_intf::gateway_agent_crd::GatewayAgent; +use k8s_intf::gateway_agent_crd::{GatewayAgent, GatewayAgentSpec}; -use crate::DeviceConfig; use crate::converters::k8s::FromK8sConversionError; use crate::external::communities::PriorityCommunityTable; use crate::external::gwgroup::GwGroupTable; use crate::external::overlay::Overlay; use crate::external::underlay::Underlay; use crate::external::{ExternalConfig, ExternalConfigBuilder}; +use crate::{DeviceConfig, GenId}; + +/// A struct synthesizing the data we get from k8s +pub struct K8sInput { + pub gwname: String, + pub genid: GenId, + pub spec: GatewayAgentSpec, +} + +/// Validate the metadata of a `GatewayAgent`. +/// # Errors +/// Returns `FromK8sConversionError` in case data is missing or is invalid +pub fn validate_metadata(ga: &GatewayAgent) -> Result { + let genid = ga + .metadata + .generation + .ok_or(FromK8sConversionError::K8sInfra( + "Missing metadata generation Id".to_string(), + ))?; + + if genid == 0 { + return Err(FromK8sConversionError::K8sInfra( + "Invalid metadata generation Id".to_string(), + )); + } + + let gwname = ga + .metadata + .name + .as_ref() + .ok_or(FromK8sConversionError::K8sInfra( + "Missing metadata gateway name".to_string(), + ))?; + + if gwname.is_empty() { + return Err(FromK8sConversionError::K8sInfra( + "Empty gateway name".to_string(), + )); + } + let namespace = ga + .metadata + .namespace + .as_ref() + .ok_or(FromK8sConversionError::K8sInfra( + "Missing namespace".to_string(), + ))?; + + if namespace.as_str() != "fab" { + return Err(FromK8sConversionError::K8sInfra(format!( + "Invalid namespace {namespace}" + ))); + } + + let _ = ga + .spec + .gateway + .as_ref() + .ok_or(FromK8sConversionError::K8sInfra(format!( + "Missing gateway section in spec for gateway {gwname}" + )))?; + + let spec = K8sInput { + gwname: gwname.clone(), + genid, + spec: ga.spec.clone(), + }; + + Ok(spec) +} /// Convert from `GatewayAgent` (k8s CRD) to `ExternalConfig` with default values impl TryFrom<&GatewayAgent> for ExternalConfig { type Error = FromK8sConversionError; fn try_from(ga: &GatewayAgent) -> Result { - let name = ga - .metadata - .name - .as_ref() - .ok_or(FromK8sConversionError::K8sInfra( - "metadata.name not found".to_string(), - ))? - .as_str(); - - let Some(gen_id) = ga.metadata.generation else { - return Err(FromK8sConversionError::K8sInfra(format!( - "metadata.generation not found for gateway {name}" - ))); - }; - - let ga_spec_gw = ga + let input = validate_metadata(ga)?; + + let ga_spec_gw = input .spec .gateway .as_ref() - .ok_or(FromK8sConversionError::K8sInfra(format!( - "gateway section not found in spec for gateway {name}" - )))?; + .unwrap_or_else(|| unreachable!()); let device = DeviceConfig::try_from(ga_spec_gw)?; let mut underlay = Underlay::try_from(ga_spec_gw)?; @@ -61,7 +114,7 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { let comtable = PriorityCommunityTable::try_from(&ga.spec)?; let external_config = ExternalConfigBuilder::default() - .genid(gen_id) + .genid(input.genid) .device(device) .underlay(underlay) .overlay(overlay) @@ -70,7 +123,8 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { .build() .map_err(|e| { FromK8sConversionError::InternalError(format!( - "Failed to build ExternalConfig for {name}: {e}" + "Failed to translate configuration for gateway {}: {e}", + input.gwname )) })?; Ok(external_config) diff --git a/validator/src/main.rs b/validator/src/main.rs index 8763b1358..05744ebce 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -10,6 +10,7 @@ #![allow(clippy::result_large_err)] #![allow(clippy::field_reassign_with_default)] +use config::converters::k8s::config::gateway_config::validate_metadata; use config::{ExternalConfig, GwConfig}; use k8s_intf::generated::gateway_agent_crd::GatewayAgent; use serde::{Deserialize, Serialize}; @@ -120,53 +121,15 @@ fn deserialize(ga_input: &str) -> Result { Ok(crd) } -/// Validate metadata -fn validate_metadata(crd: &GatewayAgent) -> Result<&str, ValidateError> { - let genid = crd.metadata.generation.ok_or(ValidateError::MetadataError( - "Missing generation Id".to_string(), - ))?; - if genid == 0 { - return Err(ValidateError::MetadataError( - "Invalid generation Id".to_string(), - )); - } - let gwname = crd - .metadata - .name - .as_ref() - .ok_or(ValidateError::MetadataError( - "Missing gateway name".to_string(), - ))?; - if gwname.is_empty() { - return Err(ValidateError::MetadataError( - "Invalid gateway name".to_string(), - )); - } - let namespace = crd - .metadata - .namespace - .as_ref() - .ok_or(ValidateError::MetadataError( - "Missing namespace".to_string(), - ))?; - if namespace.as_str() != "fab" { - return Err(ValidateError::MetadataError(format!( - "Invalid namespace {namespace}" - ))); - } - - Ok(gwname.as_str()) -} - /// Main validation function fn validate(gwagent_json: &str) -> Result<(), ValidateError> { let crd = deserialize(gwagent_json)?; - let gwname = validate_metadata(&crd)?; + let input = validate_metadata(&crd).map_err(|e| ValidateError::MetadataError(e.to_string()))?; let external = ExternalConfig::try_from(&crd) .map_err(|e| ValidateError::ConversionError(e.to_string()))?; - let mut gwconfig = GwConfig::new(gwname, external); + let mut gwconfig = GwConfig::new(input.gwname.as_str(), external); gwconfig.validate().map_err(|e| { let mut config = ConfigErrors::default(); config.errors.push(e.to_string()); From c058c76cbc3e06c2e5105a733f73d687158e04e3 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 22:31:07 +0100 Subject: [PATCH 08/10] feat(config,dependants): move gwname to ExternalConfig Before, we were storing the gateway name in a GwConfig object, which wrapped an ExternalConfig. This patch moves the gateway name to the ExternalConfig object itself, as this is more natural, simplifies the code and will allow for further simplifications. Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/gateway_config.rs | 1 + config/src/external/mod.rs | 11 +++++++---- config/src/gwconfig.rs | 8 +++----- k8s-intf/src/bolero/crd.rs | 3 ++- mgmt/src/processor/confbuild/internal.rs | 2 +- mgmt/src/processor/k8s_client.rs | 2 +- mgmt/src/processor/k8s_less_client.rs | 2 +- mgmt/src/tests/mgmt.rs | 5 +++-- nat/src/stateful/test.rs | 3 ++- nat/src/stateless/test.rs | 3 ++- validator/src/main.rs | 5 +++-- 11 files changed, 26 insertions(+), 19 deletions(-) diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index d45240ec4..ef14c39df 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -114,6 +114,7 @@ impl TryFrom<&GatewayAgent> for ExternalConfig { let comtable = PriorityCommunityTable::try_from(&ga.spec)?; let external_config = ExternalConfigBuilder::default() + .gwname(input.gwname.clone()) .genid(input.genid) .device(device) .underlay(underlay) diff --git a/config/src/external/mod.rs b/config/src/external/mod.rs index d6a38c300..822ec798e 100644 --- a/config/src/external/mod.rs +++ b/config/src/external/mod.rs @@ -23,6 +23,7 @@ pub type GenId = i64; /// The configuration object as seen by the gRPC server #[derive(Builder, Clone, Debug)] pub struct ExternalConfig { + pub gwname: String, /* name of gateway */ pub genid: GenId, /* configuration generation id (version) */ pub device: DeviceConfig, /* goes as-is into the internal config */ pub underlay: Underlay, /* goes as-is into the internal config */ @@ -35,8 +36,9 @@ impl ExternalConfig { #[allow(clippy::new_without_default)] #[must_use] - pub fn new() -> Self { + pub fn new(gwname: &str) -> Self { Self { + gwname: gwname.to_owned(), genid: Self::BLANK_GENID, device: DeviceConfig::new(), underlay: Underlay::default(), @@ -90,7 +92,8 @@ impl ExternalConfig { } } - fn validate_peering_gw_groups(&mut self, gwname: &str) -> ConfigResult { + fn validate_peering_gw_groups(&mut self) -> ConfigResult { + let gwname = &self.gwname; let gwgroups = &self.gwgroups; let comtable = &self.communities; for vpc in self.overlay.vpc_table.values_mut() { @@ -114,11 +117,11 @@ impl ExternalConfig { } Ok(()) } - pub fn validate(&mut self, gwname: &str) -> ConfigResult { + pub fn validate(&mut self) -> ConfigResult { self.device.validate()?; self.underlay.validate()?; self.overlay.validate()?; - self.validate_peering_gw_groups(gwname)?; + self.validate_peering_gw_groups()?; // if there are vpcs configured, there MUST be a vtep configured if !self.overlay.vpc_table.is_empty() && self.underlay.vtep.is_none() { diff --git a/config/src/gwconfig.rs b/config/src/gwconfig.rs index 1557b07c8..efe57d4c2 100644 --- a/config/src/gwconfig.rs +++ b/config/src/gwconfig.rs @@ -58,7 +58,6 @@ impl GwConfigMeta { #[derive(Clone, Debug)] pub struct GwConfig { - pub gwname: String, /* name of gateway */ pub meta: GwConfigMeta, /* config metadata */ pub external: ExternalConfig, /* external config: received */ pub internal: Option, /* internal config: built by gw from internal */ @@ -69,9 +68,8 @@ impl GwConfig { /// Create a [`GwConfig`] object with a given [`ExternalConfig`]. ////////////////////////////////////////////////////////////////// #[must_use] - pub fn new(gwname: &str, external: ExternalConfig) -> Self { + pub fn new(external: ExternalConfig) -> Self { Self { - gwname: gwname.to_owned(), meta: GwConfigMeta::new(external.genid), external, internal: None, @@ -84,7 +82,7 @@ impl GwConfig { ////////////////////////////////////////////////////////////////// #[must_use] pub fn blank() -> Self { - Self::new("", ExternalConfig::new()) + Self::new(ExternalConfig::new("")) } ////////////////////////////////////////////////////////////////// @@ -107,6 +105,6 @@ impl GwConfig { ////////////////////////////////////////////////////////////////// pub fn validate(&mut self) -> ConfigResult { debug!("Validating external config with genid {} ..", self.genid()); - self.external.validate(&self.gwname) + self.external.validate() } } diff --git a/k8s-intf/src/bolero/crd.rs b/k8s-intf/src/bolero/crd.rs index 4d530b1c2..f2712181e 100644 --- a/k8s-intf/src/bolero/crd.rs +++ b/k8s-intf/src/bolero/crd.rs @@ -32,7 +32,8 @@ impl TypeGenerator for LegalValue { Some(LegalValue(GatewayAgent { metadata: ObjectMeta { name: Some(simple_hostname(d)?), - generation: Some(d.gen_i64(Bound::Included(&0), Bound::Unbounded)?), + generation: Some(d.gen_i64(Bound::Excluded(&0), Bound::Unbounded)?), + namespace: Some("fab".to_string()), ..Default::default() }, spec: d.produce::>()?.take(), diff --git a/mgmt/src/processor/confbuild/internal.rs b/mgmt/src/processor/confbuild/internal.rs index 5dfb64664..bdaf96837 100644 --- a/mgmt/src/processor/confbuild/internal.rs +++ b/mgmt/src/processor/confbuild/internal.rs @@ -347,7 +347,7 @@ pub fn build_internal_config( } /* Build internal config: device and underlay configs are copied as received (with adjusted default_vrf) */ - let mut internal = InternalConfig::new(&config.gwname, external.device.clone()); + let mut internal = InternalConfig::new(&external.gwname, external.device.clone()); internal.add_vrf_config(default_vrf)?; internal.set_vtep(external.underlay.vtep.clone()); diff --git a/mgmt/src/processor/k8s_client.rs b/mgmt/src/processor/k8s_client.rs index 2396d1e21..be08ddc8f 100644 --- a/mgmt/src/processor/k8s_client.rs +++ b/mgmt/src/processor/k8s_client.rs @@ -164,7 +164,7 @@ impl K8sClient { return; } - let gwconfig = GwConfig::new(&k8s_client.hostname, external_config); + let gwconfig = GwConfig::new(external_config); // request the config processor to apply the config and update status on success match k8s_client.client.apply_config(gwconfig).await { diff --git a/mgmt/src/processor/k8s_less_client.rs b/mgmt/src/processor/k8s_less_client.rs index baa96cb8a..a28b51aae 100644 --- a/mgmt/src/processor/k8s_less_client.rs +++ b/mgmt/src/processor/k8s_less_client.rs @@ -97,7 +97,7 @@ impl K8sLess { }; info!("Current configuration is {applied_genid}"); - let gwconfig = GwConfig::new(&k8sless.name, external_config); + let gwconfig = GwConfig::new(external_config); // request the config processor to apply the config and update status on success match k8sless.client.apply_config(gwconfig).await { diff --git a/mgmt/src/tests/mgmt.rs b/mgmt/src/tests/mgmt.rs index 844484eaf..dbbab1834 100644 --- a/mgmt/src/tests/mgmt.rs +++ b/mgmt/src/tests/mgmt.rs @@ -359,6 +359,7 @@ pub mod test { /* assemble external config */ ExternalConfigBuilder::default() + .gwname("test-gw".to_string()) .genid(1) .device(device_cfg) .underlay(underlay) @@ -374,7 +375,7 @@ pub mod test { fn check_frr_config() { /* Not really a test but a tool to check generated FRR configs given a gateway config */ let external = sample_external_config(); - let mut config = GwConfig::new("test-gw", external); + let mut config = GwConfig::new(external); config.validate().expect("Config validation failed"); if false { let vpc_table = &config.external.overlay.vpc_table; @@ -399,7 +400,7 @@ pub mod test { println!("External config is:\n{external:#?}"); /* build a gw config from a sample external config */ - let config = GwConfig::new("test-gw", external); + let config = GwConfig::new(external); let dp_status_r: Arc> = Arc::new(RwLock::new(DataplaneStatus::new())); diff --git a/nat/src/stateful/test.rs b/nat/src/stateful/test.rs index 1d2feb7b3..c8e3233d9 100644 --- a/nat/src/stateful/test.rs +++ b/nat/src/stateful/test.rs @@ -78,6 +78,7 @@ mod tests { }; let mut external_builder = ExternalConfigBuilder::default(); + external_builder.gwname("test-gw".to_string()); external_builder.genid(1); external_builder.device(device_config); external_builder.underlay(underlay); @@ -89,7 +90,7 @@ mod tests { .build() .expect("Failed to build external config"); - GwConfig::new("test-gw", external_config) + GwConfig::new(external_config) } #[allow(clippy::too_many_lines)] diff --git a/nat/src/stateless/test.rs b/nat/src/stateless/test.rs index 551b69a4e..d41788b5d 100644 --- a/nat/src/stateless/test.rs +++ b/nat/src/stateless/test.rs @@ -555,6 +555,7 @@ mod tests { }; let mut external_builder = ExternalConfigBuilder::default(); + external_builder.gwname("test-gw".to_string()); external_builder.genid(1); external_builder.device(device_config); external_builder.underlay(underlay); @@ -565,7 +566,7 @@ mod tests { .build() .expect("Failed to build external config"); - GwConfig::new("test-gw", external_config) + GwConfig::new(external_config) } fn check_packet( diff --git a/validator/src/main.rs b/validator/src/main.rs index 05744ebce..9fbc2054f 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -124,12 +124,13 @@ fn deserialize(ga_input: &str) -> Result { /// Main validation function fn validate(gwagent_json: &str) -> Result<(), ValidateError> { let crd = deserialize(gwagent_json)?; - let input = validate_metadata(&crd).map_err(|e| ValidateError::MetadataError(e.to_string()))?; + let _input = + validate_metadata(&crd).map_err(|e| ValidateError::MetadataError(e.to_string()))?; let external = ExternalConfig::try_from(&crd) .map_err(|e| ValidateError::ConversionError(e.to_string()))?; - let mut gwconfig = GwConfig::new(input.gwname.as_str(), external); + let mut gwconfig = GwConfig::new(external); gwconfig.validate().map_err(|e| { let mut config = ConfigErrors::default(); config.errors.push(e.to_string()); From ce9b361615c85ff7ffaf28c71cf6f8609c448b69 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 27 Jan 2026 22:39:06 +0100 Subject: [PATCH 09/10] feat(config,validator): remove call to validate_metadata() Since we store the gateway name and genid in the ExternalConfig, we don't need to call validate_metadata() since that is called already in the TryFrom implementation to decode the spec into an ExternalConfig object. This further ensures that both dataplane and the validator attempt to process a spec using exactly the same logic. Signed-off-by: Fredi Raspall --- config/src/converters/k8s/config/gateway_config.rs | 2 +- validator/src/main.rs | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/config/src/converters/k8s/config/gateway_config.rs b/config/src/converters/k8s/config/gateway_config.rs index ef14c39df..274461aa0 100644 --- a/config/src/converters/k8s/config/gateway_config.rs +++ b/config/src/converters/k8s/config/gateway_config.rs @@ -21,7 +21,7 @@ pub struct K8sInput { /// Validate the metadata of a `GatewayAgent`. /// # Errors /// Returns `FromK8sConversionError` in case data is missing or is invalid -pub fn validate_metadata(ga: &GatewayAgent) -> Result { +fn validate_metadata(ga: &GatewayAgent) -> Result { let genid = ga .metadata .generation diff --git a/validator/src/main.rs b/validator/src/main.rs index 9fbc2054f..84d7122ea 100644 --- a/validator/src/main.rs +++ b/validator/src/main.rs @@ -10,8 +10,7 @@ #![allow(clippy::result_large_err)] #![allow(clippy::field_reassign_with_default)] -use config::converters::k8s::config::gateway_config::validate_metadata; -use config::{ExternalConfig, GwConfig}; +use config::{ExternalConfig, GwConfig, converters::k8s::FromK8sConversionError}; use k8s_intf::generated::gateway_agent_crd::GatewayAgent; use serde::{Deserialize, Serialize}; use std::io::{self, Read}; @@ -124,11 +123,10 @@ fn deserialize(ga_input: &str) -> Result { /// Main validation function fn validate(gwagent_json: &str) -> Result<(), ValidateError> { let crd = deserialize(gwagent_json)?; - let _input = - validate_metadata(&crd).map_err(|e| ValidateError::MetadataError(e.to_string()))?; - - let external = ExternalConfig::try_from(&crd) - .map_err(|e| ValidateError::ConversionError(e.to_string()))?; + let external = ExternalConfig::try_from(&crd).map_err(|e| match e { + FromK8sConversionError::K8sInfra(e) => ValidateError::MetadataError(e.to_string()), + _ => ValidateError::ConversionError(e.to_string()), + })?; let mut gwconfig = GwConfig::new(external); gwconfig.validate().map_err(|e| { From 5c4ea1f6b2a30d605405177d86c30b7e2c141937 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 28 Jan 2026 11:36:11 +0100 Subject: [PATCH 10/10] feat(config): simplify impl of Ord for GwGroupMember Simplify the implementation of Ord for GwGroupMember, given that no two group members can have the same name. Signed-off-by: Fredi Raspall --- config/src/external/gwgroup.rs | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/config/src/external/gwgroup.rs b/config/src/external/gwgroup.rs index de8584423..7dbacb7e5 100644 --- a/config/src/external/gwgroup.rs +++ b/config/src/external/gwgroup.rs @@ -33,14 +33,10 @@ impl Ord for GwGroupMember { fn cmp(&self, other: &Self) -> std::cmp::Ordering { let ord_prio = self.priority.cmp(&other.priority); let ord_ip = self.ipaddress.cmp(&other.ipaddress); - let ord_name = self.name.cmp(&other.name); if ord_prio == Ordering::Equal { - if ord_ip == Ordering::Equal { - ord_name - } else { - ord_ip - } + assert_ne!(ord_ip, Ordering::Equal); + ord_ip } else { ord_prio } @@ -241,6 +237,29 @@ mod test { println!("{gwtable}"); } + + #[test] + fn test_gw_group_ordering_fallback_ip() { + let mut group = GwGroup::new("gw-group-1"); + group.add_member(GwGroupMember::new("gw1", 100, IpAddr::from_str("172.128.0.1").unwrap())).unwrap(); + group.add_member(GwGroupMember::new("gw2", 100, IpAddr::from_str("172.128.0.2").unwrap())).unwrap(); + group.add_member(GwGroupMember::new("gw3", 100, IpAddr::from_str("172.128.0.3").unwrap())).unwrap(); + group.sort_members(); + + let mut prio = group.members[0].priority; + let mut ipaddress = group.members[0].ipaddress; + for m in group.iter() { + assert!(m.priority <= prio); + assert!(m.ipaddress <= ipaddress); + prio = m.priority; + ipaddress = m.ipaddress; + } + + let mut gwtable = GwGroupTable::new(); + gwtable.add_group(group).unwrap(); + println!("{gwtable}"); + } + fn build_sample_gw_groups() -> GwGroupTable { let mut gwtable = GwGroupTable::new();