Add a custom script variable for IP addresses

[jeremystretch]

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

[jeremystretch]

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

[dgarros]

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

[jeremystretch]

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]

@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.

[hSaria]

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).

[jeremystretch]

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.

[hSaria]

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.

[jeremystretch]

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.

[hSaria]

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?

[jeremystretch]

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).

[hSaria]

Alright, I've adjusted the PR.

I think that's really the only option.

Best way to answer an or question 🤣; I assumed you meant the latter.