* Re: [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist}
2024-06-27 15:01 [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist} Stefan Hanreich
@ 2024-06-28 14:35 ` Friedrich Weber
2024-07-01 8:35 ` Fabian Grünbichler
2024-07-03 12:58 ` Stefan Hanreich
2 siblings, 0 replies; 4+ messages in thread
From: Friedrich Weber @ 2024-06-28 14:35 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hanreich
On 27/06/2024 17:01, Stefan Hanreich wrote:
> This can lead to issue when upgrading from ifupdown to ifupdown2. The
> particular issue this fixes occurs in the following scenario:
>
> * Suppose there is a legacy Debian host with ifupdown and ifenslave
> installed that has a bond configured in /etc/network/interfaces.
> * ifenslave installs a script /etc/network/if-pre-up.d/ifenslave.
> * Now, an upgrade creates a second script
> /etc/network/if-pre-up.d/ifenslave.dpkg-new. As ifupdown executes
> network scripts via run-parts which ignores scripts with . in their
> name, ifenslave.dpkg-new has no effect.
> * If the host switches over to ifupdown2 by installing it (removing
> ifupdown, keeping ifenslave) and reboots, the network will not come
> up:
> /etc/network/if-pre-up.d/ifenslave still exists, but is ignored
> by ifupdown2's bond addon [1]
> /etc/network/if-pre-up.d/ifenslave.dpkg-new is executed by ifupdown2
> because it executes all scripts in /etc/network/if-pre-up.d, even if
> their name contains a dot
>
> This leads to ifreload failing on upgrades, which in turn causes
> issues with the networking of upgraded hosts.
Thanks for tackling this! Consider this
Tested-by: Friedrich Weber <f.weber@proxmox.com>
I tested the following:
- Set up a PVE8 VM with an active-backup bond, from /etc/network/interfaces:
> auto bond0
> iface bond0 inet manual
> bond-slaves ens18 ens19
> bond-miimon 100
> bond-mode active-backup
>
> auto vmbr0
> iface vmbr0 inet static
> address 10.1.1.122/16
> gateway 10.1.1.1
> bridge-ports bond0
> bridge-stp off
> bridge-fd 0
- Install ifupdown and ifenslave
- Manually create a .dpkg-new:
> cp /etc/network/if-pre-up.d/ifenslave /etc/network/if-pre-up.d/ifenslave.dpkg-new
- Reboot -- network comes up fine.
- Install latest packaged ifupdown2:
> apt install ifupdown2=3.2.0-1+pmx8
- Network is already lost during install
- Reboot -- network does not come up, bond0 and vmbr0 report NO-CARRIER
- Install ifupdown2 with this patch applied
- Reboot -- network comes up fine
- Running `execsnoop -l network & sleep 3; ifreload -a` confirms
ifenslave.dpkg-new is never executed (ifenslave is never executed by
ifupdown2, see the pull request linked by Stefan)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist}
2024-06-27 15:01 [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist} Stefan Hanreich
2024-06-28 14:35 ` Friedrich Weber
@ 2024-07-01 8:35 ` Fabian Grünbichler
2024-07-03 12:58 ` Stefan Hanreich
2 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2024-07-01 8:35 UTC (permalink / raw)
To: Proxmox VE development discussion
small nit below, otherwise
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
On June 27, 2024 5:01 pm, Stefan Hanreich wrote:
> This can lead to issue when upgrading from ifupdown to ifupdown2. The
> particular issue this fixes occurs in the following scenario:
>
> * Suppose there is a legacy Debian host with ifupdown and ifenslave
> installed that has a bond configured in /etc/network/interfaces.
> * ifenslave installs a script /etc/network/if-pre-up.d/ifenslave.
> * Now, an upgrade creates a second script
> /etc/network/if-pre-up.d/ifenslave.dpkg-new. As ifupdown executes
> network scripts via run-parts which ignores scripts with . in their
> name, ifenslave.dpkg-new has no effect.
> * If the host switches over to ifupdown2 by installing it (removing
> ifupdown, keeping ifenslave) and reboots, the network will not come
> up:
> /etc/network/if-pre-up.d/ifenslave still exists, but is ignored
> by ifupdown2's bond addon [1]
> /etc/network/if-pre-up.d/ifenslave.dpkg-new is executed by ifupdown2
> because it executes all scripts in /etc/network/if-pre-up.d, even if
> their name contains a dot
>
> This leads to ifreload failing on upgrades, which in turn causes
> issues with the networking of upgraded hosts.
>
> Also submitted upstream at [2]
>
> [1] https://github.com/CumulusNetworks/ifupdown2/blob/ccdc386cfab70703b657fe7c0ffceb95448a9c2b/ifupdown2/addons/bond.py#L45
> [2] https://github.com/CumulusNetworks/ifupdown2/pull/304
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> ...dpkg-files-when-running-hook-scripts.patch | 54 +++++++++++++++++++
> debian/patches/series | 1 +
> 2 files changed, 55 insertions(+)
> create mode 100644 debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
>
> diff --git a/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch b/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> new file mode 100644
> index 0000000..eea615f
> --- /dev/null
> +++ b/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> @@ -0,0 +1,54 @@
> +From dbb759a1383cf736a0fa769c5c5827e1e7f8145c Mon Sep 17 00:00:00 2001
> +From: Stefan Hanreich <s.hanreich@proxmox.com>
> +Date: Tue, 4 Jun 2024 16:17:54 +0200
> +Subject: [PATCH] main: ignore dpkg files when running hook scripts
> +
> +Currently ifupdown2 executes scripts that are backed up by dpkg (e.g.
> +foo.dpkg-old). This can lead to issues with hook scripts getting
> +executed after upgrading ifupdown2 via dpkg, even though they should
> +not be executed.
> +
> +This also brings ifupdown2 closer on par with the behavior of
> +ifupdown, which did not execute hook scripts with dpkg suffixes.
some nits for phrasing here:
- this not only affects ifupdown2 upgrades - any package shipping an
ifupdown hook script can potentially trigger this
- the 'via dpkg' is redundant IMHO (package updates always happen via
apt and dpkg ;))
- 'closer on par' is a bit of an oxymoron, I'd just say 'closer to' and
maybe add that ifupdown used/uses run-parts, and doesn't execute
dpkg-created copies of scripts for that reason
> +
> +Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> +---
> + ifupdown2/ifupdown/ifupdownmain.py | 4 +++-
> + ifupdown2/ifupdown/utils.py | 6 ++++++
> + 2 files changed, 9 insertions(+), 1 deletion(-)
> +
> +diff --git a/ifupdown2/ifupdown/ifupdownmain.py b/ifupdown2/ifupdown/ifupdownmain.py
> +index 51f5460..e6622f0 100644
> +--- a/ifupdown2/ifupdown/ifupdownmain.py
> ++++ b/ifupdown2/ifupdown/ifupdownmain.py
> +@@ -1540,7 +1540,9 @@ class ifupdownMain:
> + try:
> + module_list = os.listdir(msubdir)
> + for module in module_list:
> +- if self.modules.get(module) or module in self.overridden_ifupdown_scripts:
> ++ if (self.modules.get(module)
> ++ or module in self.overridden_ifupdown_scripts
> ++ or utils.is_dpkg_file(module)):
> + continue
> + self.script_ops[op].append(msubdir + '/' + module)
> + except Exception:
> +diff --git a/ifupdown2/ifupdown/utils.py b/ifupdown2/ifupdown/utils.py
> +index 05c7e48..3085e82 100644
> +--- a/ifupdown2/ifupdown/utils.py
> ++++ b/ifupdown2/ifupdown/utils.py
> +@@ -212,6 +212,12 @@ class utils():
> + # what we have in the cache (data retrieved via a netlink dump by
> + # nlmanager). nlmanager return all macs in lower-case
> +
> ++ _dpkg_suffixes = (".dpkg-old", ".dpkg-dist", ".dpkg-new", ".dpkg-tmp")
> ++
> ++ @staticmethod
> ++ def is_dpkg_file(name):
> ++ return any(name.endswith(suffix) for suffix in utils._dpkg_suffixes)
> ++
> + @classmethod
> + def importName(cls, modulename, name):
> + """ Import a named object """
> +--
> +2.39.2
> +
> diff --git a/debian/patches/series b/debian/patches/series
> index 557aa7f..d5772c9 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -7,6 +7,7 @@ pve/0006-openvswitch-ovs-ports-condone-regex-exclude-tap-veth.patch
> pve/0007-allow-vlan-tag-inside-vxlan-tunnel.patch
> pve/0008-lacp-bond-remove-bond-min-links-0-warning.patch
> pve/0009-gvgeb-fix-python-interpreter-shebang.patch
> +pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> upstream/0001-add-ipv6-slaac-support-inet6-auto-accept_ra.patch
> upstream/0001-addons-ethtool-add-rx-vlan-filter.patch
> upstream/0001-scheduler-import-traceback.patch
> --
> 2.39.2
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist}
2024-06-27 15:01 [pve-devel] [PATCH ifupdown2 1/1] fix #5197: do not run scripts ending with .dpkg-{old, new, tmp, dist} Stefan Hanreich
2024-06-28 14:35 ` Friedrich Weber
2024-07-01 8:35 ` Fabian Grünbichler
@ 2024-07-03 12:58 ` Stefan Hanreich
2 siblings, 0 replies; 4+ messages in thread
From: Stefan Hanreich @ 2024-07-03 12:58 UTC (permalink / raw)
To: pve-devel
superseded by
https://lists.proxmox.com/pipermail/pve-devel/2024-July/064404.html
On 6/27/24 17:01, Stefan Hanreich wrote:
> This can lead to issue when upgrading from ifupdown to ifupdown2. The
> particular issue this fixes occurs in the following scenario:
>
> * Suppose there is a legacy Debian host with ifupdown and ifenslave
> installed that has a bond configured in /etc/network/interfaces.
> * ifenslave installs a script /etc/network/if-pre-up.d/ifenslave.
> * Now, an upgrade creates a second script
> /etc/network/if-pre-up.d/ifenslave.dpkg-new. As ifupdown executes
> network scripts via run-parts which ignores scripts with . in their
> name, ifenslave.dpkg-new has no effect.
> * If the host switches over to ifupdown2 by installing it (removing
> ifupdown, keeping ifenslave) and reboots, the network will not come
> up:
> /etc/network/if-pre-up.d/ifenslave still exists, but is ignored
> by ifupdown2's bond addon [1]
> /etc/network/if-pre-up.d/ifenslave.dpkg-new is executed by ifupdown2
> because it executes all scripts in /etc/network/if-pre-up.d, even if
> their name contains a dot
>
> This leads to ifreload failing on upgrades, which in turn causes
> issues with the networking of upgraded hosts.
>
> Also submitted upstream at [2]
>
> [1] https://github.com/CumulusNetworks/ifupdown2/blob/ccdc386cfab70703b657fe7c0ffceb95448a9c2b/ifupdown2/addons/bond.py#L45
> [2] https://github.com/CumulusNetworks/ifupdown2/pull/304
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> ...dpkg-files-when-running-hook-scripts.patch | 54 +++++++++++++++++++
> debian/patches/series | 1 +
> 2 files changed, 55 insertions(+)
> create mode 100644 debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
>
> diff --git a/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch b/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> new file mode 100644
> index 0000000..eea615f
> --- /dev/null
> +++ b/debian/patches/pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> @@ -0,0 +1,54 @@
> +From dbb759a1383cf736a0fa769c5c5827e1e7f8145c Mon Sep 17 00:00:00 2001
> +From: Stefan Hanreich <s.hanreich@proxmox.com>
> +Date: Tue, 4 Jun 2024 16:17:54 +0200
> +Subject: [PATCH] main: ignore dpkg files when running hook scripts
> +
> +Currently ifupdown2 executes scripts that are backed up by dpkg (e.g.
> +foo.dpkg-old). This can lead to issues with hook scripts getting
> +executed after upgrading ifupdown2 via dpkg, even though they should
> +not be executed.
> +
> +This also brings ifupdown2 closer on par with the behavior of
> +ifupdown, which did not execute hook scripts with dpkg suffixes.
> +
> +Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> +---
> + ifupdown2/ifupdown/ifupdownmain.py | 4 +++-
> + ifupdown2/ifupdown/utils.py | 6 ++++++
> + 2 files changed, 9 insertions(+), 1 deletion(-)
> +
> +diff --git a/ifupdown2/ifupdown/ifupdownmain.py b/ifupdown2/ifupdown/ifupdownmain.py
> +index 51f5460..e6622f0 100644
> +--- a/ifupdown2/ifupdown/ifupdownmain.py
> ++++ b/ifupdown2/ifupdown/ifupdownmain.py
> +@@ -1540,7 +1540,9 @@ class ifupdownMain:
> + try:
> + module_list = os.listdir(msubdir)
> + for module in module_list:
> +- if self.modules.get(module) or module in self.overridden_ifupdown_scripts:
> ++ if (self.modules.get(module)
> ++ or module in self.overridden_ifupdown_scripts
> ++ or utils.is_dpkg_file(module)):
> + continue
> + self.script_ops[op].append(msubdir + '/' + module)
> + except Exception:
> +diff --git a/ifupdown2/ifupdown/utils.py b/ifupdown2/ifupdown/utils.py
> +index 05c7e48..3085e82 100644
> +--- a/ifupdown2/ifupdown/utils.py
> ++++ b/ifupdown2/ifupdown/utils.py
> +@@ -212,6 +212,12 @@ class utils():
> + # what we have in the cache (data retrieved via a netlink dump by
> + # nlmanager). nlmanager return all macs in lower-case
> +
> ++ _dpkg_suffixes = (".dpkg-old", ".dpkg-dist", ".dpkg-new", ".dpkg-tmp")
> ++
> ++ @staticmethod
> ++ def is_dpkg_file(name):
> ++ return any(name.endswith(suffix) for suffix in utils._dpkg_suffixes)
> ++
> + @classmethod
> + def importName(cls, modulename, name):
> + """ Import a named object """
> +--
> +2.39.2
> +
> diff --git a/debian/patches/series b/debian/patches/series
> index 557aa7f..d5772c9 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -7,6 +7,7 @@ pve/0006-openvswitch-ovs-ports-condone-regex-exclude-tap-veth.patch
> pve/0007-allow-vlan-tag-inside-vxlan-tunnel.patch
> pve/0008-lacp-bond-remove-bond-min-links-0-warning.patch
> pve/0009-gvgeb-fix-python-interpreter-shebang.patch
> +pve/0010-main-ignore-dpkg-files-when-running-hook-scripts.patch
> upstream/0001-add-ipv6-slaac-support-inet6-auto-accept_ra.patch
> upstream/0001-addons-ethtool-add-rx-vlan-filter.patch
> upstream/0001-scheduler-import-traceback.patch
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread