Add a custom script variable for IP addresses #2878

Originally created by @jeremystretch on GitHub (Sep 17, 2019).

Environment

Python version: 3.6.8
NetBox version: 2.6.3

Proposed Functionality

Add a variable type for representing IP addresses in custom scripts. This would be similar to the existing IPNetworkVar, but for individual IPs rather than prefixes.

Use Case

Adding this variable type will allow for validation of IP addresses.

Database Changes

None

External Dependencies

None

Originally created by @jeremystretch on GitHub (Sep 17, 2019). ### Environment * Python version: 3.6.8 * NetBox version: 2.6.3 ### Proposed Functionality Add a variable type for representing IP addresses in custom scripts. This would be similar to the existing `IPNetworkVar`, but for individual IPs rather than prefixes. ### Use Case Adding this variable type will allow for validation of IP addresses. ### Database Changes None ### External Dependencies None

adam added the status: accepted type: feature labels 2025-12-29 18:23:01 +01:00

adam closed this issue

adam commented

@jeremystretch commented on GitHub (Sep 17, 2019):

Considering this further, IPNetworkVar currently accepts either a network or an IP address. Is it worth splitting these into separate variable types?

@jeremystretch commented on GitHub (Sep 17, 2019): Considering this further, `IPNetworkVar` currently accepts either a network or an IP address. Is it worth splitting these into separate variable types?

adam commented

@dgarros commented on GitHub (Oct 1, 2019):

if IPNetworkVar already accept an IP address it's probably not necessary to have a dedicated variable type for IP ..

@dgarros commented on GitHub (Oct 1, 2019): if `IPNetworkVar` already accept an IP address it's probably not necessary to have a dedicated variable type for IP ..

adam commented

@jeremystretch commented on GitHub (Oct 1, 2019):

I can imagine instances where you want to prompt the user for specifically either an IP address (e.g. defining an NTP server) or a prefix (e.g. a site aggregate). It should be minimal effort to introduce an IPAddressVar variable and enforce a specific type for each.

@jeremystretch commented on GitHub (Oct 1, 2019): I can imagine instances where you want to prompt the user for specifically either an IP address (e.g. defining an NTP server) _or_ a prefix (e.g. a site aggregate). It should be minimal effort to introduce an `IPAddressVar` variable and enforce a specific type for each.

adam commented

@jeremystretch commented on GitHub (Jan 15, 2020):

@hSaria I started commenting on #3924 but wanted to share some more thoughts here.

IMO we should accept a mask value for IP addresses. I think the ideal behavior for the two fields would be:

IPAddressVar: Accepts a host IP, with or without a mask
IPNetworkVar: Accepts a network (non-host) address; requires a mask

IPAddressVar would be suitable for representing either assigned IPs (with a mask) or source/destination addresses (no mask), whereas IPNetworkVar would represent a prefix (e.g. a route).

I think it would make sense to allow the user to require or disable the innclusion of a mask on IPAddressVar to support the two different use cases.

@jeremystretch commented on GitHub (Jan 15, 2020): @hSaria I started commenting on #3924 but wanted to share some more thoughts here. IMO we should accept a mask value for IP addresses. I think the ideal behavior for the two fields would be: * `IPAddressVar`: Accepts a host IP, with or without a mask * `IPNetworkVar`: Accepts a network (non-host) address; requires a mask `IPAddressVar` would be suitable for representing either assigned IPs (with a mask) or source/destination addresses (no mask), whereas `IPNetworkVar` would represent a prefix (e.g. a route). I think it would make sense to allow the user to require or disable the innclusion of a mask on `IPAddressVar` to support the two different use cases.

adam commented

@hSaria commented on GitHub (Jan 15, 2020):

I'm not quite following. Wouldn't an IPNetworkVar return an object of netaddr.IPNetwork which already has a subnet mask in it?

>>> netaddr.IPNetwork('1.1.1.1/24')
IPNetwork('1.1.1.1/24')

So wouldn't that be used in case a subnet mask is needed for an address? I figured that, from your NTP example, you'd want the user to indicate that this is an IP with no relation to the L3 domain it lives in, like – as you mentioned – a source/destination address.

