[pve-devel] [PATCH container/manager 0/2] fix #3413: Add `Disconnect` option for LXC networks

[pve-devel] [PATCH container/manager 0/2] fix #3413: Add `Disconnect` option for LXC networks

[Christoph Heiss]

This adds a `Disconnect` option for network interfaces on LXC
containers, much like it already exists for VMs. This has been requested
in #3413 [0] and seems useful, esp. considering we already support the
same thing for VMs.

One thing to note is that LXC does not seem to support the notion of
setting an interface down. The `flags` property would suggest that this
possible [1], but AFAICS it does not work. I tried setting the value as
empty and to something else than "up" (since that is really the only
supported option [2][3]), which both had absolutely no effect.

The solution was to force the host-side link of the container network
down, thus effectively becoming "physically" disconnected.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=3413
[1] https://linuxcontainers.org/lxc/manpages/man5/lxc.container.conf.5.html#lbAO
[2] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L453-L467
[3] https://github.com/lxc/lxc/blob/08f0e769/src/lxc/confile.c#L5933-L5952

pve-container:

Christoph Heiss (1):
      net: Add `link_down` config to allow setting interfaces as disconnected

 src/PVE/LXC.pm        | 20 +++++++++++++-------
 src/PVE/LXC/Config.pm |  5 +++++
 src/lxcnetaddbr       |  3 ++-
 3 files changed, 20 insertions(+), 8 deletions(-)

pve-manager:

Christoph Heiss (1):
      lxc: Add `Disconnect` option for network interfaces

 www/manager6/Parser.js      |  3 +++
 www/manager6/lxc/Network.js | 13 +++++++++++++
 2 files changed, 16 insertions(+)

--
2.39.1

[pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Christoph Heiss]

If this network option is set, the host-side link will be forced down.
This has the effect that the interface inside the container has
LOWERLAYERDOWN set, which basically means that the PHY is considered
down, thus effectivly being "unplugged".

