1
\$\begingroup\$

I created a function that finds internal and external IP addresses, returns them in dict with the keys external_ips and local_ips:

def get_address_types(address_list):
    """
    determine whether an IP address is local or external from a given list of ip
    addresses

    returns a dict containing external and internal IP addresses, the internal
    are determined by the start nodes from a private ip space.
    for more information see here: https://en.wikipedia.org/wiki/Private_network
    example:
    >>> addresses = ["127.0.0.1", "10.0.1.1", "26.76.4.56"]
    >>> get_address_types(addresses)
    # <= {'external_ips': set(['26.76.4.56']), 'local_ips': set(['10.0.1.1'])}
    """
    placeholders = ("0.0.0.0", "255.255.255.255", "127.0.0.1", "8.8.8.8")
    retval = {"external_ips": set(), "local_ips": set()}
    for ip in address_list:
        if not any(n == ip for n in placeholders):
            if "." in ip:
                local_ip_ranges = ('10', '172', '192')
                start_node = ip.split(".")[0]
                if any(start == start_node for start in local_ip_ranges):
                    retval["local_ips"].add(ip)
                else:
                    try:
                        socket.inet_pton(socket.AF_INET, ip)
                        retval["external_ips"].add(ip)
                    except AttributeError:
                        try:
                            socket.inet_aton(ip)
                            retval["external_ips"].add(ip)
                        except socket.error:
                            pass
                    except socket.error:
                        pass
    return retval

So for example if I have a list that looks like this:

