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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Jul 11, 2025 at 11:40:56AM +0200, Gabriel Goller wrote:
> > > [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.

1) Hygiene.

You don't want to have multiple different generic error type helper
crates in your dependency tree just for "messages".

2) Having an overview of what can go wrong.

3) Error *handling*:

Most of the time it is enough to know "classes" of errors in the code.

- Error class: the user passed invalid data:
  In this case, certain validation functions could also be made
  available in a wasm crate, which the UI could use to display useful
  errors to the user in an input form.
  But the caller will almost never need to execute specific code based
  on which particular error happened, which means this can be a single
  variant with a message.
  (Unless you want to implement suggestions to the user on the UI
  side... but then such cases can still be specialized later. Eg. the
  error type can have an optional "source" error which could then be
  zero-zised "tag" types the UI could check via
  `.source().downcast()`...)

- The config is invalid:
  In addition to the invalid input from the user, someone may have
  manually modified the on-disk configuration files, so this case should
  be distinguishable somehow.
  Callers of the code could perhaps try an older backed-up version of
  the config.

- An internal error happened:
  Here callers usually cannot recover and that's the "catch-all" for
  pretty much everything beyond the caller's control...

- Bubbling up specific errors from deeper in the stack:
  For instance, code which uses the `proxmox-client` crate should be
  able to tell if the underlying HTTP connection responded with an
  *error* as opposed to there being problems with the *network* stack:
  Eg. in PDM we connect to a PVE cluster node to call an API method. If
  the API method returns an *error*, then we pass it on to the caller.
  But if a *network* error happens, eg. the `connect()` call fails, PDM
  will try another cluster node. The proxmox-client crate *itself*,
  however, only cares about dealing with a single target node.)

> 
> > 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