Also fix some trailing whitespace while touching the file.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/PVE/LXC.pm        | 20 +++++++++++++-------
 src/PVE/LXC/Config.pm |  5 +++++
 src/lxcnetaddbr       |  3 ++-
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index ce6d5a5..039a476 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -668,7 +668,7 @@ sub update_lxc_config {

     # some init scripts expect a linux terminal (turnkey).
     $raw .= "lxc.environment = TERM=linux\n";
-
+
     my $utsname = $conf->{hostname} || "CT$vmid";
     $raw .= "lxc.uts.name = $utsname\n";

@@ -932,8 +932,9 @@ sub update_net {
 	my $oldnet = PVE::LXC::Config->parse_lxc_network($oldnetcfg);

 	if (safe_string_ne($oldnet->{hwaddr}, $newnet->{hwaddr}) ||
-	    safe_string_ne($oldnet->{name}, $newnet->{name})) {
-
+	    safe_string_ne($oldnet->{name}, $newnet->{name}) ||
+	    defined($oldnet->{link_down}) != defined($newnet->{link_down})
+	) {
 	    PVE::Network::veth_delete($veth);
 	    delete $conf->{$opt};
 	    PVE::LXC::Config->write_config($vmid, $conf);
@@ -1010,6 +1011,11 @@ sub hotplug_net {
     $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
     PVE::Tools::run_command($cmd);

+    # In case the network device should be disconnected, force the host-link down
+    if (defined($newnet->{link_down})) {
+	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
+    }
+
     my $done = { type => 'veth' };
     foreach (qw(bridge tag firewall hwaddr name)) {
 	$done->{$_} = $newnet->{$_} if $newnet->{$_};
@@ -1703,14 +1709,14 @@ sub __mountpoint_mount {
     my $type = $mountpoint->{type};
     my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
     my $mounted_dev;
-
+
     return if !$volid || !$mount;

     $mount =~ s!/+!/!g;

     my $mount_path;
     my ($mpfd, $parentfd, $last_dir);
-
+
     if (defined($rootdir)) {
 	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
 	    __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
@@ -1719,7 +1725,7 @@ sub __mountpoint_mount {
     if (defined($stage_mount)) {
 	$mount_path = $rootdir;
     }
-
+
     my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);

     die "unknown snapshot path for '$volid'" if !$storage && defined($snapname);
@@ -1828,7 +1834,7 @@ sub __mountpoint_mount {
 	warn "cannot enable quota control for bind mounts\n" if $quota;
 	return wantarray ? ($volid, 0, undef) : $volid;
     }
-
+
     die "unsupported storage";
 }

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index af25a96..1d4362e 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -814,6 +814,11 @@ our $netconf_desc = {
 	description => "Apply rate limiting to the interface",
 	optional => 1,
     },
+    link_down => {
+	type => 'boolean',
+	description => 'Whether this interface should be disconnected (like pulling the plug).',
+	optional => 1,
+    },
 };
 PVE::JSONSchema::register_format('pve-lxc-network', $netconf_desc);

diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
index 83052e1..d8c6767 100755
--- a/src/lxcnetaddbr
+++ b/src/lxcnetaddbr
@@ -58,7 +58,8 @@ if (-d "/sys/class/net/$iface") {
     #avoid insecure dependency;
     ($bridgemtu) = $bridgemtu =~ /(\d+)/;

-    PVE::Tools::run_command("/sbin/ip link set dev $iface up mtu $bridgemtu");
+    my $linkstate = defined($net->{link_down}) ? 'down' : 'up';
+    PVE::Tools::run_command("/sbin/ip link set dev $iface $linkstate mtu $bridgemtu");
     PVE::Tools::run_command("/sbin/ip addr add 0.0.0.0/0 dev $iface");

     if ($have_sdn) {
--
2.39.1

[pve-devel] [PATCH manager 2/2] lxc: Add `Disconnect` option for network interfaces

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 www/manager6/Parser.js      |  3 +++
 www/manager6/lxc/Network.js | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
index 9f7b2c84..c3772d3b 100644
--- a/www/manager6/Parser.js
+++ b/www/manager6/Parser.js
@@ -298,6 +298,8 @@ Ext.define('PVE.Parser', {
 		data[match_res[1]] = match_res[2];
 	    } else if ((match_res = p.match(/^firewall=(\d+)$/)) !== null) {
 		data.firewall = PVE.Parser.parseBoolean(match_res[1]);
+	    } else if ((match_res = p.match(/^link_down=(\d+)$/)) !== null) {
+		data.link_down = PVE.Parser.parseBoolean(match_res[1]);
 	    } else if (!p.match(/^type=\S+$/)) {
 		console.warn(`could not parse LXC network string ${p}`);
 	    }
@@ -319,6 +321,7 @@ Ext.define('PVE.Parser', {
 	    name: 1,
 	    rate: 1,
 	    tag: 1,
+	    link_down: 1,
 	};
 	return Object.entries(config)
 	    .filter(([k, v]) => v !== undefined && v !== '' && knownKeys[k])
diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
index 85033bd8..746924b3 100644
--- a/www/manager6/lxc/Network.js
+++ b/www/manager6/lxc/Network.js
@@ -282,6 +282,12 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
 		minValue: 576,
 		maxValue: 65535,
 	    },
+	    {
+		xtype: 'proxmoxcheckbox',
+		fieldLabel: gettext('Disconnect'),
+		name: 'link_down',
+		value: cdata.link_down,
+	    },
 	];

 	me.advancedColumn2 = [
@@ -539,6 +545,12 @@ Ext.define('PVE.lxc.NetworkView', {
 		    width: 80,
 		    dataIndex: 'mtu',
 		},
+		{
+		    header: gettext('Disconnected'),
+		    width: 100,
+		    dataIndex: 'link_down',
+		    renderer: Proxmox.Utils.format_boolean,
+		},
 	    ],
 	    listeners: {
 		activate: me.load,
@@ -564,6 +576,7 @@ Ext.define('PVE.lxc.NetworkView', {
 	    'tag',
 	    'firewall',
 	    'mtu',
+	    'link_down',
 	],
     });
 });
--
2.39.1

Re: [pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Wolfgang Bumiller]

On Mon, Feb 13, 2023 at 02:56:59PM +0100, Christoph Heiss wrote:
> If this network option is set, the host-side link will be forced down.
> This has the effect that the interface inside the container has
> LOWERLAYERDOWN set, which basically means that the PHY is considered
> down, thus effectivly being "unplugged".
> 
> Also fix some trailing whitespace while touching the file.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  src/PVE/LXC.pm        | 20 +++++++++++++-------
>  src/PVE/LXC/Config.pm |  5 +++++
>  src/lxcnetaddbr       |  3 ++-
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index ce6d5a5..039a476 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -668,7 +668,7 @@ sub update_lxc_config {
> 
>      # some init scripts expect a linux terminal (turnkey).
>      $raw .= "lxc.environment = TERM=linux\n";
> -
> +
>      my $utsname = $conf->{hostname} || "CT$vmid";
>      $raw .= "lxc.uts.name = $utsname\n";
> 
> @@ -932,8 +932,9 @@ sub update_net {
>  	my $oldnet = PVE::LXC::Config->parse_lxc_network($oldnetcfg);
> 
>  	if (safe_string_ne($oldnet->{hwaddr}, $newnet->{hwaddr}) ||
> -	    safe_string_ne($oldnet->{name}, $newnet->{name})) {
> -
> +	    safe_string_ne($oldnet->{name}, $newnet->{name}) ||
> +	    defined($oldnet->{link_down}) != defined($newnet->{link_down})

Doing this here would cause the interface to be deleted and recreated in
a "down" state, which is much more disruptive than it needs to be.
Instead, this should be treated more like we do changing the 'bridge'
property (the 'else' case just below this), and stop after the
`tap_unplug` for the 'down' case.

> +	) {
>  	    PVE::Network::veth_delete($veth);
>  	    delete $conf->{$opt};
>  	    PVE::LXC::Config->write_config($vmid, $conf);
> @@ -1010,6 +1011,11 @@ sub hotplug_net {
>      $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
>      PVE::Tools::run_command($cmd);
> 
> +    # In case the network device should be disconnected, force the host-link down
> +    if (defined($newnet->{link_down})) {
> +	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);

These interfaces are usually part of a bridge, and therefore the next
`ifreload` (and probably some other things) would re-enable them
automatically.

We do need to actually "unplug" them from the bridge (tap_unplug) to
avoid this.

> +    }
> +
>      my $done = { type => 'veth' };
>      foreach (qw(bridge tag firewall hwaddr name)) {
>  	$done->{$_} = $newnet->{$_} if $newnet->{$_};
> @@ -1703,14 +1709,14 @@ sub __mountpoint_mount {
>      my $type = $mountpoint->{type};
>      my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
>      my $mounted_dev;
> -
> +
>      return if !$volid || !$mount;
> 
>      $mount =~ s!/+!/!g;
> 
>      my $mount_path;
>      my ($mpfd, $parentfd, $last_dir);
> -
> +
>      if (defined($rootdir)) {
>  	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
>  	    __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
> @@ -1719,7 +1725,7 @@ sub __mountpoint_mount {
>      if (defined($stage_mount)) {
>  	$mount_path = $rootdir;
>      }
> -
> +
>      my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
> 
>      die "unknown snapshot path for '$volid'" if !$storage && defined($snapname);
> @@ -1828,7 +1834,7 @@ sub __mountpoint_mount {
>  	warn "cannot enable quota control for bind mounts\n" if $quota;
>  	return wantarray ? ($volid, 0, undef) : $volid;
>      }
> -
> +
>      die "unsupported storage";
>  }
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index af25a96..1d4362e 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -814,6 +814,11 @@ our $netconf_desc = {
>  	description => "Apply rate limiting to the interface",
>  	optional => 1,
>      },
> +    link_down => {
> +	type => 'boolean',
> +	description => 'Whether this interface should be disconnected (like pulling the plug).',
> +	optional => 1,
> +    },
>  };
>  PVE::JSONSchema::register_format('pve-lxc-network', $netconf_desc);
> 
> diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
> index 83052e1..d8c6767 100755
> --- a/src/lxcnetaddbr
> +++ b/src/lxcnetaddbr
> @@ -58,7 +58,8 @@ if (-d "/sys/class/net/$iface") {
>      #avoid insecure dependency;
>      ($bridgemtu) = $bridgemtu =~ /(\d+)/;
> 
> -    PVE::Tools::run_command("/sbin/ip link set dev $iface up mtu $bridgemtu");
> +    my $linkstate = defined($net->{link_down}) ? 'down' : 'up';

We need to skip the rest altogether if the link is supposed to stay down
reliably. As mentioned above, a 'down' interface that is still plugged
into a bridge will get activated sooner or later...

> +    PVE::Tools::run_command("/sbin/ip link set dev $iface $linkstate mtu $bridgemtu");
>      PVE::Tools::run_command("/sbin/ip addr add 0.0.0.0/0 dev $iface");
> 
>      if ($have_sdn) {
> --
> 2.39.1

Re: [pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Christoph Heiss]

Thanks for the review!

On Mon, Feb 13, 2023 at 03:31:32PM +0100, Wolfgang Bumiller wrote:
> On Mon, Feb 13, 2023 at 02:56:59PM +0100, Christoph Heiss wrote:
> > [..]
> >
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index ce6d5a5..039a476 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -668,7 +668,7 @@ sub update_lxc_config {
> >
> >      # some init scripts expect a linux terminal (turnkey).
> >      $raw .= "lxc.environment = TERM=linux\n";
> > -
> > +
> >      my $utsname = $conf->{hostname} || "CT$vmid";
> >      $raw .= "lxc.uts.name = $utsname\n";
> >
> > @@ -932,8 +932,9 @@ sub update_net {
> >  	my $oldnet = PVE::LXC::Config->parse_lxc_network($oldnetcfg);
> >
> >  	if (safe_string_ne($oldnet->{hwaddr}, $newnet->{hwaddr}) ||
> > -	    safe_string_ne($oldnet->{name}, $newnet->{name})) {
> > -
> > +	    safe_string_ne($oldnet->{name}, $newnet->{name}) ||
> > +	    defined($oldnet->{link_down}) != defined($newnet->{link_down})
>
> Doing this here would cause the interface to be deleted and recreated in
> a "down" state, which is much more disruptive than it needs to be.
> Instead, this should be treated more like we do changing the 'bridge'
> property (the 'else' case just below this), and stop after the
> `tap_unplug` for the 'down' case.
Ack. I see, that's a good point.

>
> > +	) {
> >  	    PVE::Network::veth_delete($veth);
> >  	    delete $conf->{$opt};
> >  	    PVE::LXC::Config->write_config($vmid, $conf);
> > @@ -1010,6 +1011,11 @@ sub hotplug_net {
> >      $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
> >      PVE::Tools::run_command($cmd);
> >
> > +    # In case the network device should be disconnected, force the host-link down
> > +    if (defined($newnet->{link_down})) {
> > +	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
>
> These interfaces are usually part of a bridge, and therefore the next
> `ifreload` (and probably some other things) would re-enable them
> automatically.
I did not know this, thanks for explaining!

>
> We do need to actually "unplug" them from the bridge (tap_unplug) to
> avoid this.
Will rework that for v2 and do it properly.

>
> > +    }
> > +
> >      my $done = { type => 'veth' };
> >      foreach (qw(bridge tag firewall hwaddr name)) {
> >  	$done->{$_} = $newnet->{$_} if $newnet->{$_};
> >
> > [..]
> > diff --git a/src/lxcnetaddbr b/src/lxcnetaddbr
> > index 83052e1..d8c6767 100755
> > --- a/src/lxcnetaddbr
> > +++ b/src/lxcnetaddbr
> > @@ -58,7 +58,8 @@ if (-d "/sys/class/net/$iface") {
> >      #avoid insecure dependency;
> >      ($bridgemtu) = $bridgemtu =~ /(\d+)/;
> >
> > -    PVE::Tools::run_command("/sbin/ip link set dev $iface up mtu $bridgemtu");
> > +    my $linkstate = defined($net->{link_down}) ? 'down' : 'up';
>
> We need to skip the rest altogether if the link is supposed to stay down
> reliably. As mentioned above, a 'down' interface that is still plugged
> into a bridge will get activated sooner or later...
Funnily enough, in my first draft I actually had it like that, so that
it wasn't connected to the brigde at all. Now I know what cases to also
test for v2.

>
> > +    PVE::Tools::run_command("/sbin/ip link set dev $iface $linkstate mtu $bridgemtu");
> >      PVE::Tools::run_command("/sbin/ip addr add 0.0.0.0/0 dev $iface");
> >
> >      if ($have_sdn) {
> > --
> > 2.39.1

Re: [pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Thomas Lamprecht]

On 13/02/2023 14:56, Christoph Heiss wrote:
> +    link_down => {
> +	type => 'boolean',
> +	description => 'Whether this interface should be disconnected (like pulling the plug).',
> +	optional => 1,
> +    },

I'd prefer using kebab-case over snake_case for new properties, or do
we really gain anything here (e.g., qemu-server related infra reuse?)

besides from that, the patch here contains quite some unrelated whitespace
changes, please avoid that.

Re: [pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Christoph Heiss]

On Tue, Feb 14, 2023 at 10:52:15AM +0100, Thomas Lamprecht wrote:
> On 13/02/2023 14:56, Christoph Heiss wrote:
> > +    link_down => {
> > +	type => 'boolean',
> > +	description => 'Whether this interface should be disconnected (like pulling the plug).',
> > +	optional => 1,
> > +    },
>
> I'd prefer using kebab-case over snake_case for new properties, or do
> we really gain anything here (e.g., qemu-server related infra reuse?)
Not that I would see. This was very much inspired by the qemu-server API
and would provide some form of "uniformity" between those two, but I
will change that for v2. kebap-case is nicer anyway.

>
> besides from that, the patch here contains quite some unrelated whitespace
> changes, please avoid that.
That was intentional (as noted), since there are trailing whitespaces
all over the file, which can be somewhat annoying if one's editor
automatically trims trailing whitespaces on save purposefully.
But I will omit that from the next revision, fine by me.

Re: [pve-devel] [PATCH container 1/2] net: Add `link_down` config to allow setting interfaces as disconnected

[Thomas Lamprecht]

On 14/02/2023 11:10, Christoph Heiss wrote:
>> besides from that, the patch here contains quite some unrelated whitespace
>> changes, please avoid that.
> That was intentional (as noted), since there are trailing whitespaces
> all over the file, which can be somewhat annoying if one's editor
> automatically trims trailing whitespaces on save purposefully.
> But I will omit that from the next revision, fine by me.

I never said that fixing it is wrong, but it's completely unrelated for
your change, and thus noise - please do that in a separate patch upfront
for the next revision, then it could have been already applied now ;-)