archweb icon indicating copy to clipboard operation
archweb copied to clipboard

fields: replace IPy dependency with ipaddress module

Open johnnyapol opened this issue 4 years ago • 7 comments

Resolves #229

johnnyapol avatar Aug 01 '20 19:08 johnnyapol

I ran the tests in the README and addressed the failure (python3's ipaddress module gave a different assertion).

Please let know if you would like to see any additional regression tests, documentation, etc.

johnnyapol avatar Aug 01 '20 19:08 johnnyapol

We have IP addresses that don't work with ipaddress module such as:

>>> print(ip_address('2a0b:4342:1a31:410::/64'))                                                                                                                         
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.8/ipaddress.py", line 53, in ip_address
    raise ValueError('%r does not appear to be an IPv4 or IPv6 address' %
ValueError: '2a0b:4342:1a31:410::/64' does not appear to be an IPv4 or IPv6 address

jelly avatar Aug 03 '20 19:08 jelly

I see, I didn't see #276 when I took a look at this.

My fix was to check if '/' existed in the string. If no mask exists, then we just call ip_address like normal. Otherwise, we call into ip_interface, which handles those cases.

Although, this won't work if we're inputting the addresses in a non-string format. If that's an issue, a simple workaround can be to guard the call to ip_address with a try: block and then call into ip_interface as a backup

johnnyapol avatar Aug 04 '20 00:08 johnnyapol

Also interesting a ipaddressfield is now available in Django but that might not work as it lacks the hostmask.

jelly avatar Sep 06 '21 19:09 jelly

@jelly I just rebased this if you want to take another look. Hostmasks do work in this PR now (and tests were added to avoid regressions)

johnnyapol avatar Sep 06 '21 19:09 johnnyapol

@jelly I just rebased this if you want to take another look. Hostmasks do work in this PR now (and tests were added to avoid regressions)

Thanks a lot, taking a look at testing the changes / impact locally!

jelly avatar Sep 12 '21 10:09 jelly

Tested by creating a new rsync ipv6 address in the django admin interface causes:

    if '/' not in address:
TypeError: argument of type 'IPv6Interface' is not iterable

In https://archweb.lxd/admin/mirrors/mirror/214/change/

Same for a ipv4 address:

  File "/home/jelle/projects/archweb/mirrors/fields.py", line 37, in to_python
    return IP(value)
  File "/home/jelle/projects/archweb/mirrors/fields.py", line 9, in IP
    if '/' not in address:
TypeError: argument of type 'IPv4Address' is not iterable

jelly avatar Sep 12 '21 10:09 jelly