* [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup [not found] <20250730212614.1264010-1-trygvis@inamo.no> @ 2025-07-30 21:26 ` Trygve Laugstøl via pve-devel 2025-09-17 13:15 ` Fiona Ebner 0 siblings, 1 reply; 4+ messages in thread From: Trygve Laugstøl via pve-devel @ 2025-07-30 21:26 UTC (permalink / raw) To: pve-devel; +Cc: Trygve Laugstøl [-- Attachment #1: Type: message/rfc822, Size: 8295 bytes --] From: "Trygve Laugstøl" <trygvis@inamo.no> To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup Date: Wed, 30 Jul 2025 23:26:14 +0200 Message-ID: <20250730212614.1264010-2-trygvis@inamo.no> The problem description in #6569 is correct, but instead of depending on the freetext query parameter "q", this uses the "prefix" parameter for an explicit lookup. This also checks if there are multiple prefixes that matched. This will happen if the same prefix is registered in multiple VRFs. Signed-off-by: Trygve Laugstøl <trygvis@inamo.no> --- src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm index e118d03..3799e47 100644 --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm @@ -423,18 +423,25 @@ sub on_update_hook { sub get_prefix_id { 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") }; + # look up the prefix by matching the prefix exactly. + my $result = eval { netbox_api_request($config, "GET", "/ipam/prefixes/?prefix=$cidr") }; if ($@) { return if $noerr; die "could not obtain ID for prefix $cidr: $@"; } - my $data = @{ $result->{results} }[0]; - return $data->{id}; + # we can get multiple prefixes returned if the netbox configuration allows + # it, or if the prefix is registered in different VRFs. + my $count = $result->{count} || 0; + if ($count > 1) { + die "ambiguous prefix lookup for $cidr: found $count matches"; + } + + if ($count == 0) { + return; + } + + return $result->{results}[0]{id}; } sub get_iprange_id { -- 2.47.2 [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup 2025-07-30 21:26 ` [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup Trygve Laugstøl via pve-devel @ 2025-09-17 13:15 ` Fiona Ebner 2025-09-17 18:03 ` Trygve Laugstøl via pve-devel [not found] ` <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com> 0 siblings, 2 replies; 4+ messages in thread From: Fiona Ebner @ 2025-09-17 13:15 UTC (permalink / raw) To: Proxmox VE development discussion Sorry about the very late response and thank you for the contribution! Am 30.07.25 um 11:36 PM schrieb Trygve Laugstøl via pve-devel: > The problem description in #6569 is correct, but instead of depending on the > freetext query parameter "q", this uses the "prefix" parameter for an explicit > lookup. > > This also checks if there are multiple prefixes that matched. This will happen > if the same prefix is registered in multiple VRFs. > > Signed-off-by: Trygve Laugstøl <trygvis@inamo.no> > --- > src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm > index e118d03..3799e47 100644 > --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm > +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm > @@ -423,18 +423,25 @@ sub on_update_hook { > sub get_prefix_id { > 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") }; > + # look up the prefix by matching the prefix exactly. > + my $result = eval { netbox_api_request($config, "GET", "/ipam/prefixes/?prefix=$cidr") }; > if ($@) { > return if $noerr; > die "could not obtain ID for prefix $cidr: $@"; > } > > - my $data = @{ $result->{results} }[0]; > - return $data->{id}; > + # we can get multiple prefixes returned if the netbox configuration allows > + # it, or if the prefix is registered in different VRFs. > + my $count = $result->{count} || 0; > + if ($count > 1) { > + die "ambiguous prefix lookup for $cidr: found $count matches"; Can't this break existing setups where there are multiple prefixes? Because the old code would just pick the first, but the new code would die rather than also picking the first. If we really want this, it should honor the $noerr parameter and return instead of die if $noerr is set. > + } > + > + if ($count == 0) { > + return; > + } > + > + return $result->{results}[0]{id}; > } > > sub get_iprange_id { > -- > 2.47.2 > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup 2025-09-17 13:15 ` Fiona Ebner @ 2025-09-17 18:03 ` Trygve Laugstøl via pve-devel [not found] ` <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com> 1 sibling, 0 replies; 4+ messages in thread From: Trygve Laugstøl via pve-devel @ 2025-09-17 18:03 UTC (permalink / raw) To: Fiona Ebner, Proxmox VE development discussion; +Cc: Trygve Laugstøl [-- Attachment #1: Type: message/rfc822, Size: 9617 bytes --] From: "Trygve Laugstøl" <trygvis@inamo.no> To: "Fiona Ebner" <f.ebner@proxmox.com>, "Proxmox VE development discussion" <pve-devel@lists.proxmox.com> Subject: Re: [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup Date: Wed, 17 Sep 2025 20:03:25 +0200 Message-ID: <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com> On Wed, Sep 17, 2025, at 15:15, Fiona Ebner wrote: > Sorry about the very late response and thank you for the contribution! > > Am 30.07.25 um 11:36 PM schrieb Trygve Laugstøl via pve-devel: >> The problem description in #6569 is correct, but instead of depending on the >> freetext query parameter "q", this uses the "prefix" parameter for an explicit >> lookup. >> >> This also checks if there are multiple prefixes that matched. This will happen >> if the same prefix is registered in multiple VRFs. >> >> Signed-off-by: Trygve Laugstøl <trygvis@inamo.no> >> --- >> src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >> index e118d03..3799e47 100644 >> --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >> +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >> @@ -423,18 +423,25 @@ sub on_update_hook { >> sub get_prefix_id { >> 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") }; >> + # look up the prefix by matching the prefix exactly. >> + my $result = eval { netbox_api_request($config, "GET", "/ipam/prefixes/?prefix=$cidr") }; >> if ($@) { >> return if $noerr; >> die "could not obtain ID for prefix $cidr: $@"; >> } >> >> - my $data = @{ $result->{results} }[0]; >> - return $data->{id}; >> + # we can get multiple prefixes returned if the netbox configuration allows >> + # it, or if the prefix is registered in different VRFs. >> + my $count = $result->{count} || 0; >> + if ($count > 1) { >> + die "ambiguous prefix lookup for $cidr: found $count matches"; > > Can't this break existing setups where there are multiple prefixes? > Because the old code would just pick the first, but the new code would > die rather than also picking the first. > > If we really want this, it should honor the $noerr parameter and return > instead of die if $noerr is set. The current one would pick the first, but also a random, inconsistent one. A better solution here would be to somehow include the VRF as a part of the lookup, but that requires a bigger expansion of the Netbox support than I'm prepared to do. Also, the code doesn't handle missing prefixes well so if the prefix is removed on the Netbox side the current code will just not allow you to remove the subnet at all. But I guess that is another issue. -- Trygve >> + } >> + >> + if ($count == 0) { >> + return; >> + } >> + >> + return $result->{results}[0]{id}; >> } >> >> sub get_iprange_id { >> -- >> 2.47.2 >> >> [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com>]
* Re: [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup [not found] ` <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com> @ 2025-09-18 10:07 ` Fiona Ebner 0 siblings, 0 replies; 4+ messages in thread From: Fiona Ebner @ 2025-09-18 10:07 UTC (permalink / raw) To: Trygve Laugstøl, Proxmox VE development discussion, Stefan Hanreich Am 17.09.25 um 8:03 PM schrieb Trygve Laugstøl: > On Wed, Sep 17, 2025, at 15:15, Fiona Ebner wrote: >> Sorry about the very late response and thank you for the contribution! >> >> Am 30.07.25 um 11:36 PM schrieb Trygve Laugstøl via pve-devel: >>> The problem description in #6569 is correct, but instead of depending on the >>> freetext query parameter "q", this uses the "prefix" parameter for an explicit >>> lookup. >>> >>> This also checks if there are multiple prefixes that matched. This will happen >>> if the same prefix is registered in multiple VRFs. >>> >>> Signed-off-by: Trygve Laugstøl <trygvis@inamo.no> >>> --- >>> src/PVE/Network/SDN/Ipams/NetboxPlugin.pm | 21 ++++++++++++++------- >>> 1 file changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >>> index e118d03..3799e47 100644 >>> --- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >>> +++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm >>> @@ -423,18 +423,25 @@ sub on_update_hook { >>> sub get_prefix_id { >>> 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") }; >>> + # look up the prefix by matching the prefix exactly. >>> + my $result = eval { netbox_api_request($config, "GET", "/ipam/prefixes/?prefix=$cidr") }; >>> if ($@) { >>> return if $noerr; >>> die "could not obtain ID for prefix $cidr: $@"; >>> } >>> >>> - my $data = @{ $result->{results} }[0]; >>> - return $data->{id}; >>> + # we can get multiple prefixes returned if the netbox configuration allows >>> + # it, or if the prefix is registered in different VRFs. >>> + my $count = $result->{count} || 0; >>> + if ($count > 1) { >>> + die "ambiguous prefix lookup for $cidr: found $count matches"; >> >> Can't this break existing setups where there are multiple prefixes? >> Because the old code would just pick the first, but the new code would >> die rather than also picking the first. >> >> If we really want this, it should honor the $noerr parameter and return >> instead of die if $noerr is set. > > The current one would pick the first, but also a random, inconsistent one. A better solution here would be to somehow include the VRF as a part of the lookup, but that requires a bigger expansion of the Netbox support than I'm prepared to do. Okay, I hoped the API result from Netbox would be ordered or consistent somehow. But still, e.g. add_next_freeip() would previously succeed and now will fail if there are multiple prefixes, which can break an existing setup. Should we rather just warn about that case instead of using "die"? But let's wait for @Stefan's opinion :) > Also, the code doesn't handle missing prefixes well so if the prefix is removed on the Netbox side the current code will just not allow you to remove the subnet at all. But I guess that is another issue. Yes, that's orthogonal. Best Regards, Fiona _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-18 10:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20250730212614.1264010-1-trygvis@inamo.no>
2025-07-30 21:26 ` [pve-devel] [PATCH pve-network 1/1] fix #6569: ipam: netbox: better prefix lookup Trygve Laugstøl via pve-devel
2025-09-17 13:15 ` Fiona Ebner
2025-09-17 18:03 ` Trygve Laugstøl via pve-devel
[not found] ` <b77fd4bf-7045-47c4-bed8-5d450cb61187@app.fastmail.com>
2025-09-18 10:07 ` Fiona Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox