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 9B3AC7138D for ; Tue, 18 May 2021 13:43:50 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 82718EACD for ; Tue, 18 May 2021 13:43:50 +0200 (CEST) Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 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 8CF92EAB8 for ; Tue, 18 May 2021 13:43:48 +0200 (CEST) Received: by mail-wm1-x336.google.com with SMTP id y184-20020a1ce1c10000b02901769b409001so1296084wmg.3 for ; Tue, 18 May 2021 04:43:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=odiso-com.20150623.gappssmtp.com; s=20150623; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version; bh=8kPC0M5IzI5qwhyQsmq1B2zyz58H6vuyJ8sAQ3WQ/Jc=; b=YTMtHhcq9QVCeHr9owUfu4dQLw00AOD27zkHZmHDLUWCbgEe85tqoQgT6YVliDZ7tY YGkHA8jUo2+pwvdinYpPwQ9+oPtuO8qtahOKd7OnLltfCdiboEvrorVZ67EhJeAl7ua8 j4x50pptXntctzJgFpzbWgWzxivYV/7plgYYKFy/h4Y+KdCk2c0TULkOBE91pkRuydrf +CAJg17Q1T/3vDYvY2yma1ZigLVWLRr2yB71Z6poqvdsNjJ/vy0h2SCTOYL7+qTcs2rp RXPvYWmldDdnaSo9Xywx8KftuQgkZsIUA5ZPsZ55bqum5RZwGjsIxc2cIi4/+v2TNRdu kJxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version; bh=8kPC0M5IzI5qwhyQsmq1B2zyz58H6vuyJ8sAQ3WQ/Jc=; b=R7TeN4pPaCHWSo5WoYZDE4Ae+l/FPaOHtiSeFHHKb3CA+1XeISaqH/C81oJp4v+eD4 WhKcfefRc/KAJBQdry/GmnhZ5rJqbbmsv0WEHo9BsrMpMbDtXkjuDfKQ0h3txadhyVgD LuFd5/5XZbMHRxtehlVTJUHWwTudXjpBy6pjxL+8qnd3UBIO04Rx+oihcEjTz8R3fqdA 7Oo/+2mI4T0D7EMgNWlgib9VrBlueK/8c1EvCwSpCdCA1/j2N9LxhmnL8uYAh+C7XOEj ekmqj/99QeuY+pX/SevlEuS2wJItjhGQXgkLwXh7dPl34LSwabZmkuf7GEbActgE4pYb vhQA== X-Gm-Message-State: AOAM532XqsKRozYSQ/4K9x+gvlFgJc4BCpILTbStuboZQwyriO+WJBqX NoX2co4OvEEkEEm6MzES1tAXbA== X-Google-Smtp-Source: ABdhPJwZwGoikZVlD6q8Kt97dSTaN/946LNp/lKJpMAAi+I73G2tF54ZnTY5jnUq7unJIEFKWIaFfQ== X-Received: by 2002:a05:600c:4152:: with SMTP id h18mr4431857wmm.155.1621338222135; Tue, 18 May 2021 04:43:42 -0700 (PDT) Received: from [10.59.100.76] (globalOdiso.M6Lille.odiso.net. [89.248.211.242]) by smtp.gmail.com with ESMTPSA id i11sm21503489wrq.26.2021.05.18.04.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 May 2021 04:43:41 -0700 (PDT) Message-ID: From: aderumier@odiso.com To: Wolfgang Bumiller Cc: pve-devel@lists.proxmox.com Date: Tue, 18 May 2021 13:43:40 +0200 In-Reply-To: <20210512122702.3relfzag2yrqv2lo@wobu-vie.proxmox.com> References: <20210510153709.2271214-1-aderumier@odiso.com> <20210510153709.2271214-2-aderumier@odiso.com> <20210512122702.3relfzag2yrqv2lo@wobu-vie.proxmox.com> User-Agent: Evolution 3.40.1 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.860 Adjusted score from AWL reputation of From: address DKIM_SIGNED 0.1 Message has a DKIM or DK signature, not necessarily valid DKIM_VALID -0.1 Message has at least one valid DKIM or DK signature HTML_MESSAGE 0.001 HTML included in message RCVD_IN_DNSWL_NONE -0.0001 Sender listed at https://www.dnswl.org/, no trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [config.pm, lxc.pm] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Tue, 18 May 2021 11:43:50 -0000 thanks for the review wolfgang, I send a v2 tomorrow, with cleanup, refactoring, tests  and support for snapshot rollback (need to check ipam too for ip conflict).(and maybe backup/restore if I have time) Le mercredi 12 mai 2021 à 14:27 +0200, Wolfgang Bumiller a écrit : > 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 >