I'm probably misunderstanding, but I'm not sure how IPAddressVar would be different from IPNetworkVar at that point since they both accept subnet masks (i.e. what condition/check would I put in IPAddressVar that it warrants its own type).

@hSaria commented on GitHub (Jan 15, 2020): I'm not quite following. Wouldn't an `IPNetworkVar` return an object of `netaddr.IPNetwork` which already has a subnet mask in it? ```python >>> netaddr.IPNetwork('1.1.1.1/24') IPNetwork('1.1.1.1/24') ``` So wouldn't that be used in case a subnet mask is needed for an address? I figured that, from your NTP example, you'd want the user to indicate that this is an IP with no relation to the L3 domain it lives in, like – as you mentioned – a source/destination address. I'm probably misunderstanding, but I'm not sure how `IPAddressVar` would be different from `IPNetworkVar` at that point since they both accept subnet masks (i.e. what condition/check would I put in `IPAddressVar` that it warrants its own type).

adam commented

@jeremystretch commented on GitHub (Jan 15, 2020):

There are three use cases for an IP-like value that I can think of:

A bare IP address (no mask); e.g. a syslog server's address
A host IP address with a mask; e.g. an IP address being assigned to an interface
A network prefix; e.g. a route or ACL entry

A prefix must include a mask and must not be a host address. For example, 192.0.2.0/24 is valid but 192.0.2.17/24 is not, because we want to define the prefix itself rather than an IP within the prefix. (I realize that 192.0.2.17/24 is essentially identical to 192.0.2.0/24 and will resolve fine in most cases. This is simply about sanity-checking and validation.)

Where we run into an issue is the way netaddr wants to represent IP objects: The first two use cases would require an IPAddress and an IPNetwork, respectively. So this boils down to whether it's acceptable to pass either type to the script context, depending on how the IPAddressVar is invoked. For example:

syslog_server = IPAddressVar(include_mask=False)

would return an IPAddress object (no mask), but

interface_ip = IPAddressVar(include_mask=True)

would return an IPNetwork object (with a mask).

Granted it may not be the most intuitive approach, but I do think we need to support all three discrete use cases above.

@jeremystretch commented on GitHub (Jan 15, 2020): There are three use cases for an IP-like value that I can think of: * A bare IP address (no mask); e.g. a syslog server's address * A host IP address _with_ a mask; e.g. an IP address being assigned to an interface * A network prefix; e.g. a route or ACL entry A prefix must include a mask and must _not_ be a host address. For example, `192.0.2.0/24` is valid but `192.0.2.17/24` is not, because we want to define the prefix itself rather than an IP within the prefix. (I realize that `192.0.2.17/24` is essentially identical to `192.0.2.0/24` and will resolve fine in most cases. This is simply about sanity-checking and validation.) Where we run into an issue is the way `netaddr` wants to represent IP objects: The first two use cases would require an `IPAddress` and an `IPNetwork`, respectively. So this boils down to whether it's acceptable to pass either type to the script context, depending on how the `IPAddressVar` is invoked. For example: ```python syslog_server = IPAddressVar(include_mask=False) ``` would return an `IPAddress` object (no mask), but ```python interface_ip = IPAddressVar(include_mask=True) ``` would return an `IPNetwork` object (with a mask). Granted it may not be the most intuitive approach, but I do think we need to support all three discrete use cases above.

adam commented

@hSaria commented on GitHub (Jan 15, 2020):

Ah, now I see. That would introduce a change to the IPNetworkVar to make it so it'll check that it's a network (i.e. IPNetwork.network != IPNetwork.ip) or just throw away the host bits (i.e. return IPNetwork.cidr) when returning the object. Is that okay?

Edit: another option would be to add an optional argument to IPNetworkVar that will strip the host bits (or raise an exception). So that would leave us with

IPAddressVar: address, no mask
IPNetworkVar (default/existing): return network while keeping the hostbits
IPNetworkVar (network=True): returns network with the hostbits removed or raises an exception (not sure which is better)

