[PATCH network] fix #6255: SNAT: fix incorrect IP collection for sdn config

[PATCH network] fix #6255: SNAT: fix incorrect IP collection for sdn config

[Lukas Sichert]

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.

To use the updated ip address, parse the incoming /network/interfaces
file and look up the interface currently used. If the interface is still
present and has a gateway configured, but its ip address has changed,
update the ip address in the SDN configuration accordingly and log the
change to the journal.

If the interface or the gateway cannot be found, log this condition to
the journal and fall back to the IP address returned by ip route.
Here only print the warning for the missing gateway if the device does
not have a ip address configured.

Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
---

Notes:
    This is not a very beautiful nor robust fix to the problem. But without
    bigger changes for SNAT Stefan and me could not think of a better
    solution. I have thought about automatically switching to a interface
    with gateway, if the address reported by ip route cannot be found in
    /network/interfaces. This would look something like
    
    my $ifaces = $interfaces_config->{ifaces};
    for my $ifname (sort keys %{$ifaces // {}}) {
        my $ifgateway = $ifaces->{$ifname}->{gateway};
        my $ifaddress = $ifaces->{$ifname}->{address};
        if ( $ifgateway && $ifaddress ) {
            syslog("warning",
    "using interface $interface with ip address $ifaddress and gateway for SNAT"
            );
            syslog("info",
    "if you want to use the live kernel routing table for SNAT, delete all gateways from the interfaces file and reapply SDN"
            );
            return ($ifaddress, $ifname);
        }
    }
    
    But as this would prevent custom runtime routing configurations from
    being applied for resolving peer addresses in VxLan and EVPN I decided
    to exclude it.
    
    Also I am not very happy about the long warning, but I feel like the
    Information provided there should not be omitted.
    
    If anybody has any opinions or suggestions regarding this, please tell
    me.
    Thanks

 src/PVE/Network/SDN/Zones/EvpnPlugin.pm   |  2 +-
 src/PVE/Network/SDN/Zones/Plugin.pm       | 25 ++++++++++++++++++++---
 src/PVE/Network/SDN/Zones/SimplePlugin.pm |  2 +-
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
index 8e7ddfd..f69bc48 100644
--- a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
+++ b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
@@ -253,7 +253,7 @@ sub generate_sdn_config {
 
             #find outgoing interface
             my ($outip, $outiface) =
-                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip);
+                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..b021899 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,9 +280,9 @@ sub del_bridge_fdb {
 #helper
 
 sub get_local_route_ip {
-    my ($targetip) = @_;
+    my ($targetip, $interfaces_config) = @_;
 
-    my $ip = undef;
+    my $ip        = undef;
     my $interface = undef;
 
     run_command(
@@ -296,6 +297,24 @@ sub get_local_route_ip {
 
         },
     );
+    my $interface_in_config  = $interfaces_config->{ifaces}->{$interface};
+    my $ip_address_in_config = $interface_in_config->{address};
+    my $gateway_in_config    = $interface_in_config->{gateway};
+
+# 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 ne $ip) {
+            syslog( "warning",
+"ip address $ip_address_in_config of interface $interface in /etc/network/interfaces does not match with it ip address reported by ip route: $ip, switching to $ip_address_in_config for SNAT"
+            );
+        }
+        return ($ip_address_in_config, $interface);
+    }
+
+    if (!$interface_in_config) {
+        syslog( "warning", "current SNAT networking interface $interface is not listed in /etc/network/interfaces anymore");
+    } elsif (!$gateway_in_config && $ip_address_in_config) {
+        syslog( "warning", "currently used networking interface $interface does not have a gateway configured in /etc/network/interfaces"); }
     return ($ip, $interface);
 }
 
@@ -323,7 +342,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..11c031a 100644
--- a/src/PVE/Network/SDN/Zones/SimplePlugin.pm
+++ b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
@@ -113,7 +113,7 @@ sub generate_sdn_config {
         if ($subnet->{snat}) {
             #find outgoing interface
             my ($outip, $outiface) =
-                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip);
+                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip, $interfaces_config);
             if ($outip && $outiface) {
                 #use snat, faster than masquerade
                 push @iface_config,
-- 
2.47.3

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

[Gabriel Goller]

A `make tidy` run would be nice, there are some formatting issues :)

