Re: [pve-devel] [PATCH v2 container 3/4] lxc: Add `link-down` config to allow setting interfaces as disconnected

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Feb 15, 2023 at 03:02:44PM +0100, Christoph Heiss wrote:
> If this network option is set, the host-side link will be forced down
> and the interface won't be connected to the bridge.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> Changes v1 -> v2:
>  * Split trailing whitespace fix into separate patch
>  * Rename option to kebap-case

Sorry I missed that when Thomas replied to the v1.
This is a case where the property already exists in the VM api, so
better reuse the snake_case version here. @Thomas?

>  * Proper option comparison using `safe_boolean_ne`
>  * Copy option to new network conf like the other options
>  * Remove the veth interface from the bridge when disconnected
> 
>  src/PVE/LXC.pm        | 41 ++++++++++++++++++++++++++++++++++-------
>  src/PVE/LXC/Config.pm |  5 +++++
>  src/lxcnetaddbr       |  7 +++++--
>  3 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 0de5ba3..1b93f48 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -955,7 +955,8 @@ sub update_net {
>  	} else {
>  	    if (safe_string_ne($oldnet->{bridge}, $newnet->{bridge}) ||
>  		safe_num_ne($oldnet->{tag}, $newnet->{tag}) ||
> -		safe_num_ne($oldnet->{firewall}, $newnet->{firewall})
> +		safe_num_ne($oldnet->{firewall}, $newnet->{firewall}) ||
> +		safe_boolean_ne($oldnet->{'link-down'}, $newnet->{'link-down'})
>  	    ) {
> 
>  		if ($oldnet->{bridge}) {
> @@ -968,10 +969,28 @@ sub update_net {
>  		}
> 
>  		my ($bridge, $mac, $firewall, $rate) = $newnet->@{'bridge', 'hwaddr', 'firewall', 'rate'};
> -		PVE::LXC::net_tap_plug($veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
> +
> +		if (defined($newnet->{'link-down'})) {
> +		    # The interface must not be connected to the designated
> +		    # bridge if the link was requested to be disconnected.
> +		    # Otherwise it could get re-enabled by something like
> +		    # `ifreload`.
> +		    #
> +		    # Thus only force the host-side link down here and skip
> +		    # adding it to the bridge.
> +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
> +		} else {
> +		    # Connect the interface to the bridge
> +		    PVE::LXC::net_tap_plug(
> +			$veth, $bridge, $newnet->{tag}, $firewall, $newnet->{trunks}, $rate, { mac => $mac });
> +
> +		    # Force the host-side link up if it was previously down.
> +		    PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'up'])
> +			if defined($oldnet->{'link-down'});
> +		}
> 
>  		# This includes the rate:
> -		foreach (qw(bridge tag firewall rate)) {
> +		foreach (qw(bridge tag firewall rate link-down)) {
>  		    $oldnet->{$_} = $newnet->{$_} if $newnet->{$_};
>  		}
>  	    } elsif (safe_string_ne($oldnet->{rate}, $newnet->{rate})) {
> @@ -1002,9 +1021,6 @@ sub hotplug_net {
>      } else {
>  	PVE::Network::veth_create($veth, $vethpeer, $newnet->{bridge}, $newnet->{hwaddr});
>      }
> -    PVE::LXC::net_tap_plug(
> -	$veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks},
> -	$newnet->{rate}, { mac => $newnet->{hwaddr} });
> 
>      # attach peer in container
>      my $cmd = ['lxc-device', '-n', $vmid, 'add', $vethpeer, "$eth" ];
> @@ -1014,8 +1030,19 @@ sub hotplug_net {
>      $cmd = ['lxc-attach', '-n', $vmid, '-s', 'NETWORK', '--', '/sbin/ip', 'link', 'set', $eth ,'up'  ];
>      PVE::Tools::run_command($cmd);
> 
> +    if (defined($newnet->{'link-down'})) {

Any particular reason for moving this down here now?
I'm sure *someone* will run into issues with badly and wrongly
configured containers with this ;-)
It's just nicer for containers to immediately see a readily connected
interface, so maybe keep it up there where the original tap_plug call
was.

> +	# In case the network device should be disconnected, force the host-link down ..
> +	PVE::Tools::run_command(['/sbin/ip', 'link', 'set', 'dev', $veth, 'down']);
> +    } else {
> +	# .. otherwise, connect it normally to the bridge.
> +	# The interface is already up from creation.
> +	PVE::LXC::net_tap_plug(
> +	    $veth, $newnet->{bridge}, $newnet->{tag}, $newnet->{firewall}, $newnet->{trunks},
> +	    $newnet->{rate}, { mac => $newnet->{hwaddr} });
> +    }
> +
>      my $done = { type => 'veth' };
> -    foreach (qw(bridge tag firewall hwaddr name)) {
> +    foreach (qw(bridge tag firewall hwaddr name link-down)) {
>  	$done->{$_} = $newnet->{$_} if $newnet->{$_};
>      }
>      $conf->{$opt} = PVE::LXC::Config->print_lxc_network($done);
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index af25a96..26a2fac 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 ebd6baa..c5d724b 100755
> --- a/src/lxcnetaddbr
> +++ b/src/lxcnetaddbr
> @@ -52,10 +52,13 @@ 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");
> 
> -    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr });
> +    # Only plug the interface into the bridge if it is not set as disconnected by the user.
> +    PVE::LXC::net_tap_plug($iface, $bridge, $tag, $firewall, $trunks, $rate, { mac => $hwaddr })
> +	if !defined($net->{'link-down'});
>  }
> 
>  exit 0;
> --
> 2.39.1