From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 4AFA71FF138 for ; Wed, 04 Mar 2026 15:37:17 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AAEC0C6AE; Wed, 4 Mar 2026 15:38:19 +0100 (CET) Date: Wed, 4 Mar 2026 15:37:44 +0100 From: Gabriel Goller To: Stefan Hanreich Subject: Re: [PATCH pve-network v2 6/9] sdn: adjust frr.conf.local merging to rust template types Message-ID: Mail-Followup-To: Stefan Hanreich , pve-devel@lists.proxmox.com References: <20260302125701.196916-1-g.goller@proxmox.com> <20260302125701.196916-16-g.goller@proxmox.com> <35bbc676-4261-4887-a5a4-9553d026a5bd@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <35bbc676-4261-4887-a5a4-9553d026a5bd@proxmox.com> User-Agent: NeoMutt/20241002-35-39f9a6 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1772635038326 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.668 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.322 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 1.141 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 5UQ23MW2RO4GW34QE5IGNE5CP4NGX7A3 X-Message-ID-Hash: 5UQ23MW2RO4GW34QE5IGNE5CP4NGX7A3 X-MailFrom: g.goller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 03.03.2026 16:19, Stefan Hanreich wrote: > On 3/2/26 1:56 PM, Gabriel Goller wrote: > > The frr object in perl which stores the whole frr config is now also > > modeled in rust, so it changed a bit. Adjust the frr.conf.local merging > > code so that the frr.conf.local is still merged correctly. This makes > > use of the `custom_frr_config` properties scattered in many rust types. > > So if we encounter a line in frr.conf.local that we need to merge, we > > just throw it into this string vec and render it as-is. > > > > Co-authored-by: Stefan Hanreich > > Signed-off-by: Gabriel Goller > > --- > > src/PVE/Network/SDN/Frr.pm | 202 ++++++++++++++++++++++++++++++------- > > 1 file changed, 168 insertions(+), 34 deletions(-) > > > > diff --git a/src/PVE/Network/SDN/Frr.pm b/src/PVE/Network/SDN/Frr.pm > > index b572f4536004..af3074017ddf 100644 > > --- a/src/PVE/Network/SDN/Frr.pm > > +++ b/src/PVE/Network/SDN/Frr.pm > > @@ -271,70 +271,204 @@ sub append_local_config { > > return if !$local_config; > > > > my $section = \$frr_config->{""}; > > - my $router = undef; > > + my $isis_router_name = undef; > > + my $bgp_router_asn = undef; > > + my $bgp_router_vrf = undef; > > my $routemap = undef; > > - my $routemap_config = (); > > - my $routemap_action = undef; > > + my $interface = undef; > > + my $vrf = undef; > > + my $new_block = 0; > > + my $new_af_block = 0; > > > > - while ($local_config =~ /^\s*(.+?)\s*$/gm) { > > + while ($local_config =~ /^(.+?)\s*$/gm) { > > my $line = $1; > > - $line =~ s/^\s+|\s+$//g; > > - > > - if ($line =~ m/^router (.+)$/) { > > - $router = $1; > > - $section = \$frr_config->{'frr'}->{'router'}->{$router}->{""}; > > after this change merging now only works for isis and bgp routers? Did a > quick check with other routing protocols in frr.conf.local and the > routers seem to be missing. Good catch, fixed this! > > + $line =~ s/\s+$//g; > > + > > + if ($line =~ m/^router isis (.+)$/) { > > + $isis_router_name = $1; > > + if (defined $frr_config->{'frr'}->{'isis'}->{'router'}->{$isis_router_name}) { > > calls to defined should use parentheses (several occurences below) done! > > + $section = > > + \($frr_config->{'frr'}->{'isis'}->{'router'}->{$isis_router_name} > > + ->{'custom_frr_config'} //= []); > > + } else { > > + $new_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, > > + "router isis $isis_router_name", > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > + next; > > + } elsif ($line =~ m/^router bgp (\S+)(?: vrf (.+))?$/) { > > + $bgp_router_asn = $1; > > + $bgp_router_vrf = $2 // 'default'; > > + > > + my $config_line = > > + defined($2) > > + ? "router bgp $bgp_router_asn vrf $bgp_router_vrf" > > + : "router bgp $bgp_router_asn"; > > + > > this is only required in the else branch? moved it down. > > + if ( > > + defined $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + and $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf}->{'asn'} > > + eq $bgp_router_asn > > + ) { > > + $section = > > + \($frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'custom_frr_config'} //= []); > > + } else { > > + $new_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, $config_line, > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > next; > > } elsif ($line =~ m/^vrf (.+)$/) { > > - $section = \$frr_config->{'frr'}->{'vrf'}->{$1}; > > + $vrf = $1; > > + if (defined $frr_config->{'frr'}->{'bgp'}->{'vrfs'}->{$vrf}) { > > + $section = \$frr_config->{'frr'}->{'bgp'}->{'vrfs'}->{$vrf}->{'custom_frr_config'}; > > + } else { > > + $new_block = 1; > > + push($frr_config->{'frr'}->{'custom_frr_config'}->@*, "vrf $vrf"); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > next; > > } elsif ($line =~ m/^interface (.+)$/) { > > - $section = \$frr_config->{'frr_interfaces'}->{$1}; > > + $interface = $1; > > + if (defined $frr_config->{'frr'}->{'isis'}->{'interfaces'}->{$interface}) { > > trying to override an existing IS-IS interface doesn't work for me, e.g. > with the following IS-IS controller: > > isis: isisberserker > isis-domain 1 > isis-ifaces ens19,ens20 > isis-net 49.0000.1234.0000.00 > node berserker > > and the following frr.conf.local: > > interface ens20 > isis circuit-type level-2-only > exit > ! > > I get an deserialization error: > > error: invalid type: Option value, expected a sequence > > > This seems to be because the generated $frr_config has > > 'custom_frr_config' => undef > > in its top-level. fixed this. > > + $section = \($frr_config->{'frr'}->{'isis'}->{'interfaces'}->{$interface} > > + ->{'custom_frr_config'} //= []); > > + } else { > > + $new_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, "interface $interface", > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > next; > > } elsif ($line =~ m/^bgp community-list (.+)$/) { > > - push(@{ $frr_config->{'frr_bgp_community_list'} }, $line); > > + push(@{ $frr_config->{'frr'}->{'custom_frr_config'} }, $line); > > next; > > } elsif ($line =~ m/address-family (.+)$/) { > > - $section = \$frr_config->{'frr'}->{'router'}->{$router}->{'address-family'}->{$1}; > > + # convert the address family from frr (e.g. l2vpn evpn) into the rust property (e.g. l2vpn_evpn) > > + my $address_family_unchanged = $1; > > + my $address_family = $1 =~ s/ /_/gr; > > + > > + if ( > > + defined $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + and $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf}->{'asn'} > > + eq $bgp_router_asn > > + ) { > > + if ( > > + defined $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'address_families'}->{$address_family} > > + ) { > > + $section = > > + \($frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'address_families'}->{$address_family}->{custom_frr_config} //= []); > > + } else { > > + $new_af_block = 1; > > + push( > > + $frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'custom_frr_config'}->@*, > > + " address-family $address_family_unchanged", > > + ); > > + $section = \$frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'custom_frr_config'}; > > + } > > + } else { > > + $new_af_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, > > + " address-family $address_family_unchanged", > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > next; > > } elsif ($line =~ m/^route-map (.+) (permit|deny) (\d+)/) { > > $routemap = $1; > > - $routemap_config = (); > > - $routemap_action = $2; > > - $section = \$frr_config->{'frr_routemap'}->{$routemap}; > > + my $routemap_action = $2; > > + my $seq_number = $3; > > + if (defined $frr_config->{'frr'}->{'routemaps'}->{$routemap}) { > > + my $index = 0; > > + foreach my $single_routemap ($frr_config->{'frr'}->{'routemaps'}->{$routemap}->@*) { > > for is preferred over foreach, see: > https://pve.proxmox.com/wiki/Perl_Style_Guide#Perl_syntax_choices done. > > + if ( > > + $single_routemap->{'seq'} == $seq_number > > + && $single_routemap->{'action'} eq $routemap_action > > + ) { > > + last; > > + } > > + $index++; > > + } > > + if ($index < scalar @{ $frr_config->{'frr'}->{'routemaps'}->{$routemap} }) { > > + $section = \($frr_config->{'frr'}->{'routemaps'}->{$routemap}->[$index] > > + ->{'custom_frr_config'} //= []); > > + } else { > > + $new_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, > > + "route-map $routemap $routemap_action $seq_number", > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > + } else { > > + $new_block = 1; > > + push( > > + $frr_config->{'frr'}->{'custom_frr_config'}->@*, > > + "route-map $routemap $routemap_action $seq_number", > > + ); > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + } > > next; > > adding entries for an existing route-map (i.e. MAP_VTEP_IN) like so: > > route-map MAP_VTEP_IN deny 1 > exit > ! > route-map MAP_VTEP_IN permit 1 > exit > ! > route-map MAP_VTEP_IN permit 1 > match community cm-prefmod-400 > set local-preference 400 > exit > ! > route-map MAP_VTEP_IN permit 1 > match ip address 10 > set local-preference 200 > exit > ! > > merges the route-map configuration with this patch, if the verdict is > the same: > > route-map MAP_VTEP_IN permit 1 > match community cm-prefmod-400 > set local-preference 400 > match ip address 10 > set local-preference 200 > exit > ! > > the section with the same seq nr, but different verdict gets > additionally added: > > route-map MAP_VTEP_IN deny 1 > exit > ! > > ------------------------------------------ > > with the old merging logic, they were added as separate sections, with > increasing numbers: > > route-map MAP_VTEP_IN permit 1 > exit > ! > route-map MAP_VTEP_IN deny 2 > exit > ! > route-map MAP_VTEP_IN permit 3 > exit > ! > route-map MAP_VTEP_IN permit 4 > match community cm-prefmod-400 > set local-preference 400 > exit > ! > route-map MAP_VTEP_IN permit 5 > match ip address 10 > set local-preference 200 > exit > ! > > Is this intentional? Imo this is quite the breaking change, particularly > because route-maps are probably used relatively often in the frr.conf.local? fixed this as well I hope. > > } elsif ($line =~ m/^access-list (.+) seq (\d+) (.+)$/) { > > - $frr_config->{'frr_access_list'}->{$1}->{$2} = $3; > > + push($frr_config->{'frr'}->{'custom_frr_config'}->@*, $line); > > next; > > } elsif ($line =~ m/^ip prefix-list (.+) seq (\d+) (.*)$/) { > > - $frr_config->{'frr_prefix_list'}->{$1}->{$2} = $3; > > + push($frr_config->{'frr'}->{'custom_frr_config'}->@*, $line); > > next; > > } elsif ($line =~ m/^ipv6 prefix-list (.+) seq (\d+) (.*)$/) { > > - $frr_config->{'frr_prefix_list_v6'}->{$1}->{$2} = $3; > > + push($frr_config->{'frr'}->{'custom_frr_config'}->@*, $line); > > next; > > - } elsif ($line =~ m/^exit-address-family$/) { > > + } elsif ($line =~ m/exit-address-family$/) { > > + if ($new_af_block) { > > + push(@{$$section}, $line); > > + $section = \$frr_config->{'frr'}->{'bgp'}->{'custom_frr_config'}; > > + } else { > > + $section = > > + \($frr_config->{'frr'}->{'bgp'}->{'vrf_router'}->{$bgp_router_vrf} > > + ->{'custom_frr_config'} //= []); > > + } > > + $new_af_block = 0; > > next; > > - } elsif ($line =~ m/^exit$/) { > > - if ($router) { > > - $section = \$frr_config->{''}; > > - $router = undef; > > - } elsif ($routemap) { > > - push(@{$$section}, { rule => $routemap_config, action => $routemap_action }); > > - $section = \$frr_config->{''}; > > + } elsif ($line =~ m/^exit/) { > > + if ($bgp_router_vrf || $vrf || $interface || $routemap || $isis_router_name) { > > + # this means we just added a new router/vrf/interface/routemap > > + if ($new_block) { > > + push(@{$$section}, $line); > > + push(@{$$section}, "!"); > > + } > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + # we can't stack these, so exit out of all of them (technically we can have a vrf inside of a router bgp block, but we don't support that) > > + $isis_router_name = undef; > > + $bgp_router_vrf = undef; > > + $bgp_router_asn = undef; > > + $vrf = undef; > > + $interface = undef; > > $routemap = undef; > > - $routemap_action = undef; > > - $routemap_config = (); > > + } else { > > + $section = \$frr_config->{'frr'}->{'custom_frr_config'}; > > + push(@{$$section}, $line); > > + push(@{$$section}, "!"); > > } > > + $new_block = 0; > > next; > > } elsif ($line =~ m/!/) { > > next; > > } > > > > next if !$section; > > - if ($routemap) { > > - push(@{$routemap_config}, $line); > > - } else { > > - push(@{$$section}, $line); > > - } > > + push(@{$$section}, $line); > > } > > } > > Will send a new version soon!