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 297191FF137 for ; Tue, 03 Mar 2026 16:18:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8D31C1A126; Tue, 3 Mar 2026 16:19:13 +0100 (CET) Message-ID: <35bbc676-4261-4887-a5a4-9553d026a5bd@proxmox.com> Date: Tue, 3 Mar 2026 16:19:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network v2 6/9] sdn: adjust frr.conf.local merging to rust template types To: Gabriel Goller , pve-devel@lists.proxmox.com References: <20260302125701.196916-1-g.goller@proxmox.com> <20260302125701.196916-16-g.goller@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: <20260302125701.196916-16-g.goller@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.338 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.66 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.968 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 0.495 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: BGUSTQI7PQMGJPUCP4LXTHGNEPSQSIXJ X-Message-ID-Hash: BGUSTQI7PQMGJPUCP4LXTHGNEPSQSIXJ X-MailFrom: s.hanreich@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 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 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. > + $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) > + $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? > + 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. > + $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 > + 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? > } 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); > } > } >