IPNetwork: Allow partial addresses (again)

[kajinamit]

This introduces the new expand argument to IPNetwork class, so that users can retain the previous behavior of netaddr < 1.0.0 optionally.

>>> import netaddr
>>> netaddr.IPNetwork('10/8')
Traceback (most recent call last):
  File "/home/tkajinam/git/python/netaddr/netaddr/strategy/ipv4.py", line 126, in str_to_int
    packed = _inet_pton(AF_INET, addr)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: illegal IP address string passed to inet_pton

...

During handling of the above exception, another exception occurred:
    
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tkajinam/git/python/netaddr/netaddr/ip/__init__.py", line 1035, in __init__
    raise AddrFormatError('invalid IPNetwork %s' % (addr,))
netaddr.core.AddrFormatError: invalid IPNetwork 10/8
>>> netaddr.IPNetwork('10/8', partial=True)
IPNetwork('10.0.0.0/8')

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.11%. Comparing base (31d8e29) to head (959e045).
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
+ Coverage   85.72%   86.11%   +0.38%     
==========================================
  Files          47       47              
  Lines        4379     4392      +13     
  Branches      737      740       +3     
==========================================
+ Hits         3754     3782      +28     
+ Misses        449      434      -15     
  Partials      176      176              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kajinamit]

The comment suggests expand_partial_ipv4_address, but this requires parsing IPNetwork (by splitting host and bitmask) and then expand the first bit and combine these two again for parsing. It'd make better sense to implement that conversion within IPNetwork, to avoid redundant split -> join -> split.

[jstasiak]

Thank you for the contribution @kajinamit.

Can you advocate for the need for the old behavior? It was removed intentionally and I'm hesitant to consider giving it first-class support without a good reason for it.

[jstasiak]

Maybe put differently – is this form produced by some commonly used software or is it otherwise common (and so is the need to handle it)?

Any links to relevant materials (including but not limited to RFCs) will be appreciated.

[brianphaley]

Hi - it's related to Openstack Neutron. While we can fix our code to deal with the change in most places, the API behavior would previously allow '10/8' for a cidr [0], but with versions > 1.0.0 of netaddr this isn't possible. While I hope no one was using that notation we can't be sure.

[0] https://bugs.launchpad.net/neutron/+bug/2054203/comments/4

[kajinamit]

@jstasiak
As @brianphaley mentioned, this behavior has been used in some components (like neutron and nova) in OpenStack. netaddr has been used as a main method to parse and validate IP addresses and CIDR in user input, and we have supported INET_ATON compliant ip address format like 10 for IP addresses and shorten format like 10/8 for CIDRs for long.

Because netaddr was used at user input validation, it's quite hard to guarantee that no one has been relying on its previous behavior, and we likely need to retain the previous "wider" acceptance.

[jstasiak]

Thank you both for the explanation. Some followup questions below.

we have supported INET_ATON compliant ip address format like 10 for IP addresses and shorten format like 10/8 for CIDRs for long

Can you clarify what does "supported" mean here? Is it explicitly allowed with well-defined behavior or has it been working only as an unintended consequence of netaddr's (old) behavior?

What is 10/24 expected to parse as? How about 10.20/24?

we have supported INET_ATON compliant ip address format like 10 for IP addresses and shorten format like 10/8 for CIDRs for long

Is the fact that addresses like 10 and 10.20 have had drastically different behavior depending on whether they've been given to IPAddress or IPNetwork (when it still supported partial addresses) also part of what you've been supporting?

In [12]: IPAddress('10', flags=INET_ATON)
Out[12]: IPAddress('0.0.0.10')

In [13]: IPAddress('10.20', flags=INET_ATON)
Out[13]: IPAddress('10.0.0.20')

In [14]: expand_partial_ipv4_address('10')
Out[14]: '10.0.0.0'

In [15]: expand_partial_ipv4_address('10.20')
Out[15]: '10.20.0.0'

Is this actually a commonly used format and people are likely to be affected?

I'm not opposed to bringing this kind of behavior back, behind a feature flag, in principle – I'm trying to gauge if this is worth it.

[kajinamit]

Thank you for taking your time !

we have supported INET_ATON compliant ip address format like 10 for IP addresses and shorten format like 10/8 for CIDRs for long

Can you clarify what does "supported" mean here? Is it explicitly allowed with well-defined behavior or has it been working only as an unintended consequence of netaddr's (old) behavior?

Probably the word "accepted" would explain my intention better.
AFAIK the accepted format hasn't been strictly documented (see [1] for example...), but the OpenStack services implicitly accepted these formats because of netadd's old behavior.
The main concern here is these were not documented but were accepted for some time. In OpenStack projects we've tried to avoid making any API impacting changes, because it can cause any external tools or users, unless the change is required to resolve an actual bug or security issue.

What is 10/24 expected to parse as? How about 10.20/24?
10.0.0.0/24 and 10.20.0.0/24 , as are parsed previously.

we have supported INET_ATON compliant ip address format like 10 for IP addresses and shorten format like 10/8 for CIDRs for long

Is the fact that addresses like 10 and 10.20 have had drastically different behavior depending on whether they've been given to IPAddress or IPNetwork (when it still supported partial addresses) also part of what you've been supporting?

Yes I noticed that during looking into the changes and I found it very odd. However that's how OpenStack parsed user inputs for very long time.

Is this actually a commonly used format and people are likely to be affected?

This may not be very common (even I wasn't aware of it ...), but this behavior has been kept for several years and we can't tell no one is affected, either.

I'm not opposed to bringing this kind of behavior back, behind a feature flag, in principle – I'm trying to gauge if this is worth it.

I understand.

[1] https://docs.openstack.org/api-ref/network/v2/index.html#create-subnet
cidr body string The CIDR of the subnet.

[jstasiak]

Ok, I thought about it and let's bring this behavior back.

Changes to make to this PR:

• Unclear what the next version is gonna be, use the NEXT_NETADDR_VERSIOn placeholder
• I decided I don't quite like the flag system so let's put this feature behind a keyword-only parameter in the IPNetwork constructor, named expand_partial I think (unless you have better ideas)
• We need a usage example (a doctest in the IPNetwork constructor docstring) demonstrating this feature

[kajinamit]

Ok, I thought about it and let's bring this behavior back.

Changes to make to this PR:

* Unclear what the next version is gonna be, use the `NEXT_NETADDR_VERSIOn` placeholder

* I decided I don't quite like the flag system so let's put this feature behind a keyword-only parameter in the `IPNetwork` constructor, named `expand_partial` I think (unless you have better ideas)

* We need a usage example (a doctest in the `IPNetwork` constructor docstring) demonstrating this feature

I've updated the change to address these. I'm not very sure about the 2nd point, which may make it a bit tricky to adapt to this upgrade (we have to make the logic change when we bump netaddr), but I'd not strongly object to it.

[osfrickler]

Ok, I did some research and it seems there is in fact no standard for this shorthand notation, but I can confirm that is has been common to use it in network engineer circles for the last 20 years at least. It is also accepted e.g. by iproute2 so you can do things like ìp route show 10/8. Finally it is mentioned in some similar library docs like https://metacpan.org/pod/Net::IP#Object-Creation

[osfrickler]

thx

[brianphaley]

Not sure if this is supposed to be an actual number, otherwise lgtm.

[jstasiak]

It's a placeholder used in the project, it's good

[jstasiak]

Thank you

[jstasiak]

I allowed myself to change the new parameter to keyword-only expand_partial post-merge. The feature/fix has been released in netaddr 1.3.0.