[pve-devel] [RFC common/firewall/manager/network/proxmox{-ve-rs, -firewall} 0/7] NIC renaming mitigations

[pve-devel] [RFC common/firewall/manager/network/proxmox{-ve-rs, -firewall} 0/7] NIC renaming mitigations

[Stefan Hanreich]

This patch series contains the following features:
* transparent altname support for {pve, proxmox}-firewall and pve-network
* pveeth tool for pinning NIC names

Both are features aimed at mitigating the fallout caused from changing network
interface names. Sending it as an RFC, since I will be gone for a few days and
wanted to publish my current state to start some discussion on the approaches
I've taken with the tools and possible additions / changes. Nothing in here is
final or particularly polished.

Both patch series only received rudimentary testing and are work in progress, so
use at your own risk, I am not responsible for any broken hosts / VMs.

For more information on the pveeth tool, see the respective commit.

TODO:
* possibly change wakeonlan setting in node config
* decide on how to handle host.fw / cluster.fw:

cluster.fw cannot be automatically updated, since the generated mapping might
differ from the one generated on other nodes. One possibility would be to
generate the mapping for the NICs one-by-one on each host, thus ensuring a
consistent name on all nodes. Then add a flag that overwrites cluster.fw.

cluster/host.fw is the only configuration file that gets applied immediately
when updating it, since the firewall continously polls this file and applies the
settings. We could add the new name as altname via ip link, ensuring that the
firewall rules still work before *and* after reboot. Shouldn't be too hard to
add (possibly with a flag). This is possible because of the new altname support
{pve, proxmox}-firewall.

* update detection of physical NICs

We currently rely on the PHYSICAL_NIC_RE to detect physical network interfaces.
We could instead use the ip link output for determining whether an interface is
physical or not. This works in every case, except for PullMetric.pm. For this we
could introduce another variable and fall back on the old logic depending on its
existence. Maybe some one with more knowledge on the metrics system can chime in
here. I have patches for this on my staff repo in case you are interested:

pve-manager:physical-nic-re
pve-common:physical-nic-re

pve-common:

Stefan Hanreich (2):
  network: add ip link and altname helpers
  network: add nic prefix to physical nic regex

 src/PVE/Network.pm | 47 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)


proxmox-ve-rs:

Stefan Hanreich (1):
  config: ip link struct

 proxmox-ve-config/src/host/mod.rs     |  1 +
 proxmox-ve-config/src/host/network.rs | 35 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 proxmox-ve-config/src/host/network.rs


proxmox-firewall:

Stefan Hanreich (1):
  firewall: add altname support for firewall rules

 proxmox-firewall/src/config.rs              | 29 +++++++++++++++++++++
 proxmox-firewall/src/rule.rs                |  6 ++++-
 proxmox-firewall/tests/integration_tests.rs |  7 +++++
 3 files changed, 41 insertions(+), 1 deletion(-)


pve-firewall:

Stefan Hanreich (1):
  firewall: add altname support

 src/PVE/Firewall.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


pve-network:

Stefan Hanreich (1):
  controllers: isis: add altname support

 src/PVE/Network/SDN/Controllers/IsisPlugin.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


pve-manager:

Stefan Hanreich (1):
  cli: add pveeth

 PVE/CLI/Makefile  |   1 +
 PVE/CLI/pveeth.pm | 538 ++++++++++++++++++++++++++++++++++++++++++++++
 bin/Makefile      |   5 +
 bin/pveeth        |   8 +
 4 files changed, 552 insertions(+)
 create mode 100644 PVE/CLI/pveeth.pm
 create mode 100644 bin/pveeth


Summary over all repositories:
  12 files changed, 684 insertions(+), 5 deletions(-)

-- 
Generated by git-murpp 0.8.0

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-common 1/2] network: add ip link and altname helpers

[Stefan Hanreich]

Those helpers will be used by several other packages to implement the
altname support. Those helpers will also be used by the new pveeth
tool which can be used for pinning interface names.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network.pm | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index d084b36..ce87b93 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -973,4 +973,49 @@ sub is_ovs_bridge {
     die "failed to query OVS to determine type of '$bridge': $res\n";
 }
 
+sub ip_link_details {
+    my $link_json = '';
+
+    PVE::Tools::run_command(
+        ['ip', '-details', '-json', 'link', 'show'],
+        outfunc => sub {
+            $link_json .= shift;
+        }
+    );
+
+    my $links = JSON::decode_json($link_json);
+    my %ip_links = map { $_->{ifname} => $_ } $links->@*;
+
+    return \%ip_links;
+}
+
+sub ip_link_is_physical {
+    my ($ip_link) = @_;
+
+    # ether alone isn't enough, as virtual interfaces can also have link_type
+    # ether
+    return $ip_link->{link_type} eq 'ether'
+        && (!defined($ip_link->{linkinfo}) || !defined($ip_link->{linkinfo}->{info_kind}));
+}
+
+sub altname_mapping {
+    my ($ip_links) = @_;
+
+    $ip_links = ip_link_details() if !defined($ip_links);
+
+    my $altnames = {};
+
+    foreach my $iface_name (keys $ip_links->%*) {
+        my $iface = $ip_links->{$iface_name};
+
+        next if !$iface->{altnames};
+
+        foreach my $altname ($iface->{altnames}->@*) {
+            $altnames->{$altname} = $iface_name;
+        }
+    }
+
+    return $altnames;
+}
+
 1;
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-common 2/2] network: add nic prefix to physical nic regex

[Stefan Hanreich]

With the introduction of pveeth, users can now pin their NICs with
prefix nicX. In order for our stack to correctly pick up the pinned
interfaces, we need to add this prefix to the regex used for detecting
physical interfaces.

In the future we should abandon this method of detecting physical
interfaces altogether, either by using `ip link` or talking Netlink
directly. For now, we add this as a stop-gap so the pveeth tool
proof-of-concept works.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Network.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Network.pm b/src/PVE/Network.pm
index ce87b93..b305105 100644
--- a/src/PVE/Network.pm
+++ b/src/PVE/Network.pm
@@ -17,7 +17,7 @@ use Socket qw(NI_NUMERICHOST NI_NUMERICSERV);
 
 # host network related utility functions
 
-our $PHYSICAL_NIC_RE = qr/(?:eth\d+|en[^:.]+|ib[^:.]+)/;
+our $PHYSICAL_NIC_RE = qr/(?:eth\d+|en[^:.]+|ib[^:.]+|nic\d+)/;
 
 our $ipv4_reverse_mask = [
     '0.0.0.0',
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-ve-rs 1/1] config: ip link struct

[Stefan Hanreich]

Add a bare-bones struct for parsing the output of `ip -details -json
link show`. Currently we only require the name of the interfaces as
well as its altnames for transparently supporting altnames in the
firewall.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Notes:
    There is probably a better place than proxmox-ve-config for this. I've
    put it here for now, since we have some network-related stuff in this
    repository already. Mid to short-term it would be better to extract
    all this stuff into either a separate crate or proxmox-sys. For this
    POC its fine.

 proxmox-ve-config/src/host/mod.rs     |  1 +
 proxmox-ve-config/src/host/network.rs | 35 +++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 proxmox-ve-config/src/host/network.rs

diff --git a/proxmox-ve-config/src/host/mod.rs b/proxmox-ve-config/src/host/mod.rs
index b4ab6a6..a9da919 100644
--- a/proxmox-ve-config/src/host/mod.rs
+++ b/proxmox-ve-config/src/host/mod.rs
@@ -1,2 +1,3 @@
+pub mod network;
 pub mod types;
 pub mod utils;
