NAPALM require a primary IP although there is code to use the device name #5567

@DanSheps commented on GitHub (Oct 28, 2021):

@AlbanBedel I will assign you this on the following Caveat:

Any changes you make to the template must check either that the hostname is resolvable or there is a primary IP available

@DanSheps commented on GitHub (Oct 28, 2021): @AlbanBedel I will assign you this on the following Caveat: 1. Any changes you make to the template must check either that the hostname is resolvable or there is a primary IP available

adam commented

@AlbanBedel commented on GitHub (Oct 29, 2021):

My current solution is to just remove the check for the primary IP in the template because:

The template language conditionals seems pretty limited as they don't allow parenthesis, so the resulting expression to just add primary_ip or name into active and primary_ip and napalm_driver would lead to an unreadable monster
I assumed that devices that are active and have a napalm_driver set but don't have a primary IP nor a name could be considered a corner case.
Nothing is then broken for such corner case devices, they just show 3 useless tabs.

Now we are talking about fixing an obvious regression that render NAPALM useless for some user. I'm a bit surprised that some look and feel question for a corner case is deemed more important than the functionality itself.

Sadly, as I know nothing of danjgo, or the internals of netbox, fulfilling the additional requirement of actually resolving the hostname from the template is beyond the time I can allocate to this task right now.