> [snip]
> diff --git a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> index 8e7ddfd..f69bc48 100644
> --- a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> +++ b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
> @@ -253,7 +253,7 @@ sub generate_sdn_config {
>  
>              #find outgoing interface
>              my ($outip, $outiface) =
> -                PVE::Network::SDN::Zones::Plugin::get_local_route_ip($checkrouteip);
> +                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..b021899 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,9 +280,9 @@ sub del_bridge_fdb {
>  #helper
>  
>  sub get_local_route_ip {
> -    my ($targetip) = @_;
> +    my ($targetip, $interfaces_config) = @_;
>  
> -    my $ip = undef;
> +    my $ip        = undef;
>      my $interface = undef;
>  
>      run_command(
> @@ -296,6 +297,24 @@ sub get_local_route_ip {
>  
>          },
>      );
> +    my $interface_in_config  = $interfaces_config->{ifaces}->{$interface};
> +    my $ip_address_in_config = $interface_in_config->{address};
> +    my $gateway_in_config    = $interface_in_config->{gateway};
> +
> +# 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 ne $ip) {
> +            syslog( "warning",
> +"ip address $ip_address_in_config of interface $interface in /etc/network/interfaces does not match with it ip address reported by ip route: $ip, switching to $ip_address_in_config for SNAT"

Typo: s/it ip/its ip/

Also maybe "... does not match with its actual ip address reported by ip route ..." -- IMO this is clearer.

> +            );
> +        }
> +        return ($ip_address_in_config, $interface);
> +    }
> +

Move this check directly below the declaration -- this way we can simplify the above if-statement and exit early.

> +    if (!$interface_in_config) {
> +        syslog( "warning", "current SNAT networking interface $interface is not listed in /etc/network/interfaces anymore");
> +    } elsif (!$gateway_in_config && $ip_address_in_config) {
> +        syslog( "warning", "currently used networking interface $interface does not have a gateway configured in /etc/network/interfaces"); }
>      return ($ip, $interface);
>  }
>  
> @@ -323,7 +342,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);
>      }
>  }

The rest looks good.

Acked-by: Gabriel Goller <g.goller@proxmox.com>

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

[Stefan Hanreich]

Thanks for looking at this! Some comments inline

On 3/9/26 10:14 AM, Lukas Sichert wrote:

[snip]

> @@ -296,6 +297,24 @@ sub get_local_route_ip {
>  
>          },
>      );
> +    my $interface_in_config  = $interfaces_config->{ifaces}->{$interface};
> +    my $ip_address_in_config = $interface_in_config->{address};
> +    my $gateway_in_config    = $interface_in_config->{gateway};
> +
> +# if the device currently used for routing still has a valid description in /network/interfaces/, use it
> +    if ($interface_in_config && $gateway_in_config) {

This only checks for the IPv4 case. But $targetip could be an IPv6
address as well, in which case we would have to check for address6 /
gateway6 respectively.

E.g. the following configuration on a test host:

auto vmbr0
iface vmbr0 inet static
        address 192.0.2.10/24
        gateway 192.0.2.1
        bridge-ports nic0
        bridge-stp off
        bridge-fd 0

iface vmbr0 inet6 static
        address 2001:db8::1234/64

with the following SDN config:

subnet: simplez-fc80::-60
        vnet simplnet
        gateway fc80::1
        snat 1

Results in an error, because the plugin tries to create an IPv6 NAT rule
with an IPv4 source:

simplnet : warning: simplnet: post-up cmd 'ip6tables -t nat -A
POSTROUTING -s 'fc80::/60' -o vmbr0 -j SNAT --to-source 192.0.2.10'
failed: returned 2 (ip6tables v1.8.11 (legacy): Bad IP address "192.0.2.10"

-------------------------------

There's also the case of users announcing a default GW via a dynamic
routing protocol. Usually it is advisable to configure a default gateway
in this case as a fallback, but with this change we'd always utilize the
fallback gateway instead of the *actual* gateway.

I can see this happening in some setups where the gateways announce
routes for 0.0.0.0/1 and 128.0.0.0/1 - effectively making them the
default routes.

> +        if ($ip_address_in_config ne $ip) {

whilst probably a bit niche - for IPv6, if we want to be super correct,
we cannot do a simple comparison, since it is possible to express the
same IP address in a multitude of ways. This is actually an issue in
find_local_ip_interface_peers as well.

Technically this holds true for IPv4 as well, so if we want to be really
correct we'd always need to parse the address and use the canonicalized
form there as well.

> +            syslog( "warning",
> +"ip address $ip_address_in_config of interface $interface in /etc/network/interfaces does not match with it ip address reported by ip route: $ip, switching to $ip_address_in_config for SNAT"
> +            );
> +        }
> +        return ($ip_address_in_config, $interface);
> +    }
> +
> +    if (!$interface_in_config) {
> +        syslog( "warning", "current SNAT networking interface $interface is not listed in /etc/network/interfaces anymore");
> +    } elsif (!$gateway_in_config && $ip_address_in_config) {
> +        syslog( "warning", "currently used networking interface $interface does not have a gateway configured in /etc/network/interfaces"); }

I know we talked about this already, but I'm not so sure anymore that
printing a warning here is such a good idea, because it can trigger in
quite a few scenarios that are completely legitimate and might confuse
users more than they help:

* Changing a network interface name (which would be only once at least)
* Configuring a gateway via post-up commands in the interfaces config
* Moving the default gateway to another interface


[snip]