Re: [PATCH proxmox-ve-rs v7 08/21] frr: add bgp support with templates and serialization

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

On 27.03.2026 01:50, Thomas Lamprecht wrote:
> Am 23.03.26 um 14:50 schrieb Gabriel Goller:
> > +++ b/proxmox-frr-templates/templates/bgpd.jinja
> > +{% for ip_route in vrf.ip_routes %}
> > +{% if ip_route.vrf %}
> > + {{ "ipv6" if ip_route.is_ipv6 else "ip" }} route ...
> > +{% else %}
> > + {{ "ipv6" if ip_route.is_ipv6 else "ip" }} route ...
> > +{% endfor %}
> > +{% endif %}
> 
> the endfor and endif statements are swapped here - the for-loop is closed
> while the if-block is still open. This is a Jinja nesting error that should
> fail at template parse time, at least that's what I'd expect.
> 
> Ah, it's fixed in 09/21 but should be squashed into this patch.

Ah, I blame `jj absorb` :)

> > +++ b/proxmox-frr/src/ser/bgp.rs
> > +pub struct NeighborGroup {
> > +    pub ebgp_multihop: Option<i32>,
> 
> eBGP multihop can't be negative (FRR accepts 1-255), so probably should be
> an Option<u8>?

Agree, fixed.

> > +pub struct L2vpnEvpnAF {
> > +    pub autort_as: Option<i32>,
> 
> Similar here, ASNs are unsigned, so Option<u32>.

Fixed as well.

> > +pub struct NeighborGroup {
> > +    pub local_as: Option<u32>,
> 
> This field exists but bgp_router.jinja never renders `neighbor X local-as
> Y`. If intentionally unsupported for now, either just drop it or add some
> (TODO) comment providing rationale.

Yep, this will (most likely) be added with the bgp fabrics. Removed it for now
though.

> > +++ b/proxmox-frr-templates/templates/frr.conf.jinja
> > +{% include "bgpd.jinja" %}
> >  {% include "isisd.jinja" %}
> > +{% include "prefix_lists.jinja" %}
> >  {% for line in custom_frr_config %}
> >  {{ line }}
> >  {% endfor %}
> >  {% include "fabricd.jinja" %}
> >  {% include "ospfd.jinja" %}
> >  {% include "access_lists.jinja" %}
> >  {% include "route_maps.jinja" %}
> > +{% include "ip_routes.jinja" %}
> >  {% include "protocol_routemaps.jinja" %}
> 
> Nit: prefix_lists ends up "separated" from the includes of access_lists and
> route_maps by custom_frr_config + fabricd + ospfd. Since route-maps
> reference prefix/access lists, grouping all filter objects together would be
> more conventional and might be maybe slightly friendlier to frr-reload.py's
> incremental processing.

This would be nice, but we can't do this since this would break
backwards-compat. This is the order (circa) in which the perl-code currently
generates and we need to preserve this because otherwise stuff becomes
overrideable by the frr.conf.local suddenly.