@AlbanBedel commented on GitHub (Oct 29, 2021): My current [solution](https://github.com/AlbanBedel/netbox/commit/a225fa15702f6b3440b308a6b33e169f2dc45151) is to just remove the check for the primary IP in the template because: 1. The template language conditionals seems pretty limited as they don't allow parenthesis, so the resulting expression to just add `primary_ip or name` into `active and primary_ip and napalm_driver` would lead to an unreadable monster 2. I assumed that devices that are active and have a napalm_driver set but don't have a primary IP nor a name could be considered a corner case. 3. Nothing is then broken for such corner case devices, they just show 3 useless tabs. Now we are talking about fixing an obvious regression that render NAPALM useless for some user. I'm a bit surprised that some look and feel question for a corner case is deemed more important than the functionality itself. Sadly, as I know nothing of danjgo, or the internals of netbox, fulfilling the additional requirement of actually resolving the hostname from the template is beyond the time I can allocate to this task right now.

adam commented

@jeremystretch commented on GitHub (Oct 29, 2021):

Reclassifying this as a feature request, as the current limitation was intentional.

@jeremystretch commented on GitHub (Oct 29, 2021): Reclassifying this as a feature request, as the current limitation was intentional.

adam commented

@AlbanBedel commented on GitHub (Nov 2, 2021):

Reverting 7788bf3 was intentional?

@AlbanBedel commented on GitHub (Nov 2, 2021): Reverting 7788bf3 was intentional?

adam commented

@jeremystretch commented on GitHub (Nov 2, 2021):

"This device does not have a primary IP address configured."

Apparently.

@jeremystretch commented on GitHub (Nov 2, 2021): > "This device does not have a primary IP address configured." Apparently.

adam commented

@DanSheps commented on GitHub (Nov 2, 2021):

Reverting 7788bf3 was intentional?

Not sure why you think this is reverted, the logic is still present.

@DanSheps commented on GitHub (Nov 2, 2021): > Reverting [7788bf3](https://github.com/netbox-community/netbox/commit/7788bf3ce34aaa98d83bfafdb14bc82a7079e7a5) was intentional? Not sure why you think this is reverted, the logic is still present.

adam commented

61b61b1bc0/netbox/dcim/api/views.py (L411-L436)

@kkthxbye-code commented on GitHub (Nov 2, 2021):

@jeremystretch @DanSheps - I think you are misunderstanding him, and that he is correct.

The current code:

As you can see, the codepath in the else from L422 is dead code because the check for device.primary_ip was added back at some point (L412-413). device.primary_ip can never be falsy at that point, as it's already checked and thrown if falsy. If this was really intended, the dead code should be removed.

What's weird is that I can't find a diff for a commit where the code in L412-413 was added back, at least not from github when viewing the file history. Seems to indicate that he could be right that it's a merge mishap.

@kkthxbye-code commented on GitHub (Nov 2, 2021): @jeremystretch @DanSheps - I think you are misunderstanding him, and that he is correct. The current code: https://github.com/netbox-community/netbox/blob/61b61b1bc07c08c10256cb645327d074622b938f/netbox/dcim/api/views.py#L411-L436 As you can see, the codepath in the else from L422 is dead code because the check for device.primary_ip was added back at some point (L412-413). device.primary_ip can never be falsy at that point, as it's already checked and thrown if falsy. If this was really intended, the dead code should be removed. What's weird is that I can't find a diff for a commit where the code in L412-413 was added back, at least not from github when viewing the file history. Seems to indicate that he could be right that it's a merge mishap.

adam commented

@jeremystretch commented on GitHub (Nov 2, 2021):

This is tagged as "needs owner" for whoever would like to dig further into it and propose a specific change.

@jeremystretch commented on GitHub (Nov 2, 2021): This is tagged as "needs owner" for whoever would like to dig further into it and propose a specific change.

adam commented

@AlbanBedel commented on GitHub (Nov 4, 2021):

@kkthxbye-code yes it is a merge mishap, I spent a whole evening tracking it down to 22ee670 where you can see that one parent bring 7788bf3, so this branch doesn't have the early check, and the other parent commit do.

I proposed a specific change in my first comment, but @DanSheps added some requirement that are going beyond just fixing the broken merge.

@AlbanBedel commented on GitHub (Nov 4, 2021): @kkthxbye-code yes it is a merge mishap, I spent a whole evening tracking it down to 22ee670 where you can see that one parent bring 7788bf3, so this branch doesn't have the early check, and the other parent commit do. I proposed a specific change in my first comment, but @DanSheps added some requirement that are going beyond just fixing the broken merge.

adam commented

@DanSheps commented on GitHub (Nov 4, 2021):

You could do a nested if or set some control variables to properly check using and/or

    {% if perms.dcim.napalm_read_device and object.status == 'active' and object.platform.napalm_driver %}
        {% if object.platform.primary_ip or object.name|resolvednsnamefunction %}

Obviously, you would have to craft a filter for the template engine to resolve the dns name, but that shouldn't be too bad (literally just what is in the code with a try/catch statement)

@DanSheps commented on GitHub (Nov 4, 2021): You could do a nested if or set some control variables to properly check using and/or ``` {% if perms.dcim.napalm_read_device and object.status == 'active' and object.platform.napalm_driver %} {% if object.platform.primary_ip or object.name|resolvednsnamefunction %} ``` Obviously, you would have to craft a filter for the template engine to resolve the dns name, but that shouldn't be too bad (literally just what is in the code with a try/catch statement)

adam commented

@kkthxbye-code commented on GitHub (Nov 5, 2021):

Obviously, you would have to craft a filter for the template engine to resolve the dns name, but that shouldn't be too bad (literally just what is in the code with a try/catch statement)

Doing a DNS lookup before rendering the page is not ideal in my opinion. Page render is blocked until a response has been received.

Any changes you make to the template must check either that the hostname is resolvable or there is a primary IP available

Can't we just fail in the napalm API call if the hostname doesn't resolve, the logic is already there? Why do we have to check in the template? If the host on the IP doesn't response the API call fails as well, that isn't checked in the template.

@kkthxbye-code commented on GitHub (Nov 5, 2021): > Obviously, you would have to craft a filter for the template engine to resolve the dns name, but that shouldn't be too bad (literally just what is in the code with a try/catch statement) Doing a DNS lookup before rendering the page is not ideal in my opinion. Page render is blocked until a response has been received. > Any changes you make to the template must check either that the hostname is resolvable or there is a primary IP available Can't we just fail in the napalm API call if the hostname doesn't resolve, the logic is already there? Why do we have to check in the template? If the host on the IP doesn't response the API call fails as well, that isn't checked in the template.

adam commented

@DanSheps commented on GitHub (Nov 5, 2021):

Can't we just fail in the napalm API call if the hostname doesn't resolve, the logic is already there? Why do we have to check in the template? If the host on the IP doesn't response the API call fails as well, that isn't checked in the template.

To be clear, the original request is #4831

Tabs are there but are un-usable. This gives false assumptions to a user that they might work.

My opinion is this:

If the device resolves by hostname, it has an IP, that should be documented in the SoT
We should only do napalm requests on devices with valid IP addresses.

Now, I get the use case, however I think this is still wrong. Just because you don't use IPAM in NetBox doesn't mean you shouldn't at least have the primary IP documented in NetBox.

Just my 2c

@DanSheps commented on GitHub (Nov 5, 2021): > Can't we just fail in the napalm API call if the hostname doesn't resolve, the logic is already there? Why do we have to check in the template? If the host on the IP doesn't response the API call fails as well, that isn't checked in the template. To be clear, the original request is #4831 Tabs are there but are un-usable. This gives false assumptions to a user that they might work. My opinion is this: * If the device resolves by hostname, it has an IP, that should be documented in the SoT * We should only do napalm requests on devices with valid IP addresses. Now, I get the use case, however I think this is still wrong. Just because you don't use IPAM in NetBox doesn't mean you shouldn't at least have the primary IP documented in NetBox. Just my 2c

adam commented

@AlbanBedel commented on GitHub (Nov 5, 2021):

Tabs are there but are un-usable. This gives false assumptions to a user that they might work.

They would only be visible after you configure a platform with a NAPALM driver and set this platform on the device. Once that is configured I don't see what case is left were you wouldn't want the tabs to be visible.

My opinion is this:

If the device resolves by hostname, it has an IP, that should be documented in the SoT

We should only do napalm requests on devices with valid IP addresses.

Now, I get the use case, however I think this is still wrong. Just because you don't use IPAM in NetBox doesn't mean you shouldn't at least have the primary IP documented in NetBox.

My use case is a network that use DHCP, so there is no fixed IP that can be documented.

@AlbanBedel commented on GitHub (Nov 5, 2021): > Tabs are there but are un-usable. This gives false assumptions to a user that they might work. They would only be visible after you configure a platform with a NAPALM driver and set this platform on the device. Once that is configured I don't see what case is left were you wouldn't want the tabs to be visible. > My opinion is this: > > * If the device resolves by hostname, it has an IP, that should be documented in the SoT > > * We should only do napalm requests on devices with valid IP addresses. > > > Now, I get the use case, however I think this is still wrong. Just because you don't use IPAM in NetBox doesn't mean you shouldn't at least have the primary IP documented in NetBox. My use case is a network that use DHCP, so there is no fixed IP that can be documented.

adam commented

@DanSheps commented on GitHub (Nov 8, 2021):

Tabs are there but are un-usable. This gives false assumptions to a user that they might work.

They would only be visible after you configure a platform with a NAPALM driver and set this platform on the device. Once that is configured I don't see what case is left were you wouldn't want the tabs to be visible.

Platforms are not specific per device. You are asking to infer the napalminess of a device from the platform. The platform model is to give a hint about the operating system of a device (Can be as simple as Cisco IOS or as complex as Cisco IOS-XE Catalyst 9300 17.03.03 or more). The reason the napalm driver is there is because the napalm driver is generally common for a given operating system.

I think a better approach, may be to allow a new field for a device's management interfaces that defines whether the device's management IP is DHCP or not. If that is the case, then perhaps show the tabs and allow napalm lookups by hostname.

@DanSheps commented on GitHub (Nov 8, 2021): > > Tabs are there but are un-usable. This gives false assumptions to a user that they might work. > > They would only be visible after you configure a platform with a NAPALM driver and set this platform on the device. Once that is configured I don't see what case is left were you wouldn't want the tabs to be visible. Platforms are not specific per device. You are asking to infer the napalminess of a device from the platform. The platform model is to give a hint about the operating system of a device (Can be as simple as `Cisco IOS` or as complex as `Cisco IOS-XE Catalyst 9300 17.03.03` or more). The reason the napalm driver is there is because the napalm driver is generally common for a given operating system. I think a better approach, may be to allow a new field for a device's management interfaces that defines whether the device's management IP is DHCP or not. If that is the case, then perhaps show the tabs and allow napalm lookups by hostname.

adam commented

@AlbanBedel commented on GitHub (Nov 8, 2021):

I think a better approach, may be to allow a new field for a device's management interfaces that defines whether the device's management IP is DHCP or not. If that is the case, then perhaps show the tabs and allow napalm lookups by hostname.

So currently the "napalminess" is inferred from "has a platform with NAPALM driver and an IP", I don't think that making this logic even more complex is a good idea. If what you want is to be able to disable NAPALM on selected devices why not just add a device field to disable NAPALM?

@AlbanBedel commented on GitHub (Nov 8, 2021): > I think a better approach, may be to allow a new field for a device's management interfaces that defines whether the device's management IP is DHCP or not. If that is the case, then perhaps show the tabs and allow napalm lookups by hostname. So currently the "napalminess" is inferred from "has a platform with NAPALM driver and an IP", I don't think that making this logic even more complex is a good idea. If what you want is to be able to disable NAPALM on selected devices why not just add a device field to disable NAPALM?

adam commented