This would be backwards compatible with existing scripts.

@hSaria commented on GitHub (Jan 15, 2020): Ah, now I see. That would introduce a change to the `IPNetworkVar` to make it so it'll check that it's a network (i.e. `IPNetwork.network != IPNetwork.ip`) or just throw away the host bits (i.e. return `IPNetwork.cidr`) when returning the object. Is that okay? Edit: another option would be to add an optional argument to `IPNetworkVar` that will strip the host bits (or raise an exception). So that would leave us with * `IPAddressVar`: address, no mask * `IPNetworkVar (default/existing)`: return network while keeping the hostbits * `IPNetworkVar (network=True)`: returns network with the hostbits removed or raises an exception (not sure which is better) This would be backwards compatible with existing scripts.

adam commented

@jeremystretch commented on GitHub (Jan 16, 2020):

That would introduce a change to the IPNetworkVar to make it so it'll check that it's a network or just throw away the host bits when returning the object.

We should maintain the current validation behavior when creating a prefix normally in NetBox, which is to raise a ValidationError. For example, if I try to create the prefix 192.0.2.123/24, it fails with an error:

192.0.2.123/24 is not a valid prefix. Did you mean 192.0.2.0/24?

(Again, this is primarily a sanity check against the input data.)

We should use IPAddressVar for IPs and IPNetworkVar for networks, since that it was a script author would expect.

@jeremystretch commented on GitHub (Jan 16, 2020): > That would introduce a change to the `IPNetworkVar` to make it so it'll check that it's a network or just throw away the host bits when returning the object. We should maintain the current validation behavior when creating a prefix normally in NetBox, which is to raise a ValidationError. For example, if I try to create the prefix `192.0.2.123/24`, it fails with an error: > 192.0.2.123/24 is not a valid prefix. Did you mean 192.0.2.0/24? (Again, this is primarily a sanity check against the input data.) We should use `IPAddressVar` for IPs and `IPNetworkVar` for networks, since that it was a script author would expect.

adam commented

@hSaria commented on GitHub (Jan 16, 2020):

I fully agree.

We should maintain the current validation behavior

That's the thing. The current behavior for IPNetworkVar is to quietly accept a network with host bits. I'm happy to change it so it raises ValidationError if this is the case, but that would entail people changing up their scripts. Let me know if what you'd like do with that regard (i.e. to start raising an exception or not to).

Final thing: do you want IPAddressVar to return netaddr.IPAddress which has no mask or netaddr.IPNetwork which has one?

@hSaria commented on GitHub (Jan 16, 2020): I fully agree. > We should maintain the current validation behavior That's the thing. The current behavior for `IPNetworkVar` is to quietly accept a network with host bits. I'm happy to change it so it raises `ValidationError` if this is the case, but that would entail people changing up their scripts. Let me know if what you'd like do with that regard (i.e. to start raising an exception or not to). Final thing: do you want `IPAddressVar` to return `netaddr.IPAddress` which has no mask or `netaddr.IPNetwork` which has one?

adam commented

@jeremystretch commented on GitHub (Jan 16, 2020):

Yeah, I think we've tolerated IPNetworkVar being overloaded simply because there was no other option for representing an IP address (other than a raw string). I'm fine with changing it; we'll just need to ensure it gets documented clearly.

Final thing: do you want IPAddressVar to return netaddr.IPAddress which has no mask or netaddr.IPNetwork which has one?

I think that's really the only option. Should be fine so long as the script author knows to expect either type (which should be indicated in the documentation for the field).

@jeremystretch commented on GitHub (Jan 16, 2020): Yeah, I think we've tolerated `IPNetworkVar` being overloaded simply because there was no other option for representing an IP address (other than a raw string). I'm fine with changing it; we'll just need to ensure it gets documented clearly. > Final thing: do you want `IPAddressVar` to return `netaddr.IPAddress` which has no mask or `netaddr.IPNetwork` which has one? I think that's really the only option. Should be fine so long as the script author knows to expect either type (which should be indicated in the documentation for the field).

adam commented