From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 977121FF162
for <inbox@lore.proxmox.com>; Mon, 7 Apr 2025 17:33:17 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 321113407;
Mon, 7 Apr 2025 17:33:14 +0200 (CEST)
Message-ID: <85e475d1-d0de-4811-b158-874069768e48@proxmox.com>
Date: Mon, 7 Apr 2025 17:32:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Stefan Hanreich <s.hanreich@proxmox.com>
References: <20250310085103.30549-1-s.hanreich@proxmox.com>
Content-Language: en-US
From: Hannes Duerr <h.duerr@proxmox.com>
In-Reply-To: <20250310085103.30549-1-s.hanreich@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.967 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
KAM_MAILER 2 Automated Mailer Tag Left in Email
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-network v2 1/8] ipam: netbox: factor out
common api methods and unify error handling
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Tested this series against netbox version 4.2.2 and didn't run into any
issues.
Created and deleted a simple zone with two vnets and respective subnet.
The prefixes were created and deleted correctly
Assigned and removed VMs to the vnets and the ip addresses were created
and removed correctly.
On 3/10/25 09:50, Stefan Hanreich wrote:
> Create a helper method that abstracts the common code used in making
> netbox requests. Move all api_request incovations over to using the
> helper method. This saves us from writing lots of repeated code.
>
> This also updates the helpers and introduces error checking there.
> Helpers didn't catch any errors and the invoking methods didn't as
> well. This meant that functions with $noerr set to 1 would still error
> out. We now pass $noerr to the helper functions and they behave the
> same as the parent methods. This requires some additional checks in
> the call sites of the helpers.
>
> Also canonicalize all URLs, since Netbox does that and it saves us a
> redirect.
>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> Changes from v1 to v2:
> * removed trailing slashes from some query strings
>
> src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 241 +++++++++++-----------
> 1 file changed, 126 insertions(+), 115 deletions(-)
>
> diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
> index 18089b7..56a1787 100644
> --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
> +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
> @@ -25,6 +25,21 @@ sub options {
> };
> }
>
> +sub netbox_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}"
> + ],
> + $params,
> + $config->{fingerprint},
> + );
> +}
> +
> # Plugin implementation
>
> sub add_subnet {
> @@ -32,62 +47,42 @@ sub add_subnet {
>
> my $cidr = $subnet->{cidr};
> my $gateway = $subnet->{gateway};
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $fingerprint = $plugin_config->{fingerprint};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> -
> - my $internalid = get_prefix_id($url, $cidr, $headers, $fingerprint);
>
> - #create subnet
> - if (!$internalid) {
> -
> - my $params = { prefix => $cidr };
> + if (get_prefix_id($plugin_config, $cidr, $noerr)) {
> + return if $noerr;
> + die "prefix $cidr already exists in netbox";
> + }
>
> - eval {
> - my $result = PVE::Network::SDN::api_request(
> - "POST", "$url/ipam/prefixes/", $headers, $params, $fingerprint );
> - };
> - if ($@) {
> - die "error add subnet to ipam: $@" if !$noerr;
> - }
> + eval {
> + netbox_api_request($plugin_config, "POST", "/ipam/prefixes/", {
> + prefix => $cidr
> + });
> + };
> + if ($@) {
> + return if $noerr;
> + die "error adding subnet to ipam: $@";
> }
> -
> }
>
> sub del_subnet {
> my ($class, $plugin_config, $subnetid, $subnet, $noerr) = @_;
>
> my $cidr = $subnet->{cidr};
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $gateway = $subnet->{gateway};
> - my $fingerprint = $plugin_config->{fingerprint};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
>
> - my $internalid = get_prefix_id($url, $cidr, $headers, $fingerprint);
> - return if !$internalid;
> + my $internalid = get_prefix_id($plugin_config, $cidr, $noerr);
>
> return; #fixme: check that prefix is empty exluding gateway, before delete
>
> eval {
> - PVE::Network::SDN::api_request(
> - "DELETE", "$url/ipam/prefixes/$internalid/", $headers, undef, $fingerprint);
> + netbox_api_request($plugin_config, "DELETE", "/ipam/prefixes/$internalid/");
> };
> - if ($@) {
> - die "error deleting subnet from ipam: $@" if !$noerr;
> - }
> -
> + die "error deleting subnet from ipam: $@" if $@ && !$noerr;
> }
>
> sub add_ip {
> my ($class, $plugin_config, $subnetid, $subnet, $ip, $hostname, $mac, $vmid, $is_gateway, $noerr) = @_;
>
> my $mask = $subnet->{mask};
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $fingerprint = $plugin_config->{fingerprint};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
>
> my $description = undef;
> if ($is_gateway) {
> @@ -96,18 +91,18 @@ sub add_ip {
> $description = "mac:$mac";
> }
>
> - my $params = { address => "$ip/$mask", dns_name => $hostname, description => $description };
> -
> eval {
> - PVE::Network::SDN::api_request(
> - "POST", "$url/ipam/ip-addresses/", $headers, $params, $fingerprint);
> + netbox_api_request($plugin_config, "POST", "/ipam/ip-addresses/", {
> + address => "$ip/$mask",
> + dns_name => $hostname,
> + description => $description,
> + });
> };
>
> if ($@) {
> if ($is_gateway) {
> - if (!is_ip_gateway($url, $ip, $headers, $fingerprint) && !$noerr) {
> - die "error add subnet ip to ipam: ip $ip already exist: $@";
> - }
> + die "error add subnet ip to ipam: ip $ip already exist: $@"
> + if !is_ip_gateway($plugin_config, $ip, $noerr);
> } elsif (!$noerr) {
> die "error add subnet ip to ipam: ip already exist: $@";
> }
> @@ -118,11 +113,6 @@ sub update_ip {
> my ($class, $plugin_config, $subnetid, $subnet, $ip, $hostname, $mac, $vmid, $is_gateway, $noerr) = @_;
>
> my $mask = $subnet->{mask};
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $section = $plugin_config->{section};
> - my $fingerprint = $plugin_config->{fingerprint};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
>
> my $description = undef;
> if ($is_gateway) {
> @@ -131,14 +121,20 @@ sub update_ip {
> $description = "mac:$mac";
> }
>
> - my $params = { address => "$ip/$mask", dns_name => $hostname, description => $description };
> + my $ip_id = get_ip_id($plugin_config, $ip, $noerr);
>
> - my $ip_id = get_ip_id($url, $ip, $headers, $fingerprint);
> - die "can't find ip $ip in ipam" if !$ip_id;
> + # definedness check, because ID could be 0
> + if (!defined($ip_id)) {
> + return if $noerr;
> + die "could not find id for ip address $ip";
> + }
>
> eval {
> - PVE::Network::SDN::api_request(
> - "PATCH", "$url/ipam/ip-addresses/$ip_id/", $headers, $params, $fingerprint);
> + netbox_api_request($plugin_config, "PATCH", "/ipam/ip-addresses/$ip_id/", {
> + address => "$ip/$mask",
> + dns_name => $hostname,
> + description => $description,
> + });
> };
> if ($@) {
> die "error update ip $ip : $@" if !$noerr;
> @@ -150,20 +146,22 @@ sub add_next_freeip {
>
> my $cidr = $subnet->{cidr};
>
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $fingerprint = $plugin_config->{fingerprint};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> + my $internalid = get_prefix_id($plugin_config, $cidr, $noerr);
>
> - my $internalid = get_prefix_id($url, $cidr, $headers, $fingerprint);
> + # definedness check, because ID could be 0
> + if (!defined($internalid)) {
> + return if $noerr;
> + die "could not find id for prefix $cidr";
> + }
>
> my $description = "mac:$mac" if $mac;
>
> - my $params = { dns_name => $hostname, description => $description };
> -
> eval {
> - my $result = PVE::Network::SDN::api_request(
> - "POST", "$url/ipam/prefixes/$internalid/available-ips/", $headers, $params, $fingerprint);
> + my $result = netbox_api_request($plugin_config, "POST", "/ipam/prefixes/$internalid/available-ips/", {
> + dns_name => $hostname,
> + description => $description,
> + });
> +
> my ($ip, undef) = split(/\//, $result->{address});
> return $ip;
> };
> @@ -176,19 +174,22 @@ sub add_next_freeip {
> sub add_range_next_freeip {
> my ($class, $plugin_config, $subnet, $range, $data, $noerr) = @_;
>
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> - my $fingerprint = $plugin_config->{fingerprint};
> + my $internalid = get_iprange_id($plugin_config, $range, $noerr);
>
> - my $internalid = get_iprange_id($url, $range, $headers, $fingerprint);
> - my $description = "mac:$data->{mac}" if $data->{mac};
> + # definedness check, because ID could be 0
> + if (!defined($internalid)) {
> + return if $noerr;
> + die "could not find id for ip range $range->{'start-address'}:$range->{'end-address'}";
> + }
>
> - my $params = { dns_name => $data->{hostname}, description => $description };
> + my $description = "mac:$data->{mac}" if $data->{mac};
>
> eval {
> - my $result = PVE::Network::SDN::api_request(
> - "POST", "$url/ipam/ip-ranges/$internalid/available-ips/", $headers, $params, $fingerprint);
> + my $result = netbox_api_request($plugin_config, "POST", "/ipam/ip-ranges/$internalid/available-ips/", {
> + dns_name => $data->{hostname},
> + description => $description,
> + });
> +
> my ($ip, undef) = split(/\//, $result->{address});
> print "found ip free $ip in range $range->{'start-address'}-$range->{'end-address'}\n" if $ip;
> return $ip;
> @@ -204,16 +205,14 @@ sub del_ip {
>
> return if !$ip;
>
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> - my $fingerprint = $plugin_config->{fingerprint};
> -
> - my $ip_id = get_ip_id($url, $ip, $headers, $fingerprint);
> - die "can't find ip $ip in ipam" if !$ip_id;
> + my $ip_id = get_ip_id($plugin_config, $ip, $noerr);
> + if (!defined($ip_id)) {
> + warn "could not find id for ip $ip";
> + return;
> + }
>
> eval {
> - PVE::Network::SDN::api_request("DELETE", "$url/ipam/ip-addresses/$ip_id/", $headers, undef, $fingerprint);
> + netbox_api_request($plugin_config, "DELETE", "/ipam/ip-addresses/$ip_id/");
> };
> if ($@) {
> die "error delete ip $ip : $@" if !$noerr;
> @@ -221,18 +220,14 @@ sub del_ip {
> }
>
> sub get_ips_from_mac {
> - my ($class, $plugin_config, $mac, $zoneid) = @_;
> -
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> - my $fingerprint = $plugin_config->{fingerprint};
> + my ($class, $plugin_config, $mac, $zoneid, $noerr) = @_;
>
> my $ip4 = undef;
> my $ip6 = undef;
>
> - my $data = PVE::Network::SDN::api_request(
> - "GET", "$url/ipam/ip-addresses/?description__ic=$mac", $headers, undef, $fingerprint);
> + my $data = eval {
> + netbox_api_request($plugin_config, "GET", "/ipam/ip-addresses/?description__ic=$mac");
> + };
>
> for my $ip (@{$data->{results}}) {
> if ($ip->{family}->{value} == 4 && !$ip4) {
> @@ -247,18 +242,10 @@ sub get_ips_from_mac {
> return ($ip4, $ip6);
> }
>
> -
> sub verify_api {
> my ($class, $plugin_config) = @_;
>
> - my $url = $plugin_config->{url};
> - my $token = $plugin_config->{token};
> - my $headers = ['Content-Type' => 'application/json; charset=UTF-8', 'Authorization' => "token $token"];
> - my $fingerprint = $plugin_config->{fingerprint};
> -
> - eval {
> - PVE::Network::SDN::api_request("GET", "$url/ipam/aggregates/", $headers, undef, $fingerprint);
> - };
> + eval { netbox_api_request($plugin_config, "GET", "/ipam/aggregates/"); };
> if ($@) {
> die "Can't connect to netbox api: $@";
> }
> @@ -266,47 +253,71 @@ sub verify_api {
>
> sub on_update_hook {
> my ($class, $plugin_config) = @_;
> -
> - PVE::Network::SDN::Ipams::NetboxPlugin::verify_api($class, $plugin_config);
> + verify_api($class, $plugin_config);
> }
>
> -#helpers
> -
> +# helpers
> sub get_prefix_id {
> - my ($url, $cidr, $headers, $fingerprint) = @_;
> - my $result = PVE::Network::SDN::api_request(
> - "GET", "$url/ipam/prefixes/?q=$cidr", $headers, undef, $fingerprint);
> + my ($config, $cidr, $noerr) = @_;
> +
> + # we need to supply any IP inside the prefix, without supplying the mask, so
> + # just take the one from the cidr
> + my ($ip, undef) = split(/\//, $cidr);
> +
> + my $result = eval { netbox_api_request($config, "GET", "/ipam/prefixes/?q=$ip") };
> + if ($@) {
> + return if $noerr;
> + die "could not obtain ID for prefix $cidr: $@";
> + }
> +
> my $data = @{$result->{results}}[0];
> my $internalid = $data->{id};
> return $internalid;
> }
>
> sub get_iprange_id {
> - my ($url, $range, $headers, $fingerprint) = @_;
> - my $result = PVE::Network::SDN::api_request(
> - "GET",
> - "$url/ipam/ip-ranges/?start_address=$range->{'start-address'}&end_address=$range->{'end-address'}",
> - $headers,
> - undef,
> - $fingerprint
> - );
> + my ($config, $range, $noerr) = @_;
> +
> + my $result = eval {
> + netbox_api_request(
> + $config,
> + "GET",
> + "/ipam/ip-ranges/?start_address=$range->{'start-address'}&end_address=$range->{'end-address'}",
> + );
> + };
> + if ($@) {
> + return if $noerr;
> + die "could not obtain ID for IP range $range->{'start-address'}:$range->{'end-address'}: $@";
> + }
> +
> my $data = @{$result->{results}}[0];
> my $internalid = $data->{id};
> return $internalid;
> }
>
> sub get_ip_id {
> - my ($url, $ip, $headers, $fingerprint) = @_;
> - my $result = PVE::Network::SDN::api_request(
> - "GET", "$url/ipam/ip-addresses/?q=$ip", $headers, undef, $fingerprint);
> + my ($config, $ip, $noerr) = @_;
> +
> + my $result = eval { netbox_api_request($config, "GET", "/ipam/ip-addresses/?q=$ip") };
> + if ($@) {
> + return if $noerr;
> + die "could not obtain ID for IP $ip: $@";
> + }
> +
> my $data = @{$result->{results}}[0];
> my $ip_id = $data->{id};
> return $ip_id;
> }
>
> sub is_ip_gateway {
> - my ($url, $ip, $headers, $fingerprint) = @_;
> - my $result = PVE::Network::SDN::api_request("GET", "$url/ipam/ip-addresses/?q=$ip", $headers, undef, $fingerprint);
> + my ($config, $ip, $noerr) = @_;
> +
> + my $result = eval { netbox_api_request($config, "GET", "/ipam/ip-addresses/?q=$ip") };
> + if ($@) {
> + return if $noerr;
> + die "could not obtain ipam entry for address $ip: $@";
> + }
> +
> my $data = @{$result->{data}}[0];
> my $description = $data->{description};
> my $is_gateway = 1 if $description eq 'gateway';
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel