Re: [pve-devel] [PATCH proxmox-ve-rs v4 18/22] common: sdn: fabrics: implement validation

[Gabriel Goller <g.goller@proxmox.com>]

>> [snip]
>> diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs b/proxmox-ve-config/src/sdn/fabric/mod.rs
>> index 3342a7053d3f..a2132e8aff3f 100644
>> --- a/proxmox-ve-config/src/sdn/fabric/mod.rs
>> +++ b/proxmox-ve-config/src/sdn/fabric/mod.rs
>> @@ -1,11 +1,13 @@
>>  pub mod section_config;
>>
>> -use std::collections::BTreeMap;
>> +use std::collections::{BTreeMap, HashSet};
>>  use std::marker::PhantomData;
>>  use std::ops::Deref;
>>
>>  use serde::{Deserialize, Serialize};
>>
>> +use crate::common::valid::Validatable;
>> +
>>  use crate::sdn::fabric::section_config::{
>>      fabric::{
>>          Fabric, FabricDeletableProperties, FabricId, FabricSection, FabricSectionUpdater,
>> @@ -34,15 +36,38 @@ pub enum FabricConfigError {
>>      FabricDoesNotExist(String),
>>      #[error("node '{0}' does not exist in fabric '{1}'")]
>>      NodeDoesNotExist(String, String),
>> +    #[error("node IP {0} is outside the IP prefix {1} of the fabric")]
>> +    NodeIpOutsideFabricRange(String, String),
>>      #[error("node has a different protocol than the referenced fabric")]
>>      ProtocolMismatch,
>>      #[error("fabric '{0}' already exists in config")]
>>      DuplicateFabric(String),
>>      #[error("node '{0}' already exists in config for fabric {1}")]
>>      DuplicateNode(String, String),
>> +    #[error("fabric {0} contains nodes with duplicated IPs")]
>> +    DuplicateNodeIp(String),
>> +    #[error("fabric '{0}' does not have an IP prefix configured for the node IP {1}")]
>> +    FabricNoIpPrefixForNode(String, String),
>> +    #[error("node '{0}' does not have an IP configured for this fabric prefix {1}")]
>> +    NodeNoIpForFabricPrefix(String, String),
>> +    #[error("fabric '{0}' does not have an IP prefix configured")]
>> +    FabricNoIpPrefix(String),
>> +    #[error("node '{0}' does not have an IP configured")]
>> +    NodeNoIp(String),
>> +    #[error("interface is already in use by another fabric")]
>> +    DuplicateInterface,
>> +    #[error("IPv6 is currently not supported for protocol {0}")]
>> +    NoIpv6(String),
>
>^ "NoIpv6" is a weird variant name for this. Maybe use Ipv6Unsupported.

Agree, changed it.

>I also wonder if we really need all those messages as separate variants.
>As a followup to the previous thoughts on error types: Generally I think
>only errors which are worth distinguishing need their own variants. We
>definitely don't need one for every single message.

IMO what we could do is split it up in multiple enums and have a sort of
tree structure at the end.

>The ones here could probably all just be a single `Invalid(String)`.
>(Or if we don't want to expose `String` here,
>`Validation(ValidationError)`, with the inner one being a newtype
>wrapper around the `String`.

Hmm but then why create custom error types at all? This is the same as
having strings where we can't distinguish between errors.

>Internally there can be a helper
>`fn FabricConfigError::validation(impl Into<String>)`.
>>      // should usually not occur, but we still check for it nonetheless
>>      #[error("mismatched fabric_id")]
>>      FabricIdMismatch,
>> +    // this is technically possible, but we don't allow it
>> +    #[error("duplicate OSPF area")]
>> +    DuplicateOspfArea,
>> +    #[error("IP prefix {0} in fabric '{1}' overlaps with IPv4 prefix {2} in fabric '{3}'")]
>> +    OverlappingIp4Prefix(String, String, String, String),
>> +    #[error("IPv6 prefix {0} in fabric '{1}' overlaps with IPv6 prefix {2} in fabric '{3}'")]
>> +    OverlappingIp6Prefix(String, String, String, String),
>>  }
>>
>>  /// An entry in a [`FabricConfig`].
>> @@ -367,6 +392,93 @@ impl From<Fabric> for FabricEntry {
>>      }
>>  }
>> [snip]

Thanks!



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel