Re: [PATCH manager/network/proxmox-ve-rs 0/9] Implement route redistribution for OSPF

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

LGTM!
Just a few things:

Setting a route-map on a redistribution didn't work, the api property was
missing:
(pve-network)

diff --git a/src/PVE/Network/SDN/Fabrics.pm b/src/PVE/Network/SDN/Fabrics.pm
index d5ab808c3a..f7140187ad 100644
--- a/src/PVE/Network/SDN/Fabrics.pm
+++ b/src/PVE/Network/SDN/Fabrics.pm
@@ -313,6 +313,12 @@
                                     'static',
                                 ],
                             },
+                            route_map => {
+                                type => 'string',
+                                description =>
+                                    'The route-map to filter the redistribution.',
+                                optional => 1,
+                            },
                         },
                     },
                     optional => 1,


The route_map property also has a wrong serde rename_all type (proxmox-ve-rs):

diff --git a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs
index 2e1f37b278..66297993fe 100644
--- a/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs
+++ b/proxmox-ve-config/src/sdn/fabric/section_config/protocol/ospf.rs
@@ -36,7 +36,7 @@

 #[api]
 #[derive(Debug, Clone, Serialize, Deserialize, Hash)]
-#[serde(rename_all = "kebab-case")]
+#[serde(rename_all = "lowercase")]
 /// An OSPF redistribution
 pub struct OspfRedistribution {
     /// The source protocol for this redistribution


There is also an additional whitespace when specifying a route-map:

diff --git a/proxmox-frr-templates/templates/ospfd.jinja b/proxmox-frr-templates/templates/ospfd.jinja
index 4ba609f1e5..427ed7f9dd 100644
--- a/proxmox-frr-templates/templates/ospfd.jinja
+++ b/proxmox-frr-templates/templates/ospfd.jinja
@@ -4,7 +4,7 @@
 router ospf
  ospf router-id {{ ospf.router.router_id }}
 {% for redistribution in ospf.router.redistribute %}
- redistribute {{ redistribution.source }}{% if redistribution.metric is defined %} metric {{ redistribution.metric }}{% endif %} {% if redistribution.route_map is defined %} route-map {{ redistribution.route_map }}{% endif %}
+ redistribute {{ redistribution.source }}{% if redistribution.metric is defined %} metric {{ redistribution.metric }}{% endif %}{% if redistribution.route_map is defined %} route-map {{ redistribution.route_map }}{% endif %}

 {% endfor %}
 exit


And in pve-manager, IMO it looks better when the "Add" button is at the bottom of the table:

diff --git a/www/manager6/sdn/fabrics/RedistributionGrid.js b/www/manager6/sdn/fabrics/RedistributionGrid.js
index 8580c47939..cb7c374d45 100644
--- a/www/manager6/sdn/fabrics/RedistributionGrid.js
+++ b/www/manager6/sdn/fabrics/RedistributionGrid.js
@@ -20,7 +20,7 @@
         }
     },

-    tbar: [
+    bbar: [
         '->',
         {
             text: gettext('Add'),


Another small thing: I'm not sure if "static" is a bit confusing for the users,
because it refers to routes from the "staticd" daemon in frr and not static
routes on the host. But I can't think of a better name to be honest ...

The rest works great!
Tested redistributing every type of routes and tested filtering them by route-maps.