Introduce experimental IPAM-DNS coupling feature #68
Conversation
Three new custom fields are added on IP Address : - name, - zone, - dns_record. Through a middleware and signals, all IP Address creation and modification are intercepted. Using the new custom fields, the corresponding DNS record is created, modified or deleted accordingly. Permission on DNS records are enforced. The IP address "dns_name" field is also updated.
|
Hi @jean1, good to see you're back again, and I hope you are well. Would you mind rebasing on the |
|
Sorry for the confusion, I'm just trying to figure out how to make GitHub run the checks without having to approve it every time, but it seems that that only works after the first pull request is merged. I'll build a test release once everything is green (which it currently seems to be) and try it out as soon as I can. I just ask for a little patience as I'm currently in a pretty time-consuming phase in a project - should be finished mid-October. I'm really looking forward to see how it works and maybe adjust my GUI 'integration' (or rather, 'related records' functionality) with IPAM so both together make sense - having two different IPAM integration tools would be pretty confusing. |
|
Hi @jean1, I started testing a bit and found some issues, some of them cosmetic and some related to data integrity. I'll start writing them down here as I go so you can look into them.
That's it for today ... when I find more I'll keep you updated. Again, thanks for your contribution, it already looks pretty useful to me! |
|
As an afterthought ... we should be able to get away without the 'DNS Record' custom field altogether by extending the model for records with a That would also get rid of the read-only dynamic field that looks a bit strange on the IPAddress view without losing any functionality and the need to clean it up when something is done with the zone. |
[cosmetic] The name of the custom field in the GUI is "Dns record", which looks ugly. Define a label "DNS Record" which would improve the look and make it consistent with NetBox DNS
[cosmetic] The name of the custom field in the GUI is "dns_record", which looks ugly. Add a label "DNS Record" to improve the look and make it consistent with NetBox DNS
[cosmetic] Put custom fields in the "DNS" group to keep all NetBox DNS related CFs neatly grouped together.
When a DNS zone is renamed, and an (IPAM) IP address is linked to a "A" or "AAAA" DNS record from this zone, the dns_name field is updated. When a DNS zone is deleted, and an (IPAM) IP address is linked to a "A" or "AAAA" DNS record from this zone, the dns_name field and custom field "name" are emptied.
When a zone is renamed, and an (IPAM) IP address is linked to a "A" or "AAAA" DNS record from this zone, the dns_name field is updated. When a zone is deleted, and an (IPAM) IP address is linked to a "A" or "AAAA" DNS record from this zone, the dns_name field and custom field "name" are emptied.
When an IP address is created, and the DNS coupling is requested, if the given name and zone match an existing and unmanaged DNS record, the record is linked to the IP address and becomes managed. 0) DNS Record "R" pre-exists and has the following characteristics: id=1, name=foo, zone=example.com, type=A, value=10.0.0.1, managed=False 1) IP address, with address=10.0.0.1/24, cf_name=foo, cf_zone=example.com is requested for creation. 2) The IP address is created and points to the DNS record "R": address=10.0.0.1/24, cf_dns_record=1 ... 3) The DNS Record R becomes managed: id=1, ... , managed=True
|
Hi @jean1, I'm not so sure about 6119a7b. Take the following scenario:
It's at least a matter of taste if that should happen or not. On one side it might make sense to remove the address record when the IP address gets deleted, but on the other hand the address record has been there before and the process of adding and then removing an IP address should not have side effects such as the disappearance of an existing address record, which is somewhat unexpected at least. I thought a bit about how that could be handled, and my general idea mentioned above - removing the custom field from the
We could make it an option setting to remove the record as well, but generally I would prefer creating and removing IP addresses to be complementary actions without side effects on DNS data. There is also another scenario that would be made easier to handle with that approach: Modifying an IP address.
The difference between being a managed record or not IMHO should be that managed records can be created, changed or deleted as necessary by NetBox DNS, while unmanaged records are never touched. Another edge case comes to my mind: What if there are several address records in the same zone with the same name pointing at the same address? Sounds stupid, possibly is, but it's perfectly legal according to the RFCs - there is nothing stopping you from doing it. There might even be an application for this ... In this case, round robin resolution would return 10.0.0.1 for 50% of the queries, 10.0.0.2 for 33.3% and 10.0.0.3 for 16.6% - poor man's load balancing. I wouldn't want to interfere with that kind of mechanism by arbitrarily picking out and removing address records from it - which would invariably be the case if we implemented 6119a7b. Having an |
A new field was added to the Record model; it points to an IP Address in the ipam module. The coupling logic is now easier to handle for certain cases. The dns_record custom field in ipam.IPAddress was removed.
|
Hi @jean1, thanks! Small issues first (not really a problem, but we should keep it in mind):
You can fix it pretty easily: (and what I learned from the earlier case I fell down the same hole is not only to test against the latest NetBox release, but also against the oldest supported one. I'll probably adjust the workflows to do that in the near future). Now for some real testing ... stay tuned! |
"makemigrations" generated a dependency on a too recent version of Netbox. Fixed to keep compatibility with older supported versions of netbox.
|
Hi @jean1, I've just started testing you new code. One thing I noticed right now (by making a mistake, but that's how you notice that kind of thing): I forgot to enable your experimental feature, set up a couple of IP addresses filling in the custom fields, and then wondered why 'DNS Name' was not filled in and there were no address records ... The problem is that the migration creates the CFs, and it does that whether or not the feature is enabled. I don't think we should do that, as it creates side effects that will irritate users who do not want the feature enabled. I'm not sure whether we can hide or show CFs depending on the config setting (which would be a pretty elegant way to handle this), but if we can't I think we should move the code creating the CFs from the migration to a management command and document that it needs to be done in order to use the integration feature. What do you think? |
|
So far (haven't had the time to test thoroughly and systematically) this looks very good and I think we can merge soon - this is really a step forward! I couldn't on first sight find the reason for your FIXME-comment in I'll do some more testing, but as I said I think this might be a merge candidate. Do you want to squash commits and migrations yourself or would you prefer me to do that? It doesn't make sense to keep 16 commits in history, and the two migrations are not necessay either (even more so as the later one reverts part of its predecessor). Anyway - thanks for your contribution, this is really awesome! |
A management command is provided (setup_coupling) to add or remove the custom fields needed for coupling.
|
Hello Peter,
On Wed, Oct 11, 2023 at 12:47:20PM -0700, Peter Eckel wrote:
So far (haven't had the time to test thoroughly and systematically) this looks very good and I think we can merge soon - this is really a step forward!
I couldn't on first sight find the reason for your FIXME-comment in `middleware.py` - what do you think needs to be taken care of? I created, deleted and changed names and zones for IP addresses back and forth a couple of times, and so far the results look very reasonable to me, especially when I consider the 'experimental' status of the feature.
Removed the FIXME comment, it was not meant to be here anymore.
I'll do some more testing, but as I said I think this might be a merge candidate. Do you want to squash commits and migrations yourself or would you prefer me to do that? It doesn't make sense to keep 16 commits in history, and the two migrations are not necessay either (even more so as the later one reverts part of its predecessor).
I reduced the migration to only one file. I also added a management
command, setup_coupling. I let you test and squash the commits.
Anyway - thanks for your contribution, this is really awesome!
I hope it will be improved.
Cheers,
…--
Jean
|
|
Hi @jean1, awesome, thanks! I hope I'll find some time over the weekend to do some more testing, and if everything works out I'll merge as soon as possible so others can play with it as well. There's also been some discussion in #22 that (re-) raised an approach to link IPAM to NetBox DNS more closely, and I think it could be worth a try - the connection wouldn't be using VRFs (which, as was discussed a couple of times, doesn't really work out too well) but prefixes. I think that's a new idea that could actually work. The downside is that it uses So I'll suggest improving the validation of names in |
|
Hi @jean1, have you noticed that your tests don't actually run?
|
Since the custom fields needed for the experimental IPAM coupling feature are added with a management command, they were missing in the test database.
|
Hi Peter,
On Sun, Oct 15, 2023 at 07:18:46AM -0700, Peter Eckel wrote:
Hi @jean1, haven't you noticed that your tests don't actually run?
`__init__.py` is missing in `netbox_dns.tests.coupling`, so pytest doesn't execute them at all ... if it does, a part of them fails:
Sorry for being sloppy. I only ran a quick test with --keepdb.
Fixed both problems.
The tests failed since the custom fields were missing. I added them
in the test setup method. I am not satisfied by the code duplication.
Where could such a operation be located (necessary custom field creation).
… ```
/opt/netbox/netbox/manage.py test -v2 netbox_dns.tests.coupling
[...]
Ran 9 tests in 0.752s
FAILED (failures=5, errors=2)
```
--
Jean
|
|
You can run the management command from the setup method - that way you get the added benefit of testing its functionality as well. |
|
And don't worry about your perceived sloppiness ... the stuff is really complex, and it's easy to overlook things. I just noticed because the number of tests executed was still 381 :-) |
|
Hi @jean1, I've merged your code to a feature branch now and added a couple of small commit, mostly aiming at cosmetic changes and some improvements in terms of readability. I only found one issue that wasn't cosmetic: In Then, I renamed your custom fields. The problem with custom field names is that they are global, and Please have a look at the merged branch and tell me if you find anything you're not fine with. As I said, most of the changes are cosmetic and some deal with code segments that could be implemented in a more elegant and readable way. Most notably I replaced parts similar to with or instead of using I will now see what I can improve in my own code in order to take advantage of the new data when Then I'll add some documentation, and then finally we get the first release with sensible IPAM integration out to the public. Again, thank you very much for your work, without it it would definitely have taken much longer! |
* Introduce an experimental IPAM-DNS coupling feature Two new custom fields are added on IP Address : - name - zone Through a middleware and signals, all IP Address creation and modification are intercepted. Using the new custom fields, the corresponding DNS record is created, modified or deleted accordingly. Permission on DNS records are enforced. The IP address "dns_name" field is also updated.
(see issue #67)
Three new custom fields are added on IP Address :
Through a middleware and signals, all IP Address creation and modification are intercepted. Using the new custom fields, the corresponding DNS record is created, modified or deleted accordingly.
Permission on DNS records are enforced.
The IP address "dns_name" field is also updated.