diff --git a/proxmox-ve-config/src/host/network.rs b/proxmox-ve-config/src/host/network.rs
new file mode 100644
index 0000000..09f4fb1
--- /dev/null
+++ b/proxmox-ve-config/src/host/network.rs
@@ -0,0 +1,35 @@
+use std::collections::HashMap;
+
+#[derive(Debug, Clone, serde::Deserialize)]
+pub struct IpLink {
+    ifname: String,
+    #[serde(default)]
+    altnames: Vec<String>,
+}
+
+#[derive(Debug, Clone, serde::Deserialize)]
+pub struct InterfaceMapping {
+    mapping: HashMap<String, String>,
+}
+
+impl std::ops::Deref for InterfaceMapping {
+    type Target = HashMap<String, String>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.mapping
+    }
+}
+
+impl FromIterator<IpLink> for InterfaceMapping {
+    fn from_iter<T: IntoIterator<Item = IpLink>>(iter: T) -> Self {
+        let mut mapping = HashMap::new();
+
+        for iface in iter.into_iter() {
+            for altname in iface.altnames {
+                mapping.insert(altname, iface.ifname.clone());
+            }
+        }
+
+        Self { mapping }
+    }
+}
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall 1/1] firewall: add altname support for firewall rules

[Stefan Hanreich]

This works by reading all the currently configured altnames and then
replacing any occurences of altnames when creating the firewall rules.
We handle it this way because nftables has no support for matching on
the altnames of interfaces.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 proxmox-firewall/src/config.rs              | 29 +++++++++++++++++++++
 proxmox-firewall/src/rule.rs                |  6 ++++-
 proxmox-firewall/tests/integration_tests.rs |  7 +++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
index ec9849e..f07fb1e 100644
--- a/proxmox-firewall/src/config.rs
+++ b/proxmox-firewall/src/config.rs
@@ -13,6 +13,8 @@ use proxmox_ve_config::firewall::types::alias::{Alias, AliasName, AliasScope};
 
 use proxmox_ve_config::guest::types::Vmid;
 use proxmox_ve_config::guest::{GuestEntry, GuestMap};
+use proxmox_ve_config::host::network::InterfaceMapping;
+use proxmox_ve_config::host::network::IpLink;
 use proxmox_ve_config::host::types::BridgeName;
 
 use proxmox_nftables::command::{CommandOutput, Commands, List, ListOutput};
@@ -40,6 +42,7 @@ pub trait FirewallConfigLoader {
         &self,
         bridge_name: &BridgeName,
     ) -> Result<Option<Box<dyn io::BufRead>>, Error>;
+    fn interface_mapping(&self) -> Result<InterfaceMapping, Error>;
 }
 
 #[derive(Default)]
@@ -221,6 +224,26 @@ impl FirewallConfigLoader for PveFirewallConfigLoader {
 
         Ok(None)
     }
+
+    fn interface_mapping(&self) -> Result<InterfaceMapping, Error> {
+        let output = std::process::Command::new("ip")
+            .arg("-details")
+            .arg("-json")
+            .arg("link")
+            .arg("show")
+            .stdout(std::process::Stdio::piped())
+            .output()
+            .with_context(|| "could not obtain ip link output")?;
+
+        if !output.status.success() {
+            bail!("ip link returned non-zero exit code")
+        }
+
+        Ok(serde_json::from_slice::<Vec<IpLink>>(&output.stdout)
+            .with_context(|| "could not deserialize ip link output")?
+            .into_iter()
+            .collect())
+    }
 }
 
 pub trait NftConfigLoader {
@@ -255,6 +278,7 @@ pub struct FirewallConfig {
     nft_config: BTreeMap<String, ListChain>,
     sdn_config: Option<SdnConfig>,
     ipam_config: Option<Ipam>,
+    interface_mapping: InterfaceMapping,
 }
 
 impl FirewallConfig {
@@ -380,6 +404,7 @@ impl FirewallConfig {
             sdn_config: Self::parse_sdn(firewall_loader)?,
             ipam_config: Self::parse_ipam(firewall_loader)?,
             nft_config: Self::parse_nft(nft_loader)?,
+            interface_mapping: firewall_loader.interface_mapping()?,
         })
     }
 
@@ -415,6 +440,10 @@ impl FirewallConfig {
         self.cluster().is_enabled() && self.host().nftables()
     }
 
+    pub fn interface_mapping(&self, iface_name: &str) -> Option<&str> {
+        self.interface_mapping.get(iface_name).map(|x| x.as_str())
+    }
+
     pub fn alias(&self, name: &AliasName, vmid: Option<Vmid>) -> Option<&Alias> {
         log::trace!("getting alias {name:?}");
 
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 14ee544..c4975a9 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -135,7 +135,11 @@ impl NftRuleEnv<'_> {
 
                 rule_iface.to_string()
             }
-            None => rule_iface.to_string(),
+            None => self
+                .firewall_config
+                .interface_mapping(rule_iface)
+                .map(|iface_name| iface_name.to_string())
+                .unwrap_or_else(|| rule_iface.to_string()),
         }
     }
 
diff --git a/proxmox-firewall/tests/integration_tests.rs b/proxmox-firewall/tests/integration_tests.rs
index 1c014ad..69f9cc2 100644
--- a/proxmox-firewall/tests/integration_tests.rs
+++ b/proxmox-firewall/tests/integration_tests.rs
@@ -1,4 +1,5 @@
 use anyhow::{Context, Error};
+use proxmox_ve_config::host::network::InterfaceMapping;
 use std::collections::HashMap;
 
 use proxmox_firewall::config::{FirewallConfig, FirewallConfigLoader, NftConfigLoader};
@@ -91,6 +92,12 @@ impl FirewallConfigLoader for MockFirewallConfigLoader {
     ) -> Result<Option<Box<dyn std::io::BufRead>>, Error> {
         Ok(None)
     }
+
+    fn interface_mapping(
+        &self,
+    ) -> Result<proxmox_ve_config::host::network::InterfaceMapping, Error> {
+        Ok(InterfaceMapping::from_iter(vec![]))
+    }
 }
 
 struct MockNftConfigLoader {}
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-firewall 1/1] firewall: add altname support

[Stefan Hanreich]

Add support for altnames by transparently mapping them with the
information from 'ip link' when generating the ruleset. The firewall
will now replace any altname in the ruleset with the actual, physical,
name from the interface. We handle it this way, because iptables
cannot match on the altnames on interfaces, only the 'real' name.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/PVE/Firewall.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 173ce98..e3d21f6 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -2861,6 +2861,8 @@ sub enable_host_firewall {
     my $rules = $hostfw_conf->{rules};
     my $cluster_rules = $cluster_conf->{rules};
 
+    my $interface_mapping = PVE::Network::altname_mapping();
+
     # corosync preparation
     my $corosync_rule = "-p udp --dport 5404:5405";
     my $corosync_local_addresses = {};
@@ -2908,7 +2910,7 @@ sub enable_host_firewall {
         next if !$rule->{enable} || $rule->{errors};
         next if $rule->{ipversion} && ($rule->{ipversion} != $ipversion);
 
-        $rule->{iface_in} = $rule->{iface} if $rule->{iface};
+        $rule->{iface_in} = ($interface_mapping->{$rule->{iface}} // $rule->{iface}) if $rule->{iface};
 
         eval {
             $rule->{logmsg} = "$rule->{action}: ";
@@ -2994,7 +2996,8 @@ sub enable_host_firewall {
         next if !$rule->{enable} || $rule->{errors};
         next if $rule->{ipversion} && ($rule->{ipversion} != $ipversion);
 
-        $rule->{iface_out} = $rule->{iface} if $rule->{iface};
+        $rule->{iface_out} = ($interface_mapping->{$rule->{iface}} // $rule->{iface}) if $rule->{iface};
+
         eval {
             $rule->{logmsg} = "$rule->{action}: ";
             if ($rule->{type} eq 'group') {
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-network 1/1] controllers: isis: add altname support

[Stefan Hanreich]

Since this only has an effect on applying the configuration, users
will still need to reapply the configuration when an interface changes
names / altnames. In order to add full altname support for IS-IS,
altname support would need to be implemented in FRR.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---

Notes:
    Gabriel is currently working on altname support in FRR [1]
    
    [1] https://github.com/FRRouting/frr/pull/19156

 src/PVE/Network/SDN/Controllers/IsisPlugin.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/Controllers/IsisPlugin.pm b/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
index 6e3574d..0ef9fb9 100644
--- a/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
+++ b/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
@@ -80,9 +80,12 @@ sub generate_controller_config {
 
     my @iface_config = ("ip router isis $isis_domain");
 
+    my $altnames = PVE::Network::altname_mapping();
+
     my @ifaces = PVE::Tools::split_list($isis_ifaces);
     for my $iface (sort @ifaces) {
-        push(@{ $config->{frr_interfaces}->{$iface} }, @iface_config);
+        my $iface_name = $altnames->{$iface} // $iface;
+        push(@{ $config->{frr_interfaces}->{$iface_name} }, @iface_config);
     }
 
     return $config;
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Stefan Hanreich]

pveeth is a tool for pinning / unpinning network interface names. It
works by generating a link file in /usr/local/lib/systemd/network and
then updating the following files by replacing the old name with the
pinned name (this also works for altnames!):

* /etc/network/interfaces
* /etc/pve/nodes/nodename/host.fw
* /etc/pve/sdn/controllers.cfg (IS-IS controllers)

There are still some places where interface names occur, where we do
not update the configuration:

/etc/pve/firewall/cluster.fw - This is because we cannot update a
cluster-wide file with the locally-generated mappings. In this case a
warning is printed.

In the node configuration there is a parameter for wakeonlan that
takes an interface as argument. It is not updated currently.

Otherwise all occurrences of interfaces or interface lists should be
included.

It is also possible to remove pins for interfaces by using the unpin
command. Successive invocations of pin and unpin with the same
parameters should be idempotent.

Example invocations of pveeth:

$ pveeth pin --nic enp1s0 --force 0 --dry_run 0

Generates a pinning for enp1s0 (if it doesn't exist already) and
updates the configuration file.

$ pveeth pin --force 1 --dry_run 0

Deletes all existing pins and re-generates them.

For more information on the parameters see the API description.

I've decided to let dry_run generate the configuration files in the
current working directory, since it is then easy to diff the generated
files with the existing configuration files using the diffviewer of
the users' choice.

Additionally, when writing the configuration files, they get backed up
by creating a .bak at the location of the configuration file.

Currently we only support a fixed prefix: 'nic'. This is because we
rely on PHYISCAL_NIC_RE for detecting physical network interfaces
across several places in our codebase. For now, nic has been added as
a valid prefix for NICs in pve-common, so we use that prefix here.

In order to support custom prefixes, we would have to remove every
place in the code relying on PHYISCAL_NIC_RE (at least), in order to
avoid breakage.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 PVE/CLI/Makefile  |   1 +
 PVE/CLI/pveeth.pm | 538 ++++++++++++++++++++++++++++++++++++++++++++++
 bin/Makefile      |   5 +
 bin/pveeth        |   8 +
 4 files changed, 552 insertions(+)
 create mode 100644 PVE/CLI/pveeth.pm
 create mode 100644 bin/pveeth

diff --git a/PVE/CLI/Makefile b/PVE/CLI/Makefile
index 9ff2aeb58..5bd1ef4cd 100644
--- a/PVE/CLI/Makefile
+++ b/PVE/CLI/Makefile
@@ -10,6 +10,7 @@ SOURCES = \
 	pvesh.pm \
 	pve7to8.pm \
 	pve8to9.pm \
+	pveeth.pm \
 
 all:
 
diff --git a/PVE/CLI/pveeth.pm b/PVE/CLI/pveeth.pm
new file mode 100644
index 000000000..7244f4689
--- /dev/null
+++ b/PVE/CLI/pveeth.pm
@@ -0,0 +1,538 @@
+package PVE::CLI::pveeth;
+
+use strict;
+use warnings;
+
+use File::Copy;
+use POSIX qw(:errno_h);
+use Storable qw(dclone);
+
+use PVE::Firewall;
+use PVE::INotify;
+use PVE::Network;
+use PVE::Network::SDN;
+use PVE::Network::SDN::Controllers;
+use PVE::RPCEnvironment;
+use PVE::SectionConfig;
+use PVE::Tools;
+
+use PVE::CLIHandler;
+use base qw(PVE::CLIHandler);
+
+my $PVEETH_LOCK = "/run/lock/pveeth.lck";
+
+sub setup_environment {
+    PVE::RPCEnvironment->setup_default_cli_env();
+}
+
+my sub update_sdn_controllers {
+    my ($mapping, $dry_run) = @_;
+
+    print "Updating controllers.cfg\n";
+
+    my $code = sub {
+        my $controllers = PVE::Network::SDN::Controllers::config();
+
+        my $local_node = PVE::INotify::nodename();
+
+        for my $controller (values $controllers->{ids}->%*) {
+            next
+                if $local_node ne $controller->{node}
+                || $controller->{type} ne 'isis';
+
+            $controller->{'isis-ifaces'} = $mapping->list($controller->{'isis-ifaces'});
+        }
+
+        if ($dry_run) {
+            my $raw =
+                PVE::Network::SDN::Controllers::Plugin->write_config(undef, $controllers, 0);
+            PVE::Tools::file_set_contents('generated_controllers.cfg', $raw);
+        } else {
+            copy("/etc/pve/sdn/controllers.cfg", "/etc/pve/sdn/controllers.cfg.bak");
+            PVE::Network::SDN::Controllers::write_config($controllers);
+        }
+    };
+
+    if ($dry_run) {
+        $code->();
+    } else {
+        PVE::Network::SDN::lock_sdn_config($code);
+    }
+}
+
+my sub update_etc_network_interfaces {
+    my ($mapping, $dry_run) = @_;
+
+    print "Updating interfaces\n";
+
+    my $code = sub {
+        my $config = dclone(PVE::INotify::read_file('interfaces'));
+
+        my $old_ifaces = $config->{ifaces};
+        my $new_ifaces = {};
+
+        for my $iface_name (keys $old_ifaces->%*) {
+            my $iface = $old_ifaces->{$iface_name};
+
+            if ($iface->{type} =~ m/^(eth|OVSPort|alias)$/) {
+                $iface_name = $mapping->name($iface_name);
+            } elsif ($iface->{type} eq 'vlan') {
+                $iface_name = $mapping->name($iface_name);
+                $iface->{'vlan-raw-device'} = $mapping->name($iface->{'vlan-raw-device'});
+            } elsif ($iface->{type} eq 'bond') {
+                $iface->{'bond-primary'} = $mapping->name($iface->{'bond-primary'});
+                $iface->{slaves} = $mapping->list($iface->{slaves});
+            } elsif ($iface->{type} eq 'bridge') {
+                $iface->{bridge_ports} = $mapping->list($iface->{bridge_ports});
+            } elsif ($iface->{type} eq 'OVSBridge') {
+                $iface->{ovs_ports} = $mapping->list($iface->{ovs_ports});
+            } elsif ($iface->{type} eq 'OVSBond') {
+                $iface->{ovs_bonds} = $mapping->list($iface->{ovs_bonds});
+            }
+
+            $new_ifaces->{$iface_name} = $iface;
+        }
+
+        $config->{ifaces} = $new_ifaces;
+
+        if ($dry_run) {
+            my $raw = PVE::INotify::__write_etc_network_interfaces($config, 1);
+            PVE::Tools::file_set_contents('generated_interfaces', $raw);
+        } else {
+            copy("/etc/network/interfaces", "/etc/network/interfaces.bak");
+            PVE::INotify::write_file('interfaces', $config, 1);
+
+            my $current_config_file = "/etc/network/interfaces";
+            my $new_config_file = "/etc/network/interfaces.new";
+
+            rename($new_config_file, $current_config_file);
+        }
+    };
+
+    if ($dry_run) {
+        $code->();
+    } else {
+        PVE::Tools::lock_file("/etc/network/.pve-interfaces.lock", 10, $code);
+        die $@ if $@;
+    }
+}
+
+my sub update_host_fw_config {
+    my ($mapping, $dry_run) = @_;
+
+    print "Updating host firewall config\n";
+
+    my $code = sub {
+        my $cluster_conf = PVE::Firewall::load_clusterfw_conf();
+        my $host_conf = PVE::Firewall::load_hostfw_conf($cluster_conf);
+
+        for my $rule ($cluster_conf->{rules}->@*) {
+            next if !$rule->{iface};
+
+            warn "found reference to iface $rule->{iface} in cluster config - not updating."
+                if $mapping->{ $rule->{iface} };
+        }
+
+        for my $rule ($host_conf->{rules}->@*) {
+            next if !$rule->{iface};
+            $rule->{iface} = $mapping->name($rule->{iface});
+        }
+
+        my $filename;
+        if ($dry_run) {
+            $filename = "generated_host.fw";
+        } else {
+            my $local_node = PVE::INotify::nodename();
+            copy("/etc/pve/nodes/$local_node/host.fw",
+                "/etc/pve/nodes/$local_node/host.fw.bak");
+        }
+
+        PVE::Firewall::save_hostfw_conf($host_conf, $filename);
+    };
+
+    if ($dry_run) {
+        $code->();
+    } else {
+        PVE::Firewall::run_locked($code);
+    }
+}
+
+my sub parse_link_file {
+    my ($file_name) = @_;
+
+    my $content = PVE::Tools::file_get_contents($file_name);
+    my @lines = split(/\n/, $content);
+
+    my $section;
+    my $data = {};
+
+    for my $line (@lines) {
+        next if $line =~ m/^\s*$/;
+
+        if ($line =~ m/^\[(Match|Link)\]$/) {
+            $section = $1;
+            $data->{$section} = {};
+        } elsif ($line =~ m/^([a-zA-Z]+)=(.+)$/) {
+            die "key-value pair before section" if !$section;
+            $data->{$section}->{$1} = $2;
+        } else {
+            die "unrecognized line";
+        }
+    }
+
+    return $data;
+}
+
+my $LINK_DIRECTORY_DRY_RUN = './network/';
+my $LINK_DIRECTORY = "/usr/local/lib/systemd/network/";
+
+my sub get_pinned {
+    mkdir '/usr/local/lib/systemd' if !-d '/usr/local/lib/systemd';
+    mkdir $LINK_DIRECTORY if !-d $LINK_DIRECTORY;
+
+    my $link_files = {};
+
+    PVE::Tools::dir_glob_foreach(
+        $LINK_DIRECTORY,
+        qr/^50-pve-(.+)\.link$/,
+        sub {
+            my $parsed = parse_link_file($LINK_DIRECTORY . $_[0]);
+            $link_files->{ $parsed->{'Match'}->{'MACAddress'} } = $parsed->{'Link'}->{'Name'};
+        },
+    );
+
+    return $link_files;
+}
+
+my $LINK_FILE_TEMPLATE = <<EOF;
+[Match]
+MACAddress=%s
+Type=ether
+
+[Link]
+Name=%s
+EOF
+
+my sub link_file_name {
+    my ($iface_name) = @_;
+    return "50-pve-$iface_name.link";
+}
+
+my sub delete_link_files {
+    my ($pinned) = @_;
+
+    for my $iface_name (values %$pinned) {
+        my $link_file = $LINK_DIRECTORY . link_file_name($iface_name);
+
+        if (!unlink $link_file) {
+            return if $! == ENOENT;
+            warn "failed to delete $link_file";
+        }
+    }
+}
+
+my sub generate_link_files {
+    my ($ip_links, $mapping, $params) = @_;
+
+    print "Generating link files\n";
+
+    my $directory = ($params->{dry_run}) ? $LINK_DIRECTORY_DRY_RUN : $LINK_DIRECTORY;
+    mkdir $directory if !-d $directory;
+
+    for my $ip_link (values $ip_links->%*) {
+        my $mapped_name = $mapping->name($ip_link->{ifname});
+        my $link_file_content =
+            sprintf($LINK_FILE_TEMPLATE, get_ip_link_mac($ip_link), $mapped_name);
+
+        PVE::Tools::file_set_contents(
+            $directory . link_file_name($mapped_name),
+            $link_file_content,
+        );
+    }
+}
+
+package PVE::CLI::pveeth::InterfaceMapping {
+    use PVE::CLI::pveeth;
+    use PVE::Tools;
+
+    sub inverted {
+        my ($class, $ip_links, $pinned) = @_;
+
+        my $mapping = {};
+
+    OUTER:
+        for my $pinned_mac (keys $pinned->%*) {
+            my $pinned_name = $pinned->{$pinned_mac};
+            my $old_name;
+
+            if ($ip_links->{$pinned_name}) {
+                # the pinned names are already applied, get path name from udev
+                $old_name = PVE::CLI::pveeth::get_udevadm_path($pinned_name);
+                $mapping->{$pinned_name} = $old_name;
+            } else {
+                # the pinned names have not yet been applied, search for
+                # interface with the same MAC address
+                for my $iface_name (keys $ip_links->%*) {
+                    my $ip_link = $ip_links->{$iface_name};
+                    my $ip_link_mac = PVE::CLI::pveeth::get_ip_link_mac($ip_link);
+
+                    if ($ip_link_mac eq $pinned_mac) {
+                        $mapping->{$pinned_name} = $iface_name;
+                        next OUTER;
+                    }
+                }
+
+                warn "could not find link for mapped interface $pinned_name, not removing";
+            }
+        }
+
+        bless $mapping, $class;
+    }
+
+    sub generate {
+        my ($class, $ip_links, $pinned, $prefix) = @_;
+
+        my %existing_names = map { $_ => 1 } values $pinned->%*;
+
+        my $index = 0;
+        my $mapping = {};
+
+        for my $ifname (sort keys $ip_links->%*) {
+            my $ip_link = $ip_links->{$ifname};
+            my $generated_name;
+
+            do {
+                $generated_name = $prefix . $index++;
+            } while ($existing_names{$generated_name});
+
+            $mapping->{$ifname} = $generated_name;
+
+            for my $altname ($ip_link->{altnames}->@*) {
+                $mapping->{$altname} = $generated_name;
+            }
+        }
+
+        bless $mapping, $class;
+    }
+
+    sub name {
+        my ($self, $iface_name) = @_;
+
+        if ($iface_name =~ m/^([a-zA-Z0-9_]+)([:\.]\d+)$/) {
+            my $mapped_name = $self->{$1} // $1;
+            my $suffix = $2;
+
+            return "$mapped_name$suffix";
+        }
+
+        return $self->{$iface_name} // $iface_name;
+    }
+
+    sub list {
+        my ($self, $list) = @_;
+
+        my @mapped_list = map { $self->name($_) } PVE::Tools::split_list($list);
+        return join(' ', @mapped_list);
+    }
+}
+
+sub get_ip_link_mac {
+    my ($ip_link) = @_;
+
+    # members of bonds can have a different MAC than the physical interface, so
+    # we need to check if they're enslaved
+    return $ip_link->{link_info}->{info_slave_data}->{perm_hwaddr} // $ip_link->{address};
+}
+
+sub get_ip_links {
+    my $ip_links = PVE::Network::ip_link_details();
+
+    for my $iface_name (keys $ip_links->%*) {
+        delete $ip_links->{$iface_name}
+            if !PVE::Network::ip_link_is_physical($ip_links->{$iface_name});
+    }
+
+    return $ip_links;
+}
+
+sub get_udevadm_path {
+    my ($iface_name) = @_;
+
+    my $path;
+
+    PVE::Tools::run_command(
+        ['udevadm', 'test-builtin', 'net_id', "/sys/class/net/$iface_name"],
+        outfunc => sub {
+            my $line = shift;
+
+            if ($line =~ m/^ID_NET_NAME_PATH=(.+)$/) {
+                $path = $1;
+            }
+        },
+        errfunc => sub { },
+    );
+
+    die "could not obtain path name for interface $iface_name" if !$path;
+    return $path;
+}
+
+__PACKAGE__->register_method({
+    name => 'pin',
+    path => 'pin',
+    method => 'POST',
+    description => 'Pins the names of NICs via systemd.link files.',
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            force => {
+                description =>
+                    'If true, deletes all existing mappings before generating a new one. Otherwise it only generates mappings for NICs that do not have one yet.',
+                type => 'boolean',
+                optional => 0,
+                default => 0,
+            },
+            nic => {
+                description => 'Only pin a specific NIC.',
+                type => 'string',
+                format => 'pve-iface',
+                optional => 1,
+            },
+            dry_run => {
+                description =>
+                    'Generates the configuration files in the current directory, instead of writing them.',
+                type => 'boolean',
+                optional => 1,
+                default => 1,
+            },
+        },
+    },
+    returns => {
+        type => 'null',
+    },
+    code => sub {
+        my ($params) = @_;
+
+        my $code = sub {
+            my $prefix = 'nic';
+
+            my $ip_links = get_ip_links();
+
+            if ($params->{nic}) {
+                my $altnames = PVE::Network::altname_mapping($ip_links);
+                my $resolved_name = $altnames->{ $params->{nic} } // $params->{nic};
+
+                die "could not find nic with name $params->{nic}"
+                    if !$ip_links->{$resolved_name};
+                $ip_links = { $resolved_name => $ip_links->{$resolved_name} };
+            }
+
+            my $pinned;
+            if (!$params->{force}) {
+                $pinned = get_pinned();
+
+                for my $ip_link (values $ip_links->%*) {
+                    delete $ip_links->{ $ip_link->{ifname} }
+                        if $pinned->{ get_ip_link_mac($ip_link) };
+                }
+            } else {
+                delete_link_files($pinned) if !$params->{dry_run};
+                $pinned = {};
+            }
+
+            my $mapping = PVE::CLI::pveeth::InterfaceMapping->generate(
+                $ip_links, $pinned, $prefix,
+            );
+
+            for my $old_name (keys $mapping->%*) {
+                print "Mapping '$old_name' to '$mapping->{$old_name}'\n";
+            }
+
+            generate_link_files($ip_links, $mapping, $params);
+
+            update_host_fw_config($mapping, $params->{dry_run});
+            update_etc_network_interfaces($mapping, $params->{dry_run});
+            update_sdn_controllers($mapping, $params->{dry_run});
+        };
+
+        if ($params->{dry_run}) {
+            $code->();
+        } else {
+            PVE::Tools::lock_file($PVEETH_LOCK, 10, $code);
+            die $@ if $@;
+        }
+
+        return;
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'unpin',
+    path => 'unpin',
+    method => 'POST',
+    description => 'Unpins the names of NICs that have been previously pinned',
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            dry_run => {
+                description =>
+                    'Generates the configuration files in the current directory, instead of writing them.',
+                type => 'boolean',
+                optional => 1,
+                default => 1,
+            },
+            nic => {
+                description => 'Only unpin a specific NIC',
+                type => 'string',
+                format => 'pve-iface',
+                optional => 1,
+            },
+        },
+    },
+    returns => {
+        type => 'null',
+    },
+    code => sub {
+        my ($params) = @_;
+
+        my $code = sub {
+            my $pinned = get_pinned();
+            my $ip_links = get_ip_links();
+
+            my $mapping = PVE::CLI::pveeth::InterfaceMapping->inverted(
+                $ip_links, $pinned,
+            );
+
+            if ($params->{nic}) {
+                for my $mapped_name (keys $mapping->%*) {
+                    delete $mapping->{$mapped_name} if $mapped_name ne $params->{nic};
+                }
+            }
+
+            for my $old_name (keys $mapping->%*) {
+                print "Mapping '$old_name' to '$mapping->{$old_name}'\n";
+            }
+
+            delete_link_files($pinned) if !$params->{dry_run};
+
+            update_host_fw_config($mapping, $params->{dry_run});
+            update_etc_network_interfaces($mapping, $params->{dry_run});
+            update_sdn_controllers($mapping, $params->{dry_run});
+        };
+
+        if ($params->{dry_run}) {
+            $code->();
+        } else {
+            PVE::Tools::lock_file($PVEETH_LOCK, 10, $code);
+            die $@ if $@;
+        }
+
+        return;
+    },
+});
+
+our $cmddef = {
+    pin => [__PACKAGE__, 'pin', [], {}],
+    unpin => [__PACKAGE__, 'unpin', [], {}],
+};
+
+1;
diff --git a/bin/Makefile b/bin/Makefile
index 3931804b1..2d7e60677 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -14,6 +14,7 @@ CLITOOLS = \
 	pvesh \
 	pve7to8 \
 	pve8to9 \
+	pveeth \
 
 
 SCRIPTS =  			\
@@ -67,6 +68,10 @@ pve7to8.1:
 	printf ".SH SYNOPSIS\npve7to8 [--full]\n" >> $@.tmp
 	mv $@.tmp $@
 
+pveeth.1:
+	printf "pveeth" > $@.tmp
+	mv $@.tmp $@
+
 pve8to9.1:
 	printf ".TH PVE8TO9 1\n.SH NAME\npve8to9 \- Proxmox VE upgrade checker script for 8.4+ to current 9.x\n" > $@.tmp
 	printf ".SH DESCRIPTION\nThis tool will help you to detect common pitfalls and misconfguration\
diff --git a/bin/pveeth b/bin/pveeth
new file mode 100644
index 000000000..bf978a59a
--- /dev/null
+++ b/bin/pveeth
@@ -0,0 +1,8 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use PVE::CLI::pveeth;
+
+PVE::CLI::pveeth->run_cli_handler();
-- 
2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Gabriel Goller]

On 09.07.2025 21:45, Stefan Hanreich wrote:
>> [snip]
>Example invocations of pveeth:
>
>$ pveeth pin --nic enp1s0 --force 0 --dry_run 0
>
>Generates a pinning for enp1s0 (if it doesn't exist already) and
>updates the configuration file.
>
>$ pveeth pin --force 1 --dry_run 0

s/pin/unpin

>Deletes all existing pins and re-generates them.
>
>For more information on the parameters see the API description.
>
>I've decided to let dry_run generate the configuration files in the
>current working directory, since it is then easy to diff the generated
>files with the existing configuration files using the diffviewer of
>the users' choice.
>
>Additionally, when writing the configuration files, they get backed up
>by creating a .bak at the location of the configuration file.
>
>Currently we only support a fixed prefix: 'nic'. This is because we
>rely on PHYISCAL_NIC_RE for detecting physical network interfaces
>across several places in our codebase. For now, nic has been added as
>a valid prefix for NICs in pve-common, so we use that prefix here.

This would be nice to have as a comment in the code, I can already see
people that want to change the prefix and start editing the constant :)

>In order to support custom prefixes, we would have to remove every
>place in the code relying on PHYISCAL_NIC_RE (at least), in order to
>avoid breakage.

When pinning, could we add the previous old name as an altname? So tools
that respect altnames could work transparently.

This should be as simple as adding, e.g.:

     AlternativeName=ens18

to the link file.

However, unpinning has a problem. Currently, we reset to the
`ID_NET_NAME_PATH` name from `udevadm` and remove the link file, but we
don't know the current systemd `NamePolicy`.

For example, if the original interface was `ens18` using Slot policy,
after pinning it becomes `nic0`. When unpinning, the interface becomes
`ens18` again (because I'm using slot policy), but the config files
reference `enp0s18` (because `pveeth` assumes we use path policy). This
breaks the network configuration.

I think the solution is to avoid deleting the link file during
unpinning. Instead, we should just update the interface name.

The problem is that we can't determine which policy was originally used,
so pin-unpin cycles *don't* restore the original interface name.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Thomas Lamprecht]

Am 10.07.25 um 16:53 schrieb Gabriel Goller:
> On 09.07.2025 21:45, Stefan Hanreich wrote:
>>> [snip]
>> Example invocations of pveeth:
>>
>> $ pveeth pin --nic enp1s0 --force 0 --dry_run 0
>>
>> Generates a pinning for enp1s0 (if it doesn't exist already) and
>> updates the configuration file.
>>
>> $ pveeth pin --force 1 --dry_run 0
> 
> s/pin/unpin
> 
>> Deletes all existing pins and re-generates them.
>>
>> For more information on the parameters see the API description.
>>
>> I've decided to let dry_run generate the configuration files in the
>> current working directory, since it is then easy to diff the generated
>> files with the existing configuration files using the diffviewer of
>> the users' choice.
>>
>> Additionally, when writing the configuration files, they get backed up
>> by creating a .bak at the location of the configuration file.
>>
>> Currently we only support a fixed prefix: 'nic'. This is because we
>> rely on PHYISCAL_NIC_RE for detecting physical network interfaces
>> across several places in our codebase. For now, nic has been added as
>> a valid prefix for NICs in pve-common, so we use that prefix here.
> 
> This would be nice to have as a comment in the code, I can already see
> people that want to change the prefix and start editing the constant :)
> 
>> In order to support custom prefixes, we would have to remove every
>> place in the code relying on PHYISCAL_NIC_RE (at least), in order to
>> avoid breakage.
> 
> When pinning, could we add the previous old name as an altname? So tools
> that respect altnames could work transparently.
> 
> This should be as simple as adding, e.g.:
> 
>      AlternativeName=ens18
> 
> to the link file.

This can interfere with automatic naming of unpinned/new interfaces and can
get really confusing if the automatic derived name changes due to replugging
the NIC to another slot and then the pinned alt name is wrong, as it suggest
that the NIC is in a different slot. Or if the layout changes otherwise and
another NIC would get this alt name, so there might be different NICs then
with very similar names.

> 
> However, unpinning has a problem. Currently, we reset to the
> `ID_NET_NAME_PATH` name from `udevadm` and remove the link file, but we
> don't know the current systemd `NamePolicy`.
> 
> For example, if the original interface was `ens18` using Slot policy,
> after pinning it becomes `nic0`. When unpinning, the interface becomes
> `ens18` again (because I'm using slot policy), but the config files
> reference `enp0s18` (because `pveeth` assumes we use path policy). This
> breaks the network configuration.
> 
> I think the solution is to avoid deleting the link file during
> unpinning. Instead, we should just update the interface name.
> 
> The problem is that we can't determine which policy was originally used,
> so pin-unpin cycles *don't* restore the original interface name.
> 

This I'd need to think through, just wanted to comment on above before
I forget.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Gabriel Goller]

On 10.07.2025 17:08, Thomas Lamprecht wrote:
>Am 10.07.25 um 16:53 schrieb Gabriel Goller:
>> On 09.07.2025 21:45, Stefan Hanreich wrote:
>>>> [snip]
>>> Example invocations of pveeth:
>>>
>>> $ pveeth pin --nic enp1s0 --force 0 --dry_run 0
>>>
>>> Generates a pinning for enp1s0 (if it doesn't exist already) and
>>> updates the configuration file.
>>>
>>> $ pveeth pin --force 1 --dry_run 0
>>
>> s/pin/unpin
>>
>>> Deletes all existing pins and re-generates them.
>>>
>>> For more information on the parameters see the API description.
>>>
>>> I've decided to let dry_run generate the configuration files in the
>>> current working directory, since it is then easy to diff the generated
>>> files with the existing configuration files using the diffviewer of
>>> the users' choice.
>>>
>>> Additionally, when writing the configuration files, they get backed up
>>> by creating a .bak at the location of the configuration file.
>>>
>>> Currently we only support a fixed prefix: 'nic'. This is because we
>>> rely on PHYISCAL_NIC_RE for detecting physical network interfaces
>>> across several places in our codebase. For now, nic has been added as
>>> a valid prefix for NICs in pve-common, so we use that prefix here.
>>
>> This would be nice to have as a comment in the code, I can already see
>> people that want to change the prefix and start editing the constant :)
>>
>>> In order to support custom prefixes, we would have to remove every
>>> place in the code relying on PHYISCAL_NIC_RE (at least), in order to
>>> avoid breakage.
>>
>> When pinning, could we add the previous old name as an altname? So tools
>> that respect altnames could work transparently.
>>
>> This should be as simple as adding, e.g.:
>>
>>      AlternativeName=ens18
>>
>> to the link file.
>
>This can interfere with automatic naming of unpinned/new interfaces and can
>get really confusing if the automatic derived name changes due to replugging
>the NIC to another slot and then the pinned alt name is wrong, as it suggest
>that the NIC is in a different slot. Or if the layout changes otherwise and
>another NIC would get this alt name, so there might be different NICs then
>with very similar names.

Ah true, didn't think of this. Yeah this would create a mess, nevermind.

>>
>> However, unpinning has a problem. Currently, we reset to the
>> `ID_NET_NAME_PATH` name from `udevadm` and remove the link file, but we
>> don't know the current systemd `NamePolicy`.
>>
>> For example, if the original interface was `ens18` using Slot policy,
>> after pinning it becomes `nic0`. When unpinning, the interface becomes
>> `ens18` again (because I'm using slot policy), but the config files
>> reference `enp0s18` (because `pveeth` assumes we use path policy). This
>> breaks the network configuration.
>>
>> I think the solution is to avoid deleting the link file during
>> unpinning. Instead, we should just update the interface name.
>>
>> The problem is that we can't determine which policy was originally used,
>> so pin-unpin cycles *don't* restore the original interface name.
>>
>
>This I'd need to think through, just wanted to comment on above before
>I forget.

If we really want to make pin and unpin involutive, we would need to
store somewhere the interface names or store the interface naming
policy.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Stefan Hanreich]

On 7/10/25 18:25, Gabriel Goller wrote:
>>> However, unpinning has a problem. Currently, we reset to the
>>> `ID_NET_NAME_PATH` name from `udevadm` and remove the link file, but we
>>> don't know the current systemd `NamePolicy`.

Not sure if users overriding the default naming policy of network
interfaces is a common enough use case to consider here. In that case
users will have to take care of updating / changing the network
interface names themselves anyway, since they would change with that
setting as well.


>>> For example, if the original interface was `ens18` using Slot policy,
>>> after pinning it becomes `nic0`. When unpinning, the interface becomes
>>> `ens18` again (because I'm using slot policy), but the config files
>>> reference `enp0s18` (because `pveeth` assumes we use path policy). This
>>> breaks the network configuration.
>>>
>>> I think the solution is to avoid deleting the link file during
>>> unpinning. Instead, we should just update the interface name.
>>>
>>> The problem is that we can't determine which policy was originally used,
>>> so pin-unpin cycles *don't* restore the original interface name.

But then we'd have changed the name that we're pinning, not removed the
pinning altogether.

Unpin is trying to revert to the name that would be automatically
generated without any link files present, not the exact name that the
NIC had when we generated the pinning. I can see how this might be
obtuse though, particularly since I write that pin / unpin should be
idempotent (which they aren't, because of the behavior I described
above) - my mistake.

With storing names there's still the possibility of the stored name
clashing with another interface in the meanwhile (due to e.g. upgrades).
We cannot just store the old names and reset to them. Even with the same
naming policy, names can change across updates (which is what happens
with the upgrade to systemd v257, for instance), so we have to query how
the interface name would look like with the *current* settings, not with
the settings at the time of pinning. This also applies to storing the
policy, who says that the policy hasn't changed since we stored it?


>> This I'd need to think through, just wanted to comment on above before
>> I forget.
> 
> If we really want to make pin and unpin involutive, we would need to
> store somewhere the interface names or store the interface naming
> policy.

unpin was more intended as a solution for users that made an error with
invoking the pin command and give them an easy way to revert the changes
generated by pin. In the other thread with Dominik I've also discussed a
different approach on how to handle applying the configuration. Solving
it as follows would also introduce a way of reverting the configuration:

* Pinning generates the new configuration files in the pending config of
/e/n/i and SDN. For the firewall we'd have to create one as well and
probably just handle this manually in the following step.

* Add another command that applies the temporary changes which would
also include applying the changes via udevadm immediately.

If we solve it like this, then we could introduce a 'revert' or
'rollback' command, which would simply delete any pending changes and
then remove the generated link files. We'd have three possible actions
for handling pending configuration files:

* generate (generates the pending configuration)
* apply (which applies pending configuration)
* revert/rollback (which removes any pending configuration changes)

This would reset everything to the way it was before generating the
pending configuration. It would also obsolete a dry-run flag imo, since
we have the intermediate, pending, configuration that needs to be
manually applied. Users can use those for inspecting the potential changes.

It would still make sense to provide the opportunity for users to get
rid of all pinned names, which unpin in its current state could then do.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Stefan Hanreich]

On 7/15/25 14:30, Stefan Hanreich wrote:
> On 7/10/25 18:25, Gabriel Goller wrote:
>>>> However, unpinning has a problem. Currently, we reset to the
>>>> `ID_NET_NAME_PATH` name from `udevadm` and remove the link file, but we
>>>> don't know the current systemd `NamePolicy`.
> 
> Not sure if users overriding the default naming policy of network
> interfaces is a common enough use case to consider here. In that case
> users will have to take care of updating / changing the network
> interface names themselves anyway, since they would change with that
> setting as well.
> 
> 
>>>> For example, if the original interface was `ens18` using Slot policy,
>>>> after pinning it becomes `nic0`. When unpinning, the interface becomes
>>>> `ens18` again (because I'm using slot policy), but the config files
>>>> reference `enp0s18` (because `pveeth` assumes we use path policy). This
>>>> breaks the network configuration.
>>>>
>>>> I think the solution is to avoid deleting the link file during
>>>> unpinning. Instead, we should just update the interface name.
>>>>
>>>> The problem is that we can't determine which policy was originally used,
>>>> so pin-unpin cycles *don't* restore the original interface name.
> 
> But then we'd have changed the name that we're pinning, not removed the
> pinning altogether.
> 
> Unpin is trying to revert to the name that would be automatically
> generated without any link files present, not the exact name that the
> NIC had when we generated the pinning. I can see how this might be
> obtuse though, particularly since I write that pin / unpin should be

-obtuse +obscure

> idempotent (which they aren't, because of the behavior I described
> above) - my mistake.
> 
> With storing names there's still the possibility of the stored name
> clashing with another interface in the meanwhile (due to e.g. upgrades).
> We cannot just store the old names and reset to them. Even with the same
> naming policy, names can change across updates (which is what happens
> with the upgrade to systemd v257, for instance), so we have to query how
> the interface name would look like with the *current* settings, not with
> the settings at the time of pinning. This also applies to storing the
> policy, who says that the policy hasn't changed since we stored it?
> 
> 
>>> This I'd need to think through, just wanted to comment on above before
>>> I forget.
>>
>> If we really want to make pin and unpin involutive, we would need to
>> store somewhere the interface names or store the interface naming
>> policy.
> 
> unpin was more intended as a solution for users that made an error with
> invoking the pin command and give them an easy way to revert the changes
> generated by pin. In the other thread with Dominik I've also discussed a
> different approach on how to handle applying the configuration. Solving
> it as follows would also introduce a way of reverting the configuration:
> 
> * Pinning generates the new configuration files in the pending config of
> /e/n/i and SDN. For the firewall we'd have to create one as well and
> probably just handle this manually in the following step.
> 
> * Add another command that applies the temporary changes which would
> also include applying the changes via udevadm immediately.
> 
> If we solve it like this, then we could introduce a 'revert' or
> 'rollback' command, which would simply delete any pending changes and
> then remove the generated link files. We'd have three possible actions
> for handling pending configuration files:
> 
> * generate (generates the pending configuration)
> * apply (which applies pending configuration)
> * revert/rollback (which removes any pending configuration changes)
> 
> This would reset everything to the way it was before generating the
> pending configuration. It would also obsolete a dry-run flag imo, since
> we have the intermediate, pending, configuration that needs to be
> manually applied. Users can use those for inspecting the potential changes.
> 
> It would still make sense to provide the opportunity for users to get
> rid of all pinned names, which unpin in its current state could then do.
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Thomas Lamprecht]

Am 15.07.25 um 14:30 schrieb Stefan Hanreich:
>>> This I'd need to think through, just wanted to comment on above before
>>> I forget.
>>
>> If we really want to make pin and unpin involutive, we would need to
>> store somewhere the interface names or store the interface naming
>> policy.
> 
> unpin was more intended as a solution for users that made an error with
> invoking the pin command and give them an easy way to revert the changes
> generated by pin. In the other thread with Dominik I've also discussed a
> different approach on how to handle applying the configuration. Solving
> it as follows would also introduce a way of reverting the configuration:
> 
> * Pinning generates the new configuration files in the pending config of
> /e/n/i and SDN. For the firewall we'd have to create one as well and
> probably just handle this manually in the following step.
> * Add another command that applies the temporary changes which would
> also include applying the changes via udevadm immediately.
> 
> If we solve it like this, then we could introduce a 'revert' or
> 'rollback' command, which would simply delete any pending changes and
> then remove the generated link files. We'd have three possible actions
> for handling pending configuration files:
> 
> * generate (generates the pending configuration)
> * apply (which applies pending configuration)
> * revert/rollback (which removes any pending configuration changes)
> 
> This would reset everything to the way it was before generating the
> pending configuration. It would also obsolete a dry-run flag imo, since
> we have the intermediate, pending, configuration that needs to be
> manually applied. Users can use those for inspecting the potential changes.
> 
> It would still make sense to provide the opportunity for users to get
> rid of all pinned names, which unpin in its current state could then do.
That would be certainly nice to have for admins, but it would also
be nice if firewall stack can still transparently cope with the altnames.

btw. a reboot in the "pinned but not applied" state might need some
special handling to, because IIRC it would now apply the /e/n/i changes
but not the firewall rules.
We could add the handling for the node firewall rules in the apply /e/n/i
endpoint, as then it might work automagically?


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Stefan Hanreich]

On 7/15/25 15:51, Thomas Lamprecht wrote:
> Am 15.07.25 um 14:30 schrieb Stefan Hanreich:
>>>> This I'd need to think through, just wanted to comment on above before
>>>> I forget.
>>>
>>> If we really want to make pin and unpin involutive, we would need to
>>> store somewhere the interface names or store the interface naming
>>> policy.
>>
>> unpin was more intended as a solution for users that made an error with
>> invoking the pin command and give them an easy way to revert the changes
>> generated by pin. In the other thread with Dominik I've also discussed a
>> different approach on how to handle applying the configuration. Solving
>> it as follows would also introduce a way of reverting the configuration:
>>
>> * Pinning generates the new configuration files in the pending config of
>> /e/n/i and SDN. For the firewall we'd have to create one as well and
>> probably just handle this manually in the following step.
>> * Add another command that applies the temporary changes which would
>> also include applying the changes via udevadm immediately.
>>
>> If we solve it like this, then we could introduce a 'revert' or
>> 'rollback' command, which would simply delete any pending changes and
>> then remove the generated link files. We'd have three possible actions
>> for handling pending configuration files:
>>
>> * generate (generates the pending configuration)
>> * apply (which applies pending configuration)
>> * revert/rollback (which removes any pending configuration changes)
>>
>> This would reset everything to the way it was before generating the
>> pending configuration. It would also obsolete a dry-run flag imo, since
>> we have the intermediate, pending, configuration that needs to be
>> manually applied. Users can use those for inspecting the potential changes.
>>
>> It would still make sense to provide the opportunity for users to get
>> rid of all pinned names, which unpin in its current state could then do.
> That would be certainly nice to have for admins, but it would also
> be nice if firewall stack can still transparently cope with the altnames.

It can - with the patches included in this series.

> btw. a reboot in the "pinned but not applied" state might need some
> special handling to, because IIRC it would now apply the /e/n/i changes
> but not the firewall rules.
> We could add the handling for the node firewall rules in the apply /e/n/i
> endpoint, as then it might work automagically?

I can look into that, then this might be the best way forward, if I can
move it there easily.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-manager 1/1] cli: add pveeth

[Stefan Hanreich]

On 7/15/25 15:51, Thomas Lamprecht wrote:
> Am 15.07.25 um 14:30 schrieb Stefan Hanreich:
>>>> This I'd need to think through, just wanted to comment on above before
>>>> I forget.
>>>
>>> If we really want to make pin and unpin involutive, we would need to
>>> store somewhere the interface names or store the interface naming
>>> policy.
>>
>> unpin was more intended as a solution for users that made an error with
>> invoking the pin command and give them an easy way to revert the changes
>> generated by pin. In the other thread with Dominik I've also discussed a
>> different approach on how to handle applying the configuration. Solving
>> it as follows would also introduce a way of reverting the configuration:
>>
>> * Pinning generates the new configuration files in the pending config of
>> /e/n/i and SDN. For the firewall we'd have to create one as well and
>> probably just handle this manually in the following step.
>> * Add another command that applies the temporary changes which would
>> also include applying the changes via udevadm immediately.
>>
>> If we solve it like this, then we could introduce a 'revert' or
>> 'rollback' command, which would simply delete any pending changes and
>> then remove the generated link files. We'd have three possible actions
>> for handling pending configuration files:
>>
>> * generate (generates the pending configuration)
>> * apply (which applies pending configuration)
>> * revert/rollback (which removes any pending configuration changes)
>>
>> This would reset everything to the way it was before generating the
>> pending configuration. It would also obsolete a dry-run flag imo, since
>> we have the intermediate, pending, configuration that needs to be
>> manually applied. Users can use those for inspecting the potential changes.
>>
>> It would still make sense to provide the opportunity for users to get
>> rid of all pinned names, which unpin in its current state could then do.
> That would be certainly nice to have for admins, but it would also
> be nice if firewall stack can still transparently cope with the altnames.
> 
> btw. a reboot in the "pinned but not applied" state might need some
> special handling to, because IIRC it would now apply the /e/n/i changes
> but not the firewall rules.
> We could add the handling for the node firewall rules in the apply /e/n/i
> endpoint, as then it might work automagically?

That doesn't seem to work currently, since pvenetcommit [1] simply moves
/e/n/i.new to /e/n/i. The current dependency tree of that service also
would make changing the order quite awkward, if we simply wanted to
invoke the reload_network_configuration API endpoint.

We could implement a small perlscript, that gets executed by
pvenetcommit, which would invoke the reloading logic contained in the
apply endpoint as an alternate solution? This would then apply the SDN /
FRR config on restart as well. This would be required anyway though,
since we potentially change the SDN controller config as well.

[1]
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=services/pvenetcommit.service;h=19b2f1431d3810e4f64dc0f32fe0577e954e3e89;hb=HEAD



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] superseded: [RFC common/firewall/manager/network/proxmox{-ve-rs, -firewall} 0/7] NIC renaming mitigations

[Stefan Hanreich]

https://lore.proxmox.com/pve-devel/20250716151815.348161-1-s.hanreich@proxmox.com/

On 7/9/25 21:45, Stefan Hanreich wrote:
> This patch series contains the following features:
> * transparent altname support for {pve, proxmox}-firewall and pve-network
> * pveeth tool for pinning NIC names
> 
> Both are features aimed at mitigating the fallout caused from changing network
> interface names. Sending it as an RFC, since I will be gone for a few days and
> wanted to publish my current state to start some discussion on the approaches
> I've taken with the tools and possible additions / changes. Nothing in here is
> final or particularly polished.
> 
> Both patch series only received rudimentary testing and are work in progress, so
> use at your own risk, I am not responsible for any broken hosts / VMs.
> 
> For more information on the pveeth tool, see the respective commit.
> 
> TODO:
> * possibly change wakeonlan setting in node config
> * decide on how to handle host.fw / cluster.fw:
> 
> cluster.fw cannot be automatically updated, since the generated mapping might
> differ from the one generated on other nodes. One possibility would be to
> generate the mapping for the NICs one-by-one on each host, thus ensuring a
> consistent name on all nodes. Then add a flag that overwrites cluster.fw.
> 
> cluster/host.fw is the only configuration file that gets applied immediately
> when updating it, since the firewall continously polls this file and applies the
> settings. We could add the new name as altname via ip link, ensuring that the
> firewall rules still work before *and* after reboot. Shouldn't be too hard to
> add (possibly with a flag). This is possible because of the new altname support
> {pve, proxmox}-firewall.
> 
> * update detection of physical NICs
> 
> We currently rely on the PHYSICAL_NIC_RE to detect physical network interfaces.
> We could instead use the ip link output for determining whether an interface is
> physical or not. This works in every case, except for PullMetric.pm. For this we
> could introduce another variable and fall back on the old logic depending on its
> existence. Maybe some one with more knowledge on the metrics system can chime in
> here. I have patches for this on my staff repo in case you are interested:
> 
> pve-manager:physical-nic-re
> pve-common:physical-nic-re
> 
> pve-common:
> 
> Stefan Hanreich (2):
>   network: add ip link and altname helpers
>   network: add nic prefix to physical nic regex
> 
>  src/PVE/Network.pm | 47 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> 
> proxmox-ve-rs:
> 
> Stefan Hanreich (1):
>   config: ip link struct
> 
>  proxmox-ve-config/src/host/mod.rs     |  1 +
>  proxmox-ve-config/src/host/network.rs | 35 +++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>  create mode 100644 proxmox-ve-config/src/host/network.rs
> 
> 
> proxmox-firewall:
> 
> Stefan Hanreich (1):
>   firewall: add altname support for firewall rules
> 
>  proxmox-firewall/src/config.rs              | 29 +++++++++++++++++++++
>  proxmox-firewall/src/rule.rs                |  6 ++++-
>  proxmox-firewall/tests/integration_tests.rs |  7 +++++
>  3 files changed, 41 insertions(+), 1 deletion(-)
> 
> 
> pve-firewall:
> 
> Stefan Hanreich (1):
>   firewall: add altname support
> 
>  src/PVE/Firewall.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 
> pve-network:
> 
> Stefan Hanreich (1):
>   controllers: isis: add altname support
> 
>  src/PVE/Network/SDN/Controllers/IsisPlugin.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> 
> pve-manager:
> 
> Stefan Hanreich (1):
>   cli: add pveeth
> 
>  PVE/CLI/Makefile  |   1 +
>  PVE/CLI/pveeth.pm | 538 ++++++++++++++++++++++++++++++++++++++++++++++
>  bin/Makefile      |   5 +
>  bin/pveeth        |   8 +
>  4 files changed, 552 insertions(+)
>  create mode 100644 PVE/CLI/pveeth.pm
>  create mode 100644 bin/pveeth
> 
> 
> Summary over all repositories:
>   12 files changed, 684 insertions(+), 5 deletions(-)
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel