Pr/fredi/cleanup and validation

config/src/converters/k8s/config/bgp.rs

@@ -16,7 +16,7 @@ impl TryFrom<&GatewayAgentGatewayNeighbors> for BgpNeighbor {
     fn try_from(neighbor: &GatewayAgentGatewayNeighbors) -> Result<Self, Self::Error> {
         let neighbor_addr = match neighbor.ip.as_ref() {
             Some(ip) => ip.parse::<IpAddr>().map_err(|e| {
-                FromK8sConversionError::ParseError(format!("Invalid neighbor address {ip}: {e}"))
+                FromK8sConversionError::InvalidData(format!("neighbor address {ip}: {e}"))
             })?,
             None => {
                 return Err(FromK8sConversionError::MissingData(format!(

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)?;
                 }

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<Self, Self::Error> {
+    fn try_from(gagw: &GatewayAgentGateway) -> Result<Self, Self::Error> {
         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,19 +24,22 @@ mod test {
     use super::*;
 
     use k8s_intf::bolero::LegalValue;
+    use k8s_intf::gateway_agent_crd::GatewayAgent;
 
     #[test]
     fn test_simple_hostname() {
         bolero::check!()
             .with_type::<LegalValue<GatewayAgent>>()
             .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()
+                    );
+                }
             });
     }
 }

config/src/converters/k8s/config/expose.rs

@@ -18,10 +18,9 @@ fn parse_port_ranges(ports_str: &str) -> Result<Vec<PortRange>, FromK8sConversio
         // Split port ranges for prefix on ','
         .split(',')
         .map(|port_range_str| {
-            port_range_str
-                .trim()
-                .parse::<PortRange>()
-                .map_err(|e| FromK8sConversionError::ParseError(format!("Invalid port range: {e}")))
+            port_range_str.trim().parse::<PortRange>().map_err(|e| {
+                FromK8sConversionError::InvalidData(format!("port range {port_range_str}: {e}"))
+            })
         })
         .collect()
 }
@@ -56,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}"
                 ))
             })?;
@@ -88,15 +87,15 @@ fn process_ip_block(
         }
         (Some(cidr), None, None) => {
             let prefix = cidr.parse::<Prefix>().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);
             }
         }
         (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);
@@ -117,21 +116,21 @@ 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::<Prefix>().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);
             }
         }
         (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);
@@ -147,19 +146,19 @@ fn process_nat_block(
 ) -> Result<VpcExpose, FromK8sConversionError> {
     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(),
             )),
             (Some(stateful), None) => {
                 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),
@@ -177,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(),
                 ));
             }
@@ -299,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]

config/src/converters/k8s/config/gateway_config.rs

@@ -1,43 +1,99 @@
 // 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
+fn validate_metadata(ga: &GatewayAgent) -> Result<K8sInput, FromK8sConversionError> {
+    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<Self, Self::Error> {
-        let name = ga
-            .metadata
-            .name
-            .as_ref()
-            .ok_or(FromK8sConversionError::MissingData(
-                "metadata.name not found".to_string(),
-            ))?
-            .as_str();
-
-        let Some(gen_id) = ga.metadata.generation else {
-            return Err(FromK8sConversionError::Invalid(format!(
-                "metadata.generation not found for {name}"
-            )));
-        };
+        let input = validate_metadata(ga)?;
 
-        let device = DeviceConfig::try_from(ga)?;
+        let ga_spec_gw = input
+            .spec
+            .gateway
+            .as_ref()
+            .unwrap_or_else(|| unreachable!());
 
-        let mut underlay = Underlay::try_from(ga.spec.gateway.as_ref().ok_or(
-            FromK8sConversionError::MissingData(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
@@ -58,7 +114,8 @@ impl TryFrom<&GatewayAgent> for ExternalConfig {
         let comtable = PriorityCommunityTable::try_from(&ga.spec)?;
 
         let external_config = ExternalConfigBuilder::default()
-            .genid(gen_id)
+            .gwname(input.gwname.clone())
+            .genid(input.genid)
             .device(device)
             .underlay(underlay)
             .overlay(overlay)
@@ -67,7 +124,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)

config/src/converters/k8s/config/gwgroups.rs

@@ -13,7 +13,7 @@ impl TryFrom<&GatewayAgentGroupsMembers> for GwGroupMember {
     fn try_from(value: &GatewayAgentGroupsMembers) -> Result<Self, Self::Error> {
         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(),
@@ -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)?;
                 }