From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CD7B47B622 for ; Wed, 12 May 2021 14:27:35 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BA0D0D1FF for ; Wed, 12 May 2021 14:27:05 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9B78CD1F2 for ; Wed, 12 May 2021 14:27:04 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 66BAC42F58; Wed, 12 May 2021 14:27:04 +0200 (CEST) Date: Wed, 12 May 2021 14:27:02 +0200 From: Wolfgang Bumiller To: Alexandre Derumier Cc: pve-devel@lists.proxmox.com Message-ID: <20210512122702.3relfzag2yrqv2lo@wobu-vie.proxmox.com> References: <20210510153709.2271214-1-aderumier@odiso.com> <20210510153709.2271214-2-aderumier@odiso.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210510153709.2271214-2-aderumier@odiso.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.017 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-container 1/1] add ipam support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 May 2021 12:27:35 -0000 On Mon, May 10, 2021 at 05:37:07PM +0200, Alexandre Derumier wrote: > This add ipam support for nic using sdn vnets. > > - if ips are specified manally, we verify that subnet exist on vnet, and we register ip in ipam > - if nic is on a vnet, but no ip is specified, we auto find the next available ips in this vnet subnet > - if a gateway is defined on the subnet, we override current vm nc gateway > - extra informations like mac address,hostname are registered in external ipam. > - if dns server exist in the zone, we register vm hostname in ipam for each ip address > > - ips addresses are removed on ct destroy, nic destroy, ip change or vnet change > > Signed-off-by: Alexandre Derumier > --- > src/PVE/LXC.pm | 3 + > src/PVE/LXC/Config.pm | 168 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 171 insertions(+) > > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm > index 7e6f378..f6bd806 100644 > --- a/src/PVE/LXC.pm > +++ b/src/PVE/LXC.pm > @@ -847,6 +847,9 @@ sub destroy_lxc_container { > rmdir "/var/lib/lxc/$vmid/rootfs"; > unlink "/var/lib/lxc/$vmid/config"; > rmdir "/var/lib/lxc/$vmid"; > + > + PVE::LXC::Config->delete_ipam_ifaces($conf); > + > if (defined $replacement_conf) { > PVE::LXC::Config->write_config($vmid, $replacement_conf); > } else { > diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm > index 7b82f65..c0be257 100644 > --- a/src/PVE/LXC/Config.pm > +++ b/src/PVE/LXC/Config.pm > @@ -12,6 +12,14 @@ use PVE::INotify; > use PVE::JSONSchema qw(get_standard_option); > use PVE::Tools; > > +my $have_sdn; > +eval { > + require PVE::Network::SDN::Vnets; > + require PVE::Network::SDN::Subnets; > + $have_sdn = 1; > +}; > + > + > use base qw(PVE::AbstractConfig); > > use constant {FIFREEZE => 0xc0045877, > @@ -1044,6 +1052,10 @@ sub update_pct_config { > $class->check_protection($conf, "can't remove CT $vmid drive '$opt'"); > } elsif ($opt eq 'unprivileged') { > die "unable to delete read-only option: '$opt'\n"; > + } elsif ($opt =~ m/^net(\d+)$/) { > + my $netid = $1; > + my $oldnet = $class->parse_lxc_network($conf->{$opt}); > + delete_net_ip($conf->{hostname}, $oldnet, "vm:$vmid net:$netid"); > } > $class->add_to_pending_delete($conf, $opt); > } > @@ -1071,7 +1083,24 @@ sub update_pct_config { > $value = PVE::LXC::verify_searchdomain_list($value); > } elsif ($opt eq 'unprivileged') { > die "unable to modify read-only option: '$opt'\n"; > + } elsif ($opt =~ m/^net(\d+)$/) { > + my $netid = $1; > + my $net = $class->parse_lxc_network($value); > + my $oldnet = $class->parse_lxc_network($conf->{$opt}); > + my $hostname = $param->{hostname} ? $param->{hostname} : $conf->{hostname}; > + update_net_ip($net, $oldnet, $hostname, $hostname, "vm:$vmid net:$netid"); > + $value = $class->print_lxc_network($net); > + } elsif ($opt eq 'hostname') { > + #if hostname change, update ipam + dns for each ip > + foreach my $netopt (sort keys %$conf) { > + next if $netopt !~ m/^net(\d+)$/; > + my $netid = $1; > + my $net = $class->parse_lxc_network($conf->{$netopt}); > + my $oldnet = $class->parse_lxc_network($conf->{$netopt}); ^ you can juse use $net twice, no need to parse the same value twice > + update_net_ip($net, $oldnet, $value, $conf->{hostname}, "vm:$vmid net:$netid"); > + } > } > + > $conf->{pending}->{$opt} = $value; > $class->remove_from_pending_delete($conf, $opt); > } > @@ -1663,4 +1692,143 @@ sub get_backup_volumes { > return $return_volumes; > } > > +sub delete_ipam_ifaces { > + my ($class, $conf) = @_; > + > + return if !$have_sdn; > + > + foreach my $opt (sort keys %$conf) { ^ do we need this to be sorted? > + next if $opt !~ m/^net(\d+)$/; > + my $netid = $1; > + my $net = $class->parse_lxc_network($conf->{$opt}); > + my $bridge = $net->{bridge}; > + my $hostname = $conf->{hostname}; > + my $description = ''; > + > + my $subnets = PVE::Network::SDN::Vnets::get_subnets($bridge); > + next if !keys %{$subnets}; > + > + eval { > + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip}, $hostname, $description) if $net->{ip} && $net->{ip} ne 'dhcp' && $net->{ip} ne 'manual'; > + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip6}, $hostname, $description) if $net->{ip6} && $net->{ip6} ne 'dhcp' && $net->{ip6} ne 'manual' && $net->{ip6} ne 'auto'; ^ Can we please factor out the above conditions in the post-fix if statements into a separate helper (ie. 'is_static_ip' or something?), because you use the same condition multiple times, but there's a case further down below where it's inconsistent and probably wrong... (btw. a helper wouldn't necessarily need to bother distinguishing between ipv4 and ipv6 since in ipv4 'auto' simply never happens) > + }; > + if ($@) { > + warn $@; > + } > + } > +} > + > +sub update_net_ip { > + my ($net, $oldnet, $hostname, $oldhostname, $description) = @_; > + > + return if !$have_sdn; > + > + my $oldbridge = $oldnet->{bridge}; > + my $bridge = $net->{bridge}; > + my $mac = $net->{hwaddr}; > + > + my $subnets = PVE::Network::SDN::Vnets::get_subnets($bridge); > + > + return if !keys %{$subnets}; I have a feeling the code below might get a little more readable if we pull out some conditions into variables here, not sure, something like $ip_changed, $new_ip_not_static, $old_ip_not_static for both ipv4 and ipv6 Though I'm not sure... > + > + #add new ip first > + my $new_ipv4_allocated = undef; > + eval { > + if (!$net->{ip}) { > + $net->{ip} = PVE::Network::SDN::Vnets::get_next_free_cidr($bridge, $hostname, $mac, $description); > + $new_ipv4_allocated = 1; > + } elsif ($net->{ip} ne 'dhcp' && $net->{ip} ne 'manual') { > + > + if ($oldnet->{ip} && $net->{ip} eq $oldnet->{ip}) { would be !$ip_changed > + #update ip attributes if no ip address change > + PVE::Network::SDN::Vnets::update_cidr($bridge, $net->{ip}, $hostname, $oldhostname, $mac, $description); > + } else { > + PVE::Network::SDN::Vnets::add_cidr($bridge, $net->{ip}, $hostname, $mac, $description) if !$oldnet->{ip} || $net->{ip} ne $oldnet->{ip}; I think it's better to move the post-fix condition up to replace the `else` with an `elsif`, since IMO it should also cover the line below, right? Note that the 2nd half of the condition is already implied at this point. > + $new_ipv4_allocated = 1; > + } > + } > + }; > + if ($@) { > + die $@; > + } > + > + #then add ip6 > + eval { > + if (!$net->{ip6}) { > + $net->{ip6} = PVE::Network::SDN::Vnets::get_next_free_cidr($bridge, $hostname, $mac, $description, 6); > + } elsif ($net->{ip6} ne 'dhcp' && $net->{ip6} ne 'manual' && $net->{ip6} ne 'auto') { > + if ($oldnet->{ip6} && $net->{ip6} eq $oldnet->{ip6}) { > + #update ipv6 attributes if no ip address change > + PVE::Network::SDN::Vnets::update_cidr($bridge, $net->{ip6}, $hostname, $oldhostname, $mac, $description); > + } else { > + PVE::Network::SDN::Vnets::add_cidr($bridge, $net->{ip6}, $hostname, $mac, $description) if !$oldnet->{ip6} || $net->{ip6} ne $oldnet->{ip6}; ^ same here > + } > + } > + }; > + if ($@) { > + my $err = $@; > + #if error, delete previously added ipv4 > + if ($new_ipv4_allocated) { > + eval { > + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip}, $hostname, $description); > + }; > + } > + die $err; > + } > + > + #delete old ip > + if ($oldnet->{ip} && $oldnet->{ip} ne 'dhcp' && $oldnet->{ip} ne 'manual') { > + my $deletebridge = $oldbridge ne $bridge ? $oldbridge : $bridge; > + eval { > + PVE::Network::SDN::Vnets::del_cidr($deletebridge, $oldnet->{ip}, $hostname, $description) if !$net->{ip} || $net->{ip} ne $oldnet->{ip}; > + }; > + warn $@ if $@; > + } > + > + #delete old ip6 > + if ($oldnet->{ip6} && $oldnet->{ip6} ne 'dhcp' && $oldnet->{ip6} ne 'manual' && $net->{ip6} ne 'auto') { > + my $deletebridge = $oldbridge ne $bridge ? $oldbridge : $bridge; > + eval { > + PVE::Network::SDN::Vnets::del_cidr($deletebridge, $oldnet->{ip6}, $hostname) if !$net->{ip6} || $net->{ip6} ne $oldnet->{ip6}; > + }; > + warn $@ if $@; > + } > + > + #update gateway > + if ($net->{ip} && $net->{ip} ne 'dhcp' && $net->{ip} ne 'manual') { > + my ($ip, $mask) = split(/\//, $net->{ip}); > + my ($subnetidv4, $subnetv4) = PVE::Network::SDN::Subnets::find_ip_subnet($ip, $mask, $subnets); > + $net->{gw} = $subnetv4->{gateway} if $subnetv4->{gateway}; > + } > + > + if ($net->{ip6} && $net->{ip6} ne 'dhcp' && $net->{ip6} ne 'manual' && $net->{ip6} ne 'auto') { > + my ($ip6, $mask6) = split(/\//, $net->{ip6}); > + my ($subnetidv6, $subnetv6) = PVE::Network::SDN::Subnets::find_ip_subnet($ip6, $mask6, $subnets); > + $net->{gw6} = $subnetv6->{gateway} if $subnetv6->{gateway}; > + } > + > +} > + > +sub delete_net_ip { > + my ($hostname, $net, $description) = @_; > + > + return if !$have_sdn; > + > + my $bridge = $net->{bridge}; > + my $subnets = PVE::Network::SDN::Vnets::get_subnets($bridge); > + return if !keys %{$subnets}; > + > + if ($net->{ip6} && ($net->{ip6} eq 'auto' || $net->{ip6} eq 'manual' || $net->{ip6} eq 'dhcp')) { So here this reads as: "$net->{ip6} is one of 'auto', 'manual' or 'dhcp'" with the helper mentioned above: if ($net->{ip6} && !ip_is_static($net->{ip6})) > + eval { > + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip6}, $hostname, $description); > + }; > + warn $@ if $@; > + } elsif ($net->{ip} && $net->{ip} ne 'dhcp' && $net->{ip} ne 'manual') { but the IPv4 case here reads as: "$net->{ip} is set but is neither 'dhcp' nor 'manual'" with the helper mentioned above: if ($net->{ip} && ip_is_static($net->{ip})) this seems inconsistent (note the '!' on the `ip_is_static` call differ between the ipv4 & ipv6 cases) I think you meant to use the same condition for both? > + eval { > + PVE::Network::SDN::Vnets::del_cidr($bridge, $net->{ip}, $hostname, $description); > + }; > + warn $@ if $@; > + } > +} > + > 1; > -- > 2.20.1