['224.0.0.251', '10.0.1.51', '10.0.1.255', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.31', '10.0.1.68', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.202', '224.0.0.251', '10.0.1.202', '224.0.0.251', '172.18.0.1', '172.18.255.255', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.9', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.202', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.47', '10.0.1.255', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.25', '224.0.0.251', '10.0.1.13', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.9', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.51', '224.0.0.251', '10.0.1.202', '224.0.0.251', '10.0.1.25', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.63', '224.0.0.251', '10.0.1.35', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.18', '10.0.1.13', '224.0.0.251', '10.0.1.31', '10.0.1.255', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.51', '255.255.255.255', '10.0.1.19', '255.255.255.255', '172.18.0.1', '172.18.255.255', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.13', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.51', '255.255.255.255', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.47', '10.0.1.255', '10.0.1.68', '10.0.1.22', '10.0.1.68', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.13', '224.0.0.251', '10.0.1.202', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.11', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.35', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.145', '10.0.1.255', '10.0.1.51', '255.255.255.255', '10.0.1.13', '224.0.0.251', '10.0.1.13', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.202', '224.0.0.251', '10.0.1.51', '10.0.1.255', '10.0.1.68', '224.0.0.251', '10.0.1.68', '224.0.0.251', '10.0.1.68', '8.8.8.8', '10.0.1.52', '224.0.0.251', '10.0.1.68', '10.0.1.22', '10.0.1.68', '10.0.1.22', '10.0.1.31', '10.0.1.255', '172.18.0.1', '172.18.255.255', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.27', '224.0.0.251', '10.0.1.68', '8.8.8.8', '10.0.1.68', '74.125.198.108', '10.0.1.13', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.13', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '8.8.8.8', '10.0.1.27', '224.0.0.251', '10.0.1.51', '255.255.255.255', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.63', '255.255.255.255', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.24', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.50', '10.0.1.68', '10.0.1.68', '163.172.24.175', '10.0.1.68', '224.0.0.251', '10.0.1.13', '224.0.0.251', '10.0.1.68', '8.8.8.8', '10.0.1.13', '224.0.0.251', '10.0.1.35', '224.0.0.251', '172.17.0.1', '172.17.255.255', '172.17.0.1', '172.17.255.255', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.68', '163.172.24.175', '10.0.1.15', '224.0.0.251', '10.0.1.27', '224.0.0.251', '10.0.1.31', '10.0.1.255', '10.0.1.200', '224.0.0.251', '172.18.0.1', '172.18.255.255', '172.18.0.1', '172.18.255.255', '172.19.0.1', '172.19.255.255', '172.19.0.1', '172.19.255.255', '10.0.1.9', '224.0.0.251', '10.0.1.68', '8.8.8.8', '10.0.1.68', '8.8.8.8', '10.0.1.68', '74.125.198.108', '10.0.1.13', '224.0.0.251', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.68', '74.125.198.108', '10.0.1.35', '224.0.0.251', '10.0.1.68', '10.0.1.200', '10.0.1.68', '74.125.198.108', '10.0.1.27', '224.0.0.251', '10.0.1.25', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.202', '224.0.0.251', '10.0.1.68', '74.125.198.109', '10.0.1.68', '74.125.198.109', '10.0.1.202', '224.0.0.251', '10.0.1.51', '255.255.255.255', '10.0.1.68', '224.0.0.251', '10.0.1.68', '224.0.0.251', '10.0.1.13', '224.0.0.251', '10.0.1.63', '255.255.255.255', '10.0.1.68', '8.8.8.8', '10.0.1.51', '255.255.255.255', '10.0.1.15', '224.0.0.251', '10.0.1.68', '10.0.1.22', '127.0.0.1', '127.0.0.1', '10.0.1.63', '255.255.255.255', '10.0.1.68', '10.0.1.11', '10.0.1.68', '10.0.1.22']

And I run it through my function, it will return the following results:

{'external_ips': set(['163.172.24.175', '74.125.198.108', '224.0.0.251', '74.125.198.109']), 'local_ips': set(['10.0.1.200', '10.0.1.202', '172.17.0.1', '10.0.1.145', '172.19.255.255', '10.0.1.63', '172.18.0.1', '172.19.0.1', '10.0.1.47', '10.0.1.68', '172.18.255.255', '10.0.1.27', '10.0.1.25', '10.0.1.24', '10.0.1.22', '172.17.255.255', '10.0.1.255', '10.0.1.9', '10.0.1.31', '10.0.1.52', '10.0.1.35', '10.0.1.50', '10.0.1.51', '10.0.1.13', '10.0.1.11', '10.0.1.15', '10.0.1.18', '10.0.1.19'])}

I would like some critique on this function and to know what I can do better to solve this specific problem, is there anything that can be changed or simplified to make this function shorter? Is there anything I can add to this function to make it better, etc? Thank you ahead of time.

\$\endgroup\$
  • \$\begingroup\$ To clarify this only works with IPv4 addresses \$\endgroup\$ – 13aal Feb 22 at 19:11
1
\$\begingroup\$

First, very good job with function documentation! A few minor notes on that:

  • Convention usually calls for sentence case (capital first letter ended with a period) for sentences in docstrings. This will make them look nice if you ever render them with something like pydoc
  • Including an example is fantastic! Additionally if you remove the # <= from the return value you can use this for doctests, which is a clever way of both documenting and testing your code
  • You may want to also lightly comment your function to demarcate each of the phases of processing the IP

Some random comments:

  • I'd name address_list just addresses
  • You can use ip not in placeholders; it is preferred to any(p == ip for p in placeholders)
  • You probably check '.' in ip so that ip.split('.')[0] can't raise an IndexError. In python, we usually prefer just to try ip.split('.')[0] and handle the IndexError if it happens. "Try first, apologize later."

Let's move on to the bigger stuff:

First, your return value is odd. Having a variable named retval is often a sign that you could improve your function. And further, returning a dictionary (from a function that doesn't operate on dictionaries) is very strange. Although your function is documented nicely, it leaves a lot of room for mistakes. What if I forget the name of the field for externals (I try "externals" and I get a KeyError)? Further, I can add keys to that dictionary, which is probably not something you want consumers of your function to be able to do. In addition, the dict is going to have some extra cost (above what I'll recommend) for look ups will will surely take up more space in memory. A dict is not the right datastructure.

Here's what I recommend. Inside your function, have two variables external_ips and local_ips. Instead of doing retval['local_ips'].add(...) just do local_ips.add(...). I'm even going to go as far as recommending that they be lists instead of sets. You don't really need them to be sets, because you don't take advantage of any of the set properties for local_ips or external_ips. Then, create a namedtuple for the return value:

IpLocalities = namedtuple('IpLocalities', 'external_ips local_ips')

# Then at the end of your function:
return IpLocalities(external_ips=external_ips, local_ips=local_ips)

The benefit here is multiple-fold. Printing this gives you the nice labels (like you had with dict):

IpLocalities(external_ips=['26.76.4.56'], local_ips=['10.0.1.1'])

But your access to them is now as efficient as tuple access, but is carries a name, so you don't have to remember if external_ips or local_ips is first. With just a tuple it would be easy to do:

# Note the order is wrong here
local_ips, external_ips = get_address_types(addresses)

With named tuples you can instead do:

localities = get_address_types(addresses)
print(localities.external_ips)

Next, unfortunately the rules for IP addresses are fairly complicated. Fortunately, python already has a library that handles parsing and manipulating IPs: ipaddress. You should be using it. Even if you weren't, you'd probably want to split your function up. Currently it has multiple responsibilities (although one of those responsibilities is unnecessary, we'll discuss that last). First, it loosely parses an IP to verify it is valid, then it categories the IP into external or local. This is two jobs for one function. Generally, you want to have functions do one thing and do it well. What if later you need to just verify an IP address, but don't want to classify it as local or external? If you pulled out just the verification into a separate function, say parse_ip, you could do this. You can also test this function in isolation, which will make unit testing much easier. Then, your get_address_types just assumes it is given valid IPs (perhaps you wrap them in an object) and implementing/unit testing it becomes so much easier.

Now, ipaddress already does basically all of this. Check it out:

def classify_addresses(ips):
    public_ips = []
    private_ips = []

    for ip in ips:
        if ip.is_global:
            public_ips.append(ip)
        if ip.is_private:
            private_ips.append(ip)


    return IpLocalities(public_ips=public_ips, private_ips=private_ips)


from ipaddress import ip_address

ips = [
    ip_address('224.0.0.251'), 
    ip_address('10.0.1.51'),
]

localities = classify_addresses(ips)

Note how ip_address handles the parsing for you and raises on an invalid IP. As part of the parsing, ipaddress is capable of identifying if an IP is public or private (this is the proper nomenclature, you should rename your terms). Note that there are some other classifications like is_multicast, is_reserved, and is_loopback that you may want to decide how to handle.

Also note that ip_address already supports IPv6. So you just got IPv6 support by doing absolutely nothing :)


Finally, let's address socket.inet_pton. You'll note in my example above, I don't use it. Why is that? inet_pton doesn't make any external network connections. It is only useful to parse an IP and return the packed binary representation of it (which can be sent over the network). You could actually replace your parsing of IPs with socket.inet_pton. It raises OSError if the IP address is invalid. But, that's all it does. And if you did use it, you wouldn't get the nice .is_public nor would you automatically get IPv6 support (without trying it again with AF_INET6). You just don't need it! Python's ipaddress handles all (and more) of what inet_pton or inet_aton would do for you.

\$\endgroup\$
0
\$\begingroup\$

Sets are a really nice thing if you only care about membership and not order. They have \$\mathcal{O}(1)\$ membership testing. Which you completely destroy by manually iterating over every element of the set when you do if not any(n == ip for n in placeholders). Just do if ip not in placeholders.

Similarly for your local IP ranges. Make it into a set and just use in. Even for tuples you can use in, it is just \$\mathcal{O}(n)\$, the same as your code.

It might make sense to have a third category where you put all IPs which you neither determine to be local nor external.

Instead of using a dictionary with already given keys and empty sets, you can just use a collections.defaultdict(set).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.