Re: [PATCH networking v2] fix #6255: SNAT: fix incorrect IP collection for sdn config

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

functionality-wise looks fine, some annotations to the code inline

On 3/27/26 4:39 PM, Lukas Sichert wrote:
> On a host with a SimpleZone or EVPN and SNAT configured, when changing
> the ip address of the bridge that handles the SNAT, the changes are not
> reflected in /etc/network/interfaces.d/sdn. This happens because the ip
> address is resolved via 'ip route get 8.8.8.8'. By the time this command
> is executed, no ifreload has been executed yet, so ip route still
> contains the old ip address.
> 
> Determine the IP version (IPv4 or IPv6) of the address used with 'ip
> route'. Then parse the incoming /etc/network/interfaces file and look up
> the interface currently used by 'ip route'. If the interface is still
> present and has a gateway and ip address with the correct version, but
> the ip address has changed, update the ip address in the SDN
> configuration accordingly and log the change to the journal.
> 
> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
> ---
> 
> Notes:
>     changes from v1 to v2: (thanks @Gabriel, @Stefan)
>     -add handling for IPV6 addresses
>     -remove warnings for missing ip/gateway, as they might trigger
>       in some legitimate scenarios
>     -add canonical formatting for IPv4 and IPV6
>     -execute make tidy
>     -small restructuring of if clauses for earlier exit
>     -fix typos
>     
>     concerning the ignoring of live routing tables:
>     
>     The function only determines the currently used interface (via 'ip
>     route') and its ip address and returns these two. The gateway configured
>     in /etc/network/interfaces is only used to verify that the interface is
>     still a valid candidate after an 'ifreload -a'. As it cannot switch
>     interfaces relative what to 'ip route' reports it should not be able
>     influence gateway selection.
>     
>     If I am missing a more obscure way or an edge case where gateway
>     selection could still be affected, please let me know.
>     
>     Also I have thought about adding "... does not match with its actual ip
>     address reported by ip route ...", but after this would be displayed
>     'ifreload -a' gets executed and 'ip route' changes.
> 
>  src/PVE/Network/SDN/Zones/EvpnPlugin.pm   |  5 ++--
>  src/PVE/Network/SDN/Zones/Plugin.pm       | 30 +++++++++++++++++++++--
>  src/PVE/Network/SDN/Zones/SimplePlugin.pm |  5 ++--
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> index 8e7ddfd..91b8a7e 100644
> --- a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> +++ b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> @@ -252,8 +252,9 @@ sub generate_sdn_config {
>          if ($subnet->{snat}) {
>  
>              #find outgoing interface
> -            my ($outip, $outiface) =
> -                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip);
> +            my ($outip, $outiface) = PVE::Network::SDN::Zones::Plugin::get_local_route_ip(
> +                $checkrouteip, $interfaces_config,
> +            );
>              if ($outip && $outiface && $is_evpn_gateway) {
>                  #use snat, faster than masquerade
>                  push @iface_config,
> diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
> index 826ebdf..1d18acc 100644
> --- a/src/PVE/Network/SDN/Zones/Plugin.pm
> +++ b/src/PVE/Network/SDN/Zones/Plugin.pm
> @@ -8,6 +8,7 @@ use PVE::IPRoute2;
>  use PVE::JSONSchema;
>  use PVE::Cluster;
>  use PVE::Network;
> +use PVE::SafeSyslog;
>  
>  use PVE::JSONSchema qw(get_standard_option);
>  use base qw(PVE::SectionConfig);
> @@ -279,7 +280,7 @@ sub del_bridge_fdb {
>  #helper
>  
>  sub get_local_route_ip {
> -    my ($targetip) = @_;
> +    my ($targetip, $interfaces_config) = @_;
>  
>      my $ip = undef;
>      my $interface = undef;
> @@ -296,6 +297,31 @@ sub get_local_route_ip {
>  
>          },
>      );
> +    my $ipversion = Net::IP::ip_is_ipv6($targetip) ? 6 : 4;
> +    my $interface_in_config = $interfaces_config->{ifaces}->{$interface};
> +    my $ip_address_in_config = undef;
> +    my $gateway_in_config = undef;
> +
> +    if ($ipversion == 6) {
> +        $ip_address_in_config = PVE::Network::canonical_ip($interface_in_config->{address6});
> +        $gateway_in_config = PVE::Network::canonical_ip($interface_in_config->{gateway6});
> +    } else {
> +        $ip_address_in_config = PVE::Network::canonical_ip($interface_in_config->{address});
> +        $gateway_in_config = PVE::Network::canonical_ip($interface_in_config->{gateway});
> +    }

nit: potentially the following is a bit nicer?

my $address_key = ($ipversion == 6) ? 'address6' : 'address';
my $ip_address_in_config =
PVE::Network::canonical_ip($interface_in_config->{$address_key});


> +    # if the device currently used for routing still has a valid description in /network/interfaces/, use it
> +    if ($interface_in_config && $gateway_in_config) {
> +        if ($ip_address_in_config eq $ip) {
> +            return ($ip_address_in_config, $interface);
> +        } else {
> +            syslog(
> +                "warning",
> +                "ip address $ip_address_in_config of interface $interface in /etc/network/interfaces does not match with its ip address reported by ip route: $ip, switching to $ip_address_in_config for SNAT",
> +            );
> +            return ($ip_address_in_config, $interface);
> +        }
> +    }
>      return ($ip, $interface);

nit: could be shortened

if (
  $interface_in_config
  && $gateway_in_config
  && $ip_address_in_config ne $ip
) {
  syslog(..)
  return ($ip_address_in_config, $interface);
}

return ($ip, $interface);

>  }
>  
> @@ -323,7 +349,7 @@ sub find_local_ip_interface_peers {
>  
>      #if peer is remote, find source with ip route
>      foreach my $address (@{$peers}) {
> -        my ($ip, $interface) = get_local_route_ip($address);
> +        my ($ip, $interface) = get_local_route_ip($address, $network_config);
>          return ($ip, $interface);
>      }
>  }
> diff --git a/src/PVE/Network/SDN/Zones/SimplePlugin.pm b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
> index f5cd18e..ac772e7 100644
> --- a/src/PVE/Network/SDN/Zones/SimplePlugin.pm
> +++ b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
> @@ -112,8 +112,9 @@ sub generate_sdn_config {
>          push @iface_config, "up ip route add $cidr dev $vnetid" if $mask == 32 && $ipversion == 4;
>          if ($subnet->{snat}) {
>              #find outgoing interface
> -            my ($outip, $outiface) =
> -                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip);
> +            my ($outip, $outiface) = PVE::Network::SDN::Zones::Plugin::get_local_route_ip(
> +                $checkrouteip, $interfaces_config,
> +            );
>              if ($outip && $outiface) {
>                  #use snat, faster than masquerade
>                  push @iface_config,