[pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

[pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

[Lou Lecrivain via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 5040 bytes --]

From: Lou Lecrivain <lou.lecrivain@wdz.de>
To: pve-devel@lists.proxmox.com
Subject: SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
Date: Wed, 27 Nov 2024 17:17:47 +0100
Message-ID: <20241127161803.8866-1-lou.lecrivain@wdz.de>

This patch series introduce support for Nautobot as an IPAM.

Please note that:
- Some stuff could still be refined, in my opinion.
  I would be happy to receive some comments, as I'm a
  Perl beginner.
- Test code has not been written *yet*.
- Nautobot itself has some restrictions, for instance
  it does not support IP ranges. So I had to hack
  around this a bit. I'd be happy to only do prefix
  allocations, if that's possible.

MfG
--
Lou Lecrivain, WDZ GmbH

Lou Lecrivain (16):
  ipam: nautobot support initial commit
  ipam: add Nautobot plugin sources to Makefile
  ipam: change verify URL (now common to Nautobot and Netbox)
  ipam: nautobot: add verification for ipam namespace plugin parameter
  ipam: nautobot: fix on_update_hook for NautobotPlugin
  ipam: nautobot: add default active status check
  ipam: nautobot: fix typo
  ipam: nautobot: 1st working "add subnet" and "add ip"
  ipam: nautobot: simplify query
  ipam: nautobot: api endpoint change no longer needed
  ipam: nautobot: add update_ip sub
  ipam: nautobot: add get ips for mac function
  ipam: nautobot: 1st draft for allocating ip in dhcp range
  ipam: nautobot: hacky ip range support
  ipam: nautobot: implement plain prefix allocation (without ranges)
  ipam: nautobot: add word of warning for dhcp range support

 src/PVE/API2/Network/SDN/Ipams.pm           |   1 +
 src/PVE/Network/SDN/Ipams.pm                |   3 +
 src/PVE/Network/SDN/Ipams/Makefile          |   2 +-
 src/PVE/Network/SDN/Ipams/NautobotPlugin.pm | 263 ++++++++++++++++++++
 4 files changed, 268 insertions(+), 1 deletion(-)
 create mode 100644 src/PVE/Network/SDN/Ipams/NautobotPlugin.pm

-- 
2.39.5



[-- 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

Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

[Hannes Dürr]

Thanks for contributing and sending the patch series, we really 
appreciate it!

At first glance it looks quite good, I have a few suggestions for changes:

*  The plugin is based on the Netbox plugin, I would suggest changing it
     to the base plugin. I know Nautobot is a fork of Netbox, but this
     dependency doesn't make things easier in my eyes, unless I'm missing
     something, please let me know.

*  The commit history is currently a bit unnecessarily long and does not
     build up well. What I mean is:
     - in 1/12 you build the plugin and copy/paste some stuff into it
     - in 3/12 you delete everything for now
     -> just leave it out
     or
     - in 6/12 you introduce a typo in an url
     - in 7/12 you fix the typo
     Such changes don't need to be committed in the first place,
     so you don't need to fix them 2 commits later.

*  The commit messages could be a bit longer and more explanatory for
     coming changes: e.g. “api endpoint change no longer needed” -> Why is
     the endpoint change no longer needed? Is there more background
     information on this, e.g. a link?

*  Comments can be helpful to make complex code easier to understand
     after a long time. Comments like '#helper' or '#impl' are not really
     necessary in my opinion. I know our codebase already contains such
     comments elsewhere, but I think we can leave them out here :)

*  Apart from the tests, the documentation (pve-docs/pvesdn.adoc) is also
     missing.

On 11/27/24 17:17, Lou Lecrivain via pve-devel wrote:
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


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

Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

[Lou Lecrivain via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 15642 bytes --]

From: <Lou.Lecrivain@wdz.de>
To: <h.duerr@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: RE: Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
Date: Tue, 10 Dec 2024 13:19:41 +0000
Message-ID: <BE1P281MB2116C1CA8BA34E5C86FA59D9853D2@BE1P281MB2116.DEUP281.PROD.OUTLOOK.COM>

Hello Hannes,

Thank you also very much for the feedback, I appreciate it. It's my first time writing Perl and contributing to Proxmox :) I was initially in contact with your colleague S. Hanreich, who told me to start with the Netbox plugin. Given how significant the differences are in the end, I guess it would be easier to build from the base plugin, indeed.

I'll make the necessary changes (code + test + documentation) plus squash and edit a few of these commits and will send you a v2 as soon as possible.

I just have a few questions that I'd like an answer to, namely:

- Is the IPAM IP range support mandatory? Nautobot doesn't support IP ranges natively, only prefix allocations. So far, I have a workaround but it's not really clean... What would be best? ditch the workaround and "die", or keep it?

- We also have a noop/early return in del_subnet, inherited from Netbox Plugin (doesn't actually delete the subnet in the IPAM). Do we agree that the desired operation would to be to error if subnet is not empty? cf:
https://git.proxmox.com/?p=pve-network.git;a=blame;f=PVE/Network/SDN/Ipams/NetboxPlugin.pm;hb=70b035064290a014759ce62e0093df00cd7d62fe#l69

MfG
--
Lou Lecrivain
WDZ GmbH

________________________________________
De : Hannes Dürr <h.duerr@proxmox.com>
Envoyé : mardi 10 décembre 2024 10:32
À : Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc : Lecrivain, Lou (WDZ) <Lou.Lecrivain@wdz.de>
Objet : [!!ACHTUNG extern!!] - Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
 
Thanks for contributing and sending the patch series, we really
appreciate it!

At first glance it looks quite good, I have a few suggestions for changes:

*  The plugin is based on the Netbox plugin, I would suggest changing it
     to the base plugin. I know Nautobot is a fork of Netbox, but this
     dependency doesn't make things easier in my eyes, unless I'm missing
     something, please let me know.

*  The commit history is currently a bit unnecessarily long and does not
     build up well. What I mean is:
     - in 1/12 you build the plugin and copy/paste some stuff into it
     - in 3/12 you delete everything for now
     -> just leave it out
     or
     - in 6/12 you introduce a typo in an url
     - in 7/12 you fix the typo
     Such changes don't need to be committed in the first place,
     so you don't need to fix them 2 commits later.

*  The commit messages could be a bit longer and more explanatory for
     coming changes: e.g. “api endpoint change no longer needed” -> Why is
     the endpoint change no longer needed? Is there more background
     information on this, e.g. a link?

*  Comments can be helpful to make complex code easier to understand
     after a long time. Comments like '#helper' or '#impl' are not really
     necessary in my opinion. I know our codebase already contains such
     comments elsewhere, but I think we can leave them out here :)

*  Apart from the tests, the documentation (pve-docs/pvesdn.adoc) is also
     missing.

On 11/27/24 17:17, Lou Lecrivain via pve-devel wrote:
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


[-- 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

Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM

[Hannes Dürr]

On 12/10/24 14:19, Lou.Lecrivain@wdz.de wrote:
> Hello Hannes,
>
> Thank you also very much for the feedback, I appreciate it. It's my first time writing Perl and contributing to Proxmox :) I was initially in contact with your colleague S. Hanreich, who told me to start with the Netbox plugin. Given how significant the differences are in the end, I guess it would be easier to build from the base plugin, indeed.
>
> I'll make the necessary changes (code + test + documentation) plus squash and edit a few of these commits and will send you a v2 as soon as possible.
>
> I just have a few questions that I'd like an answer to, namely:
>
> - Is the IPAM IP range support mandatory? Nautobot doesn't support IP ranges natively, only prefix allocations. So far, I have a workaround but it's not really clean... What would be best? ditch the workaround and "die", or keep it?
We have discussed this internally and would like to see IP range support 
implemented as well. It is a pity that the nautobot API does not provide 
a specific endpoint for this, but your workaround seems good enough.
> - We also have a noop/early return in del_subnet, inherited from Netbox Plugin (doesn't actually delete the subnet in the IPAM). Do we agree that the desired operation would to be to error if subnet is not empty? cf:
> https://git.proxmox.com/?p=pve-network.git;a=blame;f=PVE/Network/SDN/Ipams/NetboxPlugin.pm;hb=70b035064290a014759ce62e0093df00cd7d62fe#l69
IMO yes, i'd prefer some error message telling me that it was not 
possible to remove the Subnet or IP (currently the same behavior) 
instead of just not deleting it.
> MfG
> --
> Lou Lecrivain
> WDZ GmbH
>
> ________________________________________
> De : Hannes Dürr<h.duerr@proxmox.com>
> Envoyé : mardi 10 décembre 2024 10:32
> À : Proxmox VE development discussion<pve-devel@lists.proxmox.com>
> Cc : Lecrivain, Lou (WDZ)<Lou.Lecrivain@wdz.de>
> Objet : [!!ACHTUNG extern!!] - Re: [pve-devel] SPAM: [PATCH pve-network 00/16] add support for Nautobot IPAM
>   
> Thanks for contributing and sending the patch series, we really
> appreciate it!
>
> At first glance it looks quite good, I have a few suggestions for changes:
>
> *  The plugin is based on the Netbox plugin, I would suggest changing it
>       to the base plugin. I know Nautobot is a fork of Netbox, but this
>       dependency doesn't make things easier in my eyes, unless I'm missing
>       something, please let me know.
>
> *  The commit history is currently a bit unnecessarily long and does not
>       build up well. What I mean is:
>       - in 1/12 you build the plugin and copy/paste some stuff into it
>       - in 3/12 you delete everything for now
>       -> just leave it out
>       or
>       - in 6/12 you introduce a typo in an url
>       - in 7/12 you fix the typo
>       Such changes don't need to be committed in the first place,
>       so you don't need to fix them 2 commits later.
>
> *  The commit messages could be a bit longer and more explanatory for
>       coming changes: e.g. “api endpoint change no longer needed” -> Why is
>       the endpoint change no longer needed? Is there more background
>       information on this, e.g. a link?
>
> *  Comments can be helpful to make complex code easier to understand
>       after a long time. Comments like '#helper' or '#impl' are not really
>       necessary in my opinion. I know our codebase already contains such
>       comments elsewhere, but I think we can leave them out here :)
>
> *  Apart from the tests, the documentation (pve-docs/pvesdn.adoc) is also
>       missing.
>
> On 11/27/24 17:17, Lou Lecrivain via pve-devel wrote:
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


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