cpython GH-113171: Fix "private" (non-global) IP address ranges

The _private_networks variables, used by various is_private implementations, were missing some ranges and at the same time had overly strict ranges (where there are more specific ranges considered globally reachable by the IANA registries).

This patch updates the ranges with what was missing or otherwise incorrect.

I left 100.64.0.0/10 alone, for now, as it's been made special in [1] and I'm not sure if we want to undo that as I don't quite understand the motivation behind it.

Additionally I mainly focused on adding tests for IPAddress.is_global and I left IPNetwork.is_global alone. The reasons for that are:

I don't think it makes much sense to have properties like is_global, is_private etc. on networks, where there are more than two possibilities (can be global, can be non-global, can be partially global).
The properties aren't documented for network objects in the first place so it's unclear what the semantics are

[1] https://github.com/python/cpython/issues/61602

Issue: gh-113171

Dec 15 '23 16:12 jstasiak

Two concerns to resolve:

What to do about 100.64.0.0/10?
Should I add a news entry for this? It's both a fix and a somewhat breaking change.

Dec 15 '23 16:12 jstasiak

I thought about it a little bit and I'll have a much simpler implementations of this, not today though

Dec 15 '23 19:12 jstasiak

Simplified. I continue to be unsure about the semantics of these properties for networks, I left them alone (for now at least).

Jan 09 '24 19:01 jstasiak

IMO:

What to do about 100.64.0.0/10?

Keep it as it is, and document it? I see you did that in #113186.

Should I add a news entry for this? It's both a fix and a somewhat breaking change.

Yes, and also add it to What's New.

unsure about the semantics of these properties for networks

It would probably be best to document that if not all addresses in the network share a single value, the result is unspecified. That's the case now, too. However, if all of the network is in an exception, I think that should be honored.

Mar 13 '24 12:03 encukou

Thank you for your feedback @encukou.

To address your points in reverse order:

unsure about the semantics of these properties for networks

It would probably be best to document that if not all addresses in the network share a single value, the result is unspecified. That's the case now, too. However, if all of the network is in an exception, I think that should be honored.

Agreed and done (as far as I can tell).

Should I add a news entry for this? It's both a fix and a somewhat breaking change.

Yes, and also add it to What's New.

Done. Feedback very much appreciated, especially regarding the wording and verbosity (I opted to be verbose in the news entry but terse in the What's new entry per the "Rules for maintenance" listed in the file, I expect it'll be overwritten by an editor or a maintainer).

IMO:

What to do about 100.64.0.0/10?

Keep it as it is, and document it? I see you did that in #113186.

Here's the thing: after some thinking I realized I actually would like to fix that one too – #113186 documents the status quo but I believe we can (and IMO should) still improve it.

So I went ahead and fixed it as well. I don't think it deserves the kind of special, not quite consistent treatment it's been getting.

The fix is self-contained in a single commit which can be reverted away if you (or any else) tell me it doesn't belong in this PR or it otherwise becomes a blocker – I'd rather have this PR merged without the shared address space fix than not have it merged at all.

Happy to update the PR description to reflect the above when we know the final shape of this change.

Mar 18 '24 22:03 jstasiak

Thank you! This looks great. (I haven't checked details like the individual numbers in this pass.)

Feedback [for changelogs] very much appreciated, especially regarding the wording and verbosity

It looks fine, but it would be even better to you can link from the less-verbose docs to the details. Then the details only need to be in one place.

The fix is self-contained in a single commit which can be reverted away if you (or any else) tell me it doesn't belong in this PR or it otherwise becomes a blocker

Yup, I'll be the one to tell you. Please don't do that in this PR. We squash every PR into a single commit; this change would be better as its own revertable commit. Also, I'm not really an expert on ipaddress, and I don't feel as comfortable reviewing this change.

Mar 20 '24 13:03 encukou

Done and done.

Mar 22 '24 00:03 jstasiak

Looks good, thank you!

Mar 22 '24 16:03 encukou

Thanks @jstasiak for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. 🐍🍒⛏🤖