Re: [pve-devel] [PATCH pve-network v5 1/2] ipam: add Nautobot plugin

From: Hannes Duerr <h.duerr@proxmox.com>
To: Stefan Hanreich <s.hanreich@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-network v5 1/2] ipam: add Nautobot plugin
Date: Tue, 3 Jun 2025 14:25:23 +0200	[thread overview]
Message-ID: <ac2f5f47-de16-4406-9ae5-53a541f32faa@proxmox.com> (raw)
In-Reply-To: <06185ad1-384d-49ea-8c44-d726e19ccef7@proxmox.com>


On 5/28/25 11:32, Stefan Hanreich wrote:
> Tested the following things and they worked:
>
> * Create a new simple zone with Nautobot IPAM
> * Create a VNet with a Subnet + GW -> IP allocated
> * Create a VM on that VNet -> IP allocated
> * Create a CT on that VNet -> IP allocated
> * Change network device to another bridge -> IP deleted
> * Change back to IPAM VNet -> IP allocated
> * Live-Migrate VM
> * Check if VM / CT get IP via DHCP
> * try to delete subnet with entries -> fails
> * delete network devices and try again -> succeeds
> * update gateway -> changes in IPAM
> * add DHCP range and test C/R/U/D network device again
>
>
> I added a second IPAM with a different namespace, and this is where some
> problems started to occur. The PVE Web UI threw an error when trying to
> create the same subnet, but in a different namespace:
>
>    create sdn subnet object failed: could not add the subnet
>    HASH(0x58bc2d033e10) because it already exists in nautobot (500)
>
> (also note that here we need to print the id field of the hash in the
> error message)
>
>
> Some additional errors occurred when I manually created the subnet in
> the SDN configuration, because it seems like many functions that fetch
> IDs don't seem to consider the namespace of the objects they're fetching
> (e.g. get_ip_id / get_prefix_id / ...). This means that IP addresses get
> created in the wrong prefix, because the plugin just takes the first
> prefix found, regardless of their namespace. Might make issues for users
> that use a separated namespace for PVE, but have the same prefix
> configured in other namespaces. Since most methods use the ID returned
> by the helper functions, this should be easily fixed by adjusting those
> helpers.
>
> some additional comments inline
Thanks for testing and reviewing.
I answered to some of your comments inline.
I will try to upstream the offset parameter in nautobot and publish a
new version with all the changes afterwards.

>
>
> On 5/26/25 14:19, Hannes Duerr wrote:
>>   PERL5DIR=${DESTDIR}/usr/share/perl5
>> diff --git a/src/PVE/Network/SDN/Ipams/NautobotPlugin.pm b/src/PVE/Network/SDN/Ipams/NautobotPlugin.pm
>> new file mode 100644
>> index 0000000..a6eb7ff
>> --- /dev/null
>> +++ b/src/PVE/Network/SDN/Ipams/NautobotPlugin.pm
>> @@ -0,0 +1,510 @@
>> +package PVE::Network::SDN::Ipams::NautobotPlugin;
>> +
>> +use strict;
>> +use warnings;
>> +use PVE::INotify;
>> +use PVE::Cluster;
>> +use PVE::Tools;
>> +use NetAddr::IP;
>> +use Net::Subnet qw(subnet_matcher);
>> +
>> +use base('PVE::Network::SDN::Ipams::Plugin');
>> +
>> +sub type {
>> +    return 'nautobot';
>> +}
>> +
>> +sub properties {
>> +    return {
>> +        namespace => {
>> +            type => 'string',
>> +        },
>> +    };
>> +}
> missing description here. afaict we should also add descriptions for url
> / token? since they're not defined in the base plugin.
Good point yes they're missing, will add them.
Will also send patches for the other plugins as they're missing there as 
well.
>> +sub options {
>> +    return {
>> +        url => { optional => 0 },
>> +        token => { optional => 0 },
>> +        namespace => { optional => 0 },
>> +        fingerprint => { optional => 1 },
>> +    };
>> +}
>> +
>> +sub default_ip_status {
>> +    return 'Active';
>> +}
> could maybe just be a constant my $DEFAULT_IP_STATUS; ?
I switched to default_ip_status () {} as this gives the perl
interpreter the option to inline the funtion [0].
Another option would be using 'use contant DEFAULT_IP_STATUS ="Active"'
which is comparable to using the function AFAIU.

[0] 
https://www.oreilly.com/library/view/programming-perl-4th/9781449321451/ch07s04s01.html
>> +
>> +sub nautobot_api_request {
>> +    my ($config, $method, $path, $params) = @_;
>> +
>> +    return PVE::Network::SDN::api_request(
>> +        $method,
>> +        "$config->{url}${path}",
>> +        [
>> +            'Content-Type' => 'application/json; charset=UTF-8',
>> +            'Authorization' => "token $config->{token}",
>> +            'Accept' => "application/json",
>> +        ],
>> +        $params,
>> +        $config->{fingerprint},
>> +    );
>> +}
>> +
>> +sub add_subnet {
>> +    my ($class, $config, undef, $subnet, $noerr) = @_;
>> +
>> +    my $cidr = $subnet->{cidr};
>> +    my $namespace = $config->{namespace};
>> +
>> +    my $internalid = get_prefix_id($config, $cidr, $noerr);
>> +    if ($internalid) {
>> +        return if $noerr;
>> +        die "could not add the subnet $subnet because it already exists in nautobot\n";
> should use the CIDR field of the subnet, since $subnet is a hash
agree
> +
> +sub add_range_next_freeip {
> +    my ($class, $config, $subnet, $range, $data, $noerr) = @_;
> +
> +    my $cidr = NetAddr::IP->new($subnet->{cidr});
> +    my $namespace = $config->{namespace};
> +
> +    # Nautobot does not support IP ranges, only prefixes.
> +    # Therefore we divide the range into smaller pool prefixes,
> +    # each containing 256 addresses, and search them for available IPs
> +    my $prefix_size = $cidr->version == 4 ? 24 : 120;
> maybe we could default to the prefix size of the subnet if it is smaller
> than 24 / 120? Not so likely for IPv6, but for IPv4 it'd make sense?
>
>> +    my $increment = 256;
>> +    my $found_ip = undef;
>> +
>> +    my $start_range = NetAddr::IP->new($range->{'start-address'}, $prefix_size);
>> +    my $end_range = NetAddr::IP->new($range->{'end-address'}, $prefix_size);
>> +    my $matcher = subnet_matcher($end_range->cidr);
>> +    my $current_ip = $start_range;
>> +
>> +    while (1) {
>> +        my $current_cidr = $current_ip->addr . "/$prefix_size";
>> +
>> +        my $params = {
>> +            prefix => $current_cidr,
>> +            namespace => $namespace,
>> +            status => default_ip_status(),
>> +            type => "pool",
>> +        };
>> +
>> +        my $prefix_id = get_prefix_id($config, $current_cidr, $noerr);
>> +        if ($prefix_id) {
>> +            # search the existing prefix for valid ip
>> +            $found_ip =
>> +                find_ip_in_prefix($config, $prefix_id, $increment, $start_range, $end_range);
>> +        } else {
>> +            # create temporary pool prefix
>> +            my $temp_prefix =
>> +                eval { return nautobot_api_request($config, "POST", "/ipam/prefixes/", $params); };
> should probably do some error handling?
>
>> +
>> +            my $temp_prefix_id = $temp_prefix->{id};
>> +
>> +            # search temporarly created prefix
>> +            $found_ip =
>> +                find_ip_in_prefix($config, $temp_prefix_id, $increment, $start_range, $end_range);
>> +
>> +            # Delete temporary prefix pool
>> +            eval { nautobot_api_request($config, "DELETE", "/ipam/prefixes/$temp_prefix_id/"); };
> here as well
>
>> +        }
>> +
>> +        last if $found_ip;
>> +
>> +        # we searched the last pool prefix
>> +        last if $matcher->($current_ip->addr);
>> +
>> +        $current_ip = $current_ip->plus($increment);
>> +    }
>> +
>> +    if (!$found_ip) {
>> +        return if $noerr;
>> +        die "could not allocate ip in the range "
>> +            . $start_range->addr . " - "
>> +            . $end_range->addr
>> +            . ": $@\n";
>> +    }
>> +
>> +    $class->add_ip(
>> +        $config,
>> +        undef,
>> +        $subnet,
>> +        $found_ip,
>> +        $data->{hostname},
>> +        $data->{mac},
>> +        undef,
>> +        0,
>> +        $noerr,
>> +    );
>> +
>> +    return $found_ip;
>> +}
> we might really wanna upstream a little patch that adds at least offset
> support, or defines a start / end range. Seems like this shouldn't be
> too hard with the current implementation in nautobot. That would
> simplify this method by a lot. We could also just deny using DHCP ranges
> with the Nautobot plugin for now...
yes i thought it would be nicer if we didn't have to depend on a
special nautobot version, but i agree with you that the current
implementation is definitely more complex and therefore more
error-prone than simply having an offset parameter.
I will see if I can get the offset parameter implemented in nautobot
upstream.

> see
> https://github.com/nautobot/nautobot/blob/develop/nautobot/ipam/api/views.py#L309
>
>> +sub update_ip {
>> +    my ($class, $config, $subnetid, $subnet, $ip, $hostname, $mac, undef, $is_gateway, $noerr) = @_;
>> +
>> +    my $mask = $subnet->{mask};
>> +    my $namespace = $config->{namespace};
>> +
>> +    my $description = undef;
>> +    if ($is_gateway) {
>> +        $description = 'gateway';
>> +    } elsif ($mac) {
>> +        $description = "mac:$mac";
>> +    }
>> +
>> +    my $params = {
>> +        address => "$ip/$mask",
>> +        type => "dhcp",
>> +        description => $description,
>> +        namespace => $namespace,
>> +        status => default_ip_status(),
>> +    };
>> +
>> +    my $ip_id = get_ip_id($config, $ip, $noerr);
>> +    if (!defined($ip_id)) {
>> +        return if $noerr;
>> +        die "could not find the ip $ip in nautobot\n";
>> +    }
>> +
>> +    eval { nautobot_api_request($config, "PATCH", "/ipam/ip-addresses/$ip_id/", $params); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error updating ip $ip: $@";
>> +    }
>> +}
>> +
>> +sub del_ip {
>> +    my ($class, $config, undef, undef, $ip, $noerr) = @_;
>> +
>> +    return if !$ip;
>> +
>> +    my $ip_id = get_ip_id($config, $ip, $noerr);
>> +    if (!defined($ip_id)) {
>> +        warn("could not find the ip $ip in nautobot\n");
>> +        return;
>> +    }
>> +
>> +    eval { nautobot_api_request($config, "DELETE", "/ipam/ip-addresses/$ip_id/"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error deleting ip $ip : $@\n";
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +sub empty_subnet {
> sounds more like a check if the subnet is empty, maybe
> delete_ips_from_subnet or something is better?
Agree
>
>> +    my ($class, $config, $subnetid, $subnet, $subnetuuid, $noerr) = @_;
>> +
>> +    my $namespace = $config->{namespace};
>> +
>> +    my $response = eval {
>> +        return nautobot_api_request(
>> +            $config,
>> +            "GET",
>> +            "/ipam/ip-addresses/?namespace=$namespace&parent=$subnetuuid",
>> +        );
>> +    };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "could not find the subnet $subnet in nautobot: $@\n";
>> +    }
>> +
>> +    for my $ip (@{ $response->{results} }) {
>> +        del_ip($class, $config, undef, undef, $ip->{host}, $noerr);
>> +    }
>> +
>> +    return 1;
>> +}
>> +
>> +sub subnet_is_deletable {
>> +    my ($config, $subnetid, $subnet, $subnetuuid, $noerr) = @_;
>> +
>> +    my $namespace = $config->{namespace};
>> +
>> +    my $response = eval {
>> +        return nautobot_api_request(
>> +            $config,
>> +            "GET",
>> +            "/ipam/ip-addresses/?namespace=$namespace&parent=$subnetuuid",
>> +        );
>> +    };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error querying prefix $subnet: $@\n";
>> +    }
>> +    my $n_ips = scalar $response->{results}->@*;
>> +
>> +    # least costly check operation 1st
>> +    return 1 if ($n_ips == 0);
>> +
>> +    for my $ip (values $response->{results}->@*) {
>> +        if (!is_ip_gateway($config, $ip->{host}, $noerr)) {
> Maybe we could simply check this by comparing with the subnet
> configuration instead of querying nautobot for every IP?
yes good point.
>
>> +            # some remaining IP is not a gateway so we can't delete the subnet
>> +            return 0;
>> +        }
>> +    }
>> +    #all remaining IPs are gateways
>> +    return 1;
>> +}
>> +
>> +sub verify_api {
>> +    my ($class, $config) = @_;
>> +
>> +    my $namespace = $config->{namespace};
>> +
>> +    # check if the namespace and the status "Active" exist
>> +    eval {
>> +        get_namespace_id($config, $namespace) // die "namespace $namespace does not exist";
>> +        get_status_id($config, default_ip_status())
>> +            // die "the status " . default_ip_status() . " does not exist";
>> +    };
>> +    if ($@) {
>> +        die "could not use nautobot api: $@\n";
>> +    }
>> +}
>> +
>> +sub get_ips_from_mac {
>> +    my ($class, $config, $mac, $zone) = @_;
>> +
>> +    my $ip4 = undef;
>> +    my $ip6 = undef;
>> +
>> +    my $data = eval { nautobot_api_request($config, "GET", "/ipam/ip-addresses/?q=$mac"); };
>> +    if ($@) {
>> +        die "could not query ip address entry for mac $mac: $@";
>> +    }
>> +
>> +    for my $ip (@{ $data->{results} }) {
>> +        if ($ip->{ip_version} == 4 && !$ip4) {
>> +            ($ip4, undef) = split(/\//, $ip->{address});
>> +        }
>> +
>> +        if ($ip->{ip_version} == 6 && !$ip6) {
>> +            ($ip6, undef) = split(/\//, $ip->{address});
>> +        }
>> +    }
>> +
>> +    return ($ip4, $ip6);
>> +}
>> +
>> +sub on_update_hook {
>> +    my ($class, $config) = @_;
>> +
>> +    PVE::Network::SDN::Ipams::NautobotPlugin::verify_api($class, $config);
>> +}
>> +
>> +sub get_ip_id {
> filter for namespace below
good catch, added it to the other spots you mentioned and also  
get_ips_from_mac as it was missing there as well.

>> +    my ($config, $ip, $noerr) = @_;
>> +
>> +    my $result =
>> +        eval { return nautobot_api_request($config, "GET", "/ipam/ip-addresses/?address=$ip"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error while querying for ip $ip id: $@\n";
>> +    }
>> +
>> +    my $data = @{ $result->{results} }[0];
>> +    return $data->{id};
>> +}
>> +
>> +sub get_prefix_id {
> filter for namespace below
>
>> +    my ($config, $cidr, $noerr) = @_;
>> +
>> +    my $result =
>> +        eval { return nautobot_api_request($config, "GET", "/ipam/prefixes/?prefix=$cidr"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error while querying for cidr $cidr prefix id: $@\n";
>> +    }
>> +
>> +    my $data = @{ $result->{results} }[0];
>> +    return $data->{id};
>> +}
>> +
>> +sub get_namespace_id {
>> +    my ($config, $namespace, $noerr) = @_;
>> +
>> +    my $result =
>> +        eval { return nautobot_api_request($config, "GET", "/ipam/namespaces/?name=$namespace"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error while querying for namespace $namespace id: $@\n";
>> +    }
>> +
>> +    my $data = @{ $result->{results} }[0];
>> +    return $data->{id};
>> +}
>> +
>> +sub get_status_id {
>> +    my ($config, $status, $noerr) = @_;
>> +
>> +    my $result =
>> +        eval { return nautobot_api_request($config, "GET", "/extras/statuses/?name=$status"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error while querying for status $status id: $@\n";
>> +    }
>> +
>> +    my $data = @{ $result->{results} }[0];
>> +    return $data->{id};
>> +}
>> +
>> +sub is_ip_gateway {
> filter for namespace below
>
>> +    my ($config, $ip, $noerr) = @_;
>> +
>> +    my $result =
>> +        eval { return nautobot_api_request($config, "GET", "/ipam/ip-addresses/?address=$ip"); };
>> +    if ($@) {
>> +        return if $noerr;
>> +        die "error while checking if $ip is a gateway: $@\n";
>> +    }
>> +
>> +    my $data = @{ $result->{results} }[0];
>> +    return $data->{description} eq 'gateway';
>> +}
>> +
>> +1;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel