[PATCH proxmox-ve-rs v6 01/24] sdn: prefix lists: refactor section config and api format

[Stefan Hanreich <s.hanreich@proxmox.com>]

Sequence numbers are now required in the section config and the
backend auto-generates them when they're not contained in the user
submitted values. If an entry is missing a sequence number, then it is
assigned the highest sequence number that already exists in the
configuration plus 5 (or 5, if there is none). For this reason, split
the section config types from the API types, since they now have a
different structure. The API types are used for consuming data sent
from the API and then converted to the section config format.

The methods of the section config type now enforce those invariants by
exposing CRUD methods for manipulating its entries. They auto-generate
sequence numbers if they are missing.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 proxmox-ve-config/src/sdn/prefix_list.rs     | 305 ++++++++++++++++++-
 proxmox-ve-config/tests/prefix_lists/main.rs |  30 +-
 2 files changed, 304 insertions(+), 31 deletions(-)

diff --git a/proxmox-ve-config/src/sdn/prefix_list.rs b/proxmox-ve-config/src/sdn/prefix_list.rs
index d250a8e..0efe81d 100644
--- a/proxmox-ve-config/src/sdn/prefix_list.rs
+++ b/proxmox-ve-config/src/sdn/prefix_list.rs
@@ -20,12 +20,14 @@
 //!   entries action=deny,prefix=192.0.2.0/24,le=24
 //! ```
 
+use std::ops::{Deref, DerefMut};
+
 use const_format::concatcp;
 use serde::{Deserialize, Serialize};
 
 use proxmox_network_types::Cidr;
 use proxmox_schema::{
-    api, api_string_type, const_regex, property_string::PropertyString, ApiStringFormat, Updater,
+    api, api_string_type, const_regex, property_string::PropertyString, ApiStringFormat,
     UpdaterType,
 };
 
@@ -69,17 +71,15 @@ pub enum PrefixListAction {
         },
     }
 )]
-#[derive(Debug, Clone, Serialize, Deserialize, Updater)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
 /// IP Prefix List
 ///
 /// Corresponds to the FRR IP Prefix lists, as described in its [documentation](https://docs.frrouting.org/en/latest/filter.html#ip-prefix-list)
 pub struct PrefixListSection {
-    #[updater(skip)]
-    id: PrefixListId,
+    pub(crate) id: PrefixListId,
     /// The entries in this prefix list
     #[serde(default, skip_serializing_if = "Vec::is_empty")]
-    #[updater(serde(skip_serializing_if = "Option::is_none"))]
-    pub entries: Vec<PropertyString<PrefixListEntry>>,
+    pub(crate) entries: Vec<PropertyString<PrefixListEntry>>,
 }
 
 impl PrefixListSection {
@@ -87,6 +87,191 @@ impl PrefixListSection {
     pub fn id(&self) -> &PrefixListId {
         &self.id
     }
+
+    /// Try to update this [`PrefixListSection`].
+    ///
+    /// This method fails if the given entry list is not valid.
+    pub fn try_update(
+        &mut self,
+        updater: api::PrefixListUpdater,
+        delete: Option<Vec<api::PrefixListDeletableProperties>>,
+    ) -> Result<(), anyhow::Error> {
+        let api::PrefixListUpdater { entries } = updater;
+
+        if let Some(entries) = entries {
+            self.try_set_api_entries(entries.into_iter().map(PropertyString::into_inner))?;
+        }
+
+        for deletable_property in delete.unwrap_or_default() {
+            match deletable_property {
+                api::PrefixListDeletableProperties::Entries => {
+                    self.entries = Vec::new();
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    /// Returns the value for the next sequence number that should be inserted.
+    ///
+    /// This mirrors the logic in FRR by returning the highest existing sequence number + 5.
+    pub fn next_seq_number(&self) -> u32 {
+        self.entries
+            .iter()
+            .max_by_key(|entry| entry.seq)
+            .map(|entry| entry.seq + 5)
+            .unwrap_or(5)
+    }
+
+    /// Returns the entry with sequence number `seq`.
+    pub fn entries(&self) -> impl IntoIterator<Item = &PrefixListEntry> {
+        self.entries.iter().map(Deref::deref)
+    }
+
+    /// Returns the entry with sequence number `seq`.
+    pub fn entry(&self, seq: u32) -> Option<&PrefixListEntry> {
+        self.entries
+            .iter()
+            .find(|entry| entry.seq == seq)
+            .map(Deref::deref)
+    }
+
+    /// Returns a mutable reference to the entry with sequence number `seq`.
+    pub fn entry_mut(&mut self, seq: u32) -> Option<&mut PrefixListEntry> {
+        self.entries
+            .iter_mut()
+            .find(|entry| entry.seq == seq)
+            .map(DerefMut::deref_mut)
+    }
+
+    /// Returns the position of the entry with sequence number seq.
+    pub fn entry_position(&self, seq: u32) -> Option<usize> {
+        self.entries.iter().position(|entry| entry.seq == seq)
+    }
+
+    /// Sets the entries for this prefix list.
+    pub fn try_set_api_entries(
+        &mut self,
+        entries: impl IntoIterator<Item = api::PrefixListEntry>,
+    ) -> Result<(), anyhow::Error> {
+        let old_entries = std::mem::take(&mut self.entries);
+
+        for entry in entries {
+            if let Err(error) = self.try_insert_api_entry(entry) {
+                self.entries = old_entries;
+                return Err(error);
+            }
+        }
+
+        Ok(())
+    }
+
+    /// Try to insert a [`api::PrefixListEntry`].
+    ///
+    /// This method fails if the given entry has a sequence number, that already exists in the
+    /// configuration. If no sequence number is set in the entry, then a new sequence number will be
+    /// auto-generated via [`Self::next_seq_number`].
+    pub fn try_insert_api_entry(
+        &mut self,
+        entry: api::PrefixListEntry,
+    ) -> Result<(), anyhow::Error> {
+        if let Some(seq) = entry.seq {
+            if self.entry_position(seq).is_some() {
+                anyhow::bail!("entry with sequence number {seq} already exists!");
+            }
+        }
+
+        let entry = PrefixListEntry {
+            action: entry.action,
+            prefix: entry.prefix,
+            le: entry.le,
+            ge: entry.ge,
+            seq: entry.seq.unwrap_or_else(|| self.next_seq_number()),
+        };
+
+        self.try_insert_entry(entry)
+    }
+
+    /// Try to insert an entry.
+    ///
+    /// This method fails if the sequence number from the entry already exists in the
+    /// configuration.
+    pub fn try_insert_entry(&mut self, entry: PrefixListEntry) -> Result<(), anyhow::Error> {
+        if self.entry(entry.seq).is_some() {
+            anyhow::bail!("entry with sequence number {} already exists", entry.seq);
+        }
+
+        self.entries.push(entry.into());
+        Ok(())
+    }
+
+    /// Removes the entry with the given sequence number and returns it.
+    pub fn remove_entry(&mut self, seq: u32) -> Option<PrefixListEntry> {
+        self.entry_position(seq)
+            .map(|index| self.entries.remove(index).into_inner())
+    }
+
+    /// Try to update an entry in [`PrefixListSection`].
+    ///
+    /// This method fails if the new entry has a sequence number that already exists in the
+    /// [`PrefixListSection`].
+    pub fn try_update_entry(
+        &mut self,
+        old_seq: u32,
+        updater: api::PrefixListEntryUpdater,
+        delete: Vec<api::PrefixListEntryDeletableProperties>,
+    ) -> Result<(), anyhow::Error> {
+        let api::PrefixListEntryUpdater {
+            action,
+            prefix,
+            le,
+            ge,
+            seq,
+        } = updater;
+
+        if let Some(seq) = updater.seq {
+            if seq != old_seq && self.entry(seq).is_some() {
+                anyhow::bail!("entry with sequence number {seq} already exists!");
+            }
+        }
+
+        let mut existing_entry = self.remove_entry(old_seq).ok_or_else(|| {
+            anyhow::anyhow!("entry with sequence number {old_seq} does not exist!")
+        })?;
+
+        if let Some(seq) = seq {
+            existing_entry.seq = seq;
+        }
+
+        if let Some(action) = action {
+            existing_entry.action = action;
+        }
+
+        if let Some(prefix) = prefix {
+            existing_entry.prefix = prefix;
+        }
+
+        if let Some(le) = le {
+            existing_entry.le = Some(le);
+        }
+
+        if let Some(ge) = ge {
+            existing_entry.ge = Some(ge);
+        }
+
+        for property in delete {
+            match property {
+                api::PrefixListEntryDeletableProperties::Le => existing_entry.le = None,
+                api::PrefixListEntryDeletableProperties::Ge => existing_entry.ge = None,
+                api::PrefixListEntryDeletableProperties::Seq => {
+                    existing_entry.seq = self.next_seq_number()
+                }
+            }
+        }
+
+        self.try_insert_entry(existing_entry)
+    }
 }
 
 #[api()]
@@ -106,8 +291,13 @@ pub struct PrefixListEntry {
     #[serde(skip_serializing_if = "Option::is_none")]
     ge: Option<u32>,
     /// The sequence number for this prefix list entry.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    seq: Option<u32>,
+    seq: u32,
+}
+
+impl PrefixListEntry {
+    pub fn seq(&self) -> u32 {
+        self.seq
+    }
 }
 
 #[api(
@@ -121,7 +311,9 @@ pub struct PrefixListEntry {
 )]
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case", tag = "type")]
+/// Prefix List section config file.
 pub enum PrefixList {
+    /// A prefix list.
     PrefixList(PrefixListSection),
 }
 
@@ -150,7 +342,7 @@ pub mod frr {
                     PrefixListAction::Deny => route_map::AccessAction::Deny,
                 },
                 network: value.prefix,
-                seq: value.seq,
+                seq: Some(value.seq),
                 le: value.le,
                 ge: value.ge,
                 is_ipv6: value.prefix.is_ipv6(),
@@ -186,10 +378,62 @@ pub mod frr {
 }
 
 pub mod api {
-    use super::*;
+    use serde::{Deserialize, Serialize};
+
+    use proxmox_network_types::Cidr;
+    use proxmox_schema::{api, property_string::PropertyString, ApiStringFormat, Updater};
+
+    use super::{PrefixListAction, PrefixListId, PrefixListSection};
+
+    #[api(
+        properties: {
+            entries: {
+                type: Array,
+                optional: true,
+                items: {
+                    type: String,
+                    description: "An entry in a prefix list",
+                    format: &ApiStringFormat::PropertyString(&PrefixListEntry::API_SCHEMA),
+                }
+            },
+        }
+    )]
+    #[derive(Debug, Clone, Serialize, Deserialize, Updater)]
+    /// IP Prefix List API type.
+    ///
+    /// In the API, specifying the sequence number for entries is optional, so model that constraint here in
+    /// the API type by using the respective entry API type.
+    pub struct PrefixList {
+        #[updater(skip)]
+        pub(crate) id: PrefixListId,
+        /// The entries in this prefix list
+        #[serde(default, skip_serializing_if = "Vec::is_empty")]
+        #[updater(serde(skip_serializing_if = "Option::is_none"))]
+        pub(crate) entries: Vec<PropertyString<PrefixListEntry>>,
+    }
+
+    impl PrefixList {
+        pub fn id(&self) -> &PrefixListId {
+            &self.id
+        }
+    }
+
+    impl TryFrom<PrefixList> for PrefixListSection {
+        type Error = anyhow::Error;
 
-    pub type PrefixList = PrefixListSection;
-    pub type PrefixListUpdater = PrefixListSectionUpdater;
+        fn try_from(value: PrefixList) -> Result<Self, Self::Error> {
+            let mut section = Self {
+                id: value.id,
+                entries: Vec::new(),
+            };
+
+            for entry in value.entries {
+                section.try_insert_api_entry(entry.into_inner())?;
+            }
+
+            Ok(section)
+        }
+    }
 
     #[derive(Debug, Clone, Serialize, Deserialize)]
     #[serde(rename_all = "kebab-case")]
@@ -197,6 +441,35 @@ pub mod api {
     pub enum PrefixListDeletableProperties {
         Entries,
     }
+
+    #[api()]
+    #[derive(Debug, Clone, Serialize, Deserialize, Updater)]
+    /// IP Prefix List Entry API type.
+    ///
+    /// In the API, specifying the sequence number is optional, so model that constraint here in
+    /// the API type.
+    pub struct PrefixListEntry {
+        pub(crate) action: PrefixListAction,
+        pub(crate) prefix: Cidr,
+        /// Prefix length - entry will be applied if the prefix length is less than or equal to this
+        /// value.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        pub(crate) le: Option<u32>,
+        /// Prefix length - entry will be applied if the prefix length is greater than or equal to this
+        /// value.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        pub(crate) ge: Option<u32>,
+        /// The sequence number for this prefix list entry.
+        pub(crate) seq: Option<u32>,
+    }
+
+    #[derive(Debug, Clone, Serialize, Deserialize)]
+    #[serde(rename_all = "kebab-case")]
+    pub enum PrefixListEntryDeletableProperties {
+        Le,
+        Ge,
+        Seq,
+    }
 }
 
 #[cfg(test)]
@@ -209,11 +482,11 @@ mod tests {
     fn test_simple_prefix_list() -> Result<(), anyhow::Error> {
         let section_config = r#"
 prefix-list: somelist
-  entries action=permit,prefix=192.0.2.0/24
-  entries action=permit,prefix=192.0.2.0/24,le=32
+  entries action=permit,prefix=192.0.2.0/24,seq=22
+  entries action=permit,prefix=192.0.2.0/24,le=32,seq=122
   entries action=permit,prefix=192.0.2.0/24,le=32,ge=24,seq=123
-  entries action=permit,prefix=192.0.2.0/24,ge=24
-  entries action=permit,prefix=192.0.2.0/24,ge=24,le=31
+  entries action=permit,prefix=192.0.2.0/24,ge=24,seq=232
+  entries action=permit,prefix=192.0.2.0/24,ge=24,le=31,seq=222
 "#;
 
         PrefixList::parse_section_config("prefix-lists.cfg", section_config)?;
diff --git a/proxmox-ve-config/tests/prefix_lists/main.rs b/proxmox-ve-config/tests/prefix_lists/main.rs
index 2ed4894..3b12084 100644
--- a/proxmox-ve-config/tests/prefix_lists/main.rs
+++ b/proxmox-ve-config/tests/prefix_lists/main.rs
@@ -13,11 +13,11 @@ use proxmox_section_config::typed::ApiSectionDataEntry;
 fn test_build_prefix_list() -> Result<(), anyhow::Error> {
     let section_config = r#"
 prefix-list: example-1
-  entries action=permit,prefix=192.0.2.0/24
-  entries action=permit,prefix=192.0.2.0/24,le=32
+  entries action=permit,prefix=192.0.2.0/24,seq=1
+  entries action=permit,prefix=192.0.2.0/24,le=32,seq=4
   entries action=permit,prefix=192.0.2.0/24,le=32,ge=24,seq=123
-  entries action=permit,prefix=192.0.2.0/24,ge=24
-  entries action=permit,prefix=192.0.2.0/24,ge=24,le=31
+  entries action=permit,prefix=192.0.2.0/24,ge=24,seq=3
+  entries action=permit,prefix=192.0.2.0/24,ge=24,le=31,seq=2
 
 prefix-list: example-3
   entries action=permit,prefix=192.0.2.0/24,seq=333
@@ -25,8 +25,8 @@ prefix-list: example-3
   entries action=permit,prefix=203.0.113.0/24,seq=111
 
 prefix-list: example-2
-  entries action=deny,prefix=192.0.2.0/24,le=25
-  entries action=permit,prefix=192.0.2.0/24
+  entries action=deny,prefix=192.0.2.0/24,le=25,seq=111
+  entries action=permit,prefix=192.0.2.0/24,seq=121
 "#;
 
     let config = PrefixList::parse_section_config("prefix-lists.cfg", section_config)?;
@@ -42,14 +42,14 @@ prefix-list: example-2
     assert_eq!(
         dump(&frr_config)?,
         r#"!
-ip prefix-list example-1 permit 192.0.2.0/24
-ip prefix-list example-1 permit 192.0.2.0/24 le 32
+ip prefix-list example-1 seq 1 permit 192.0.2.0/24
+ip prefix-list example-1 seq 4 permit 192.0.2.0/24 le 32
 ip prefix-list example-1 seq 123 permit 192.0.2.0/24 le 32 ge 24
-ip prefix-list example-1 permit 192.0.2.0/24 ge 24
-ip prefix-list example-1 permit 192.0.2.0/24 le 31 ge 24
+ip prefix-list example-1 seq 3 permit 192.0.2.0/24 ge 24
+ip prefix-list example-1 seq 2 permit 192.0.2.0/24 le 31 ge 24
 !
-ip prefix-list example-2 deny 192.0.2.0/24 le 25
-ip prefix-list example-2 permit 192.0.2.0/24
+ip prefix-list example-2 seq 111 deny 192.0.2.0/24 le 25
+ip prefix-list example-2 seq 121 permit 192.0.2.0/24
 !
 ip prefix-list example-3 seq 333 permit 192.0.2.0/24
 ip prefix-list example-3 seq 222 permit 198.51.100.0/24
@@ -64,7 +64,7 @@ ip prefix-list example-3 seq 111 permit 203.0.113.0/24
 fn test_build_prefix_list_overwrite() -> Result<(), anyhow::Error> {
     let section_config = r#"
 prefix-list: example-1
-  entries action=permit,prefix=192.0.2.0/24
+  entries action=permit,prefix=192.0.2.0/24,seq=234
 "#;
 
     let config = PrefixList::parse_section_config("prefix-lists.cfg", section_config)?;
@@ -72,7 +72,7 @@ prefix-list: example-1
     let example_1_prefix_list = vec![FrrPrefixListRule {
         action: AccessAction::Deny,
         network: Cidr::new_v4([198, 51, 100, 0], 24).unwrap(),
-        seq: None,
+        seq: Some(234),
         le: None,
         ge: None,
         is_ipv6: false,
@@ -104,7 +104,7 @@ prefix-list: example-1
     assert_eq!(
         generated_frr_config,
         r#"!
-ip prefix-list example-1 permit 192.0.2.0/24
+ip prefix-list example-1 seq 234 permit 192.0.2.0/24
 "#
     );
 
-- 
2.47.3