Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-87799: Improve the textual representation of IPv4-mapped IPv6 addresses #29345

Merged
merged 13 commits into from
Jul 31, 2023

Conversation

opavlyuk
Copy link
Contributor

@opavlyuk opavlyuk commented Oct 31, 2021

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Nov 9, 2021

@maxmouchet, @ykvch guys, please take a look on this PR when you get a chance.
I'll greatly appreciate initial review, before someone from core developers gets here.

@maxmouchet
Copy link

maxmouchet commented Nov 9, 2021

Thanks for tackling this! This looks good to me.

Maybe the __str__ method could be simplified a little as I think (need to find a source) that an IPv4-mapped IPv6 address will never have a scope identifier.

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Nov 10, 2021

Maybe the __str__ method could be simplified a little as I think (need to find a source) that an IPv4-mapped IPv6 address will never have a scope identifier.

Thanks! Lets discuss it.
Indeed, according to RFC 4007 Section 11.1, IPv6 address 'scoped' format is meaningless and should not be used for global addresses.
At the same time,

This document, however, does not prohibit an implementation from using the format for those special addresses for implementation dependent purposes.

In addition, RFC 4007 Section 4 says that

[1] defines IPv6 addresses with embedded IPv4 addresses as being part
of global addresses. Thus, those addresses have global scope, with
regard to the IPv6 scoped address architecture. However, an
implementation may use those addresses as if they had other scopes
for convenience. For instance, [6] assigns link-local scope to IPv4
auto-configured link-local addresses (the addresses from the prefix
169.254.0.0/16 [7]) and converts those addresses into IPv4-mapped
IPv6 addresses in order to perform destination address selection
among IPv4 and IPv6 addresses. This would implicitly mean that the
IPv4-mapped IPv6 addresses equivalent to the IPv4 auto-configuration
link-local addresses have link-local scope. This document does not
preclude such a usage, as long as it is limited within the
implementation.

That is all I managed to find regarding the subject.

Please, let me know what you think about it.

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Nov 10, 2021

Noticed, that ipaddress.IPv6Address.exploded behavior differs from expected after the patch:

>>> addr = IPv6Address('::ffff:8.8.4.4')
# Expected
>>> addr.exploded
'0000:0000:0000:0000:0000:ffff:8.8.4.4'
# Actual
>>> addr.exploded
'0000:0000:0000:0000:0000:ffff:0808:0404'

I'm in progress fixing that.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 11, 2021
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran 203 tests in 0.198s

OK

== Tests result: SUCCESS ==
And I get:
>>> addr.exploded '0000:0000:0000:0000:0000:ffff:8.8.4.4'
Looks good to me.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented May 3, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

…esses

Represent IPv4-mapped IPv6 address as x:x:x:x:x:x:d.d.d.d,
where the 'x's are the hexadecimal values
of the six high-order 16-bit pieces of the address,
and the 'd's are the decimal values
of the four low-order 8-bit pieces of the address
(standard IPv4 representation).
@arhadthedev arhadthedev changed the title bpo-43633 Improve the textual representation of IPv4-mapped IPv6 addresses gh-87799: Improve the textual representation of IPv4-mapped IPv6 addresses Apr 13, 2023
@arhadthedev arhadthedev added the stdlib Python modules in the Lib dir label Apr 13, 2023
@arhadthedev
Copy link
Member

@opavlyuk Could you sign the new CLA by clicking not signed button in the cpython-cla-bot's message, please?

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Apr 13, 2023

@arhadthedev Thanks for letting me know! I initially used the wrong SSH key for the commits, but I've fixed it and the CLA is now marked as signed.
Could you suggest what should I do next to move forward with this PR? Should I contact the python-dev mailing list or take any other steps?

@arhadthedev
Copy link
Member

For now, we need to wait.

  1. Until 3.12b1 is out next month, core devs with network expertise can be busy with release blockers and PEP-grade features
  2. Core devs need time to process through their unread notification queue after work (a couple of days or so, maybe sooner, it depends)
  3. There is a feedback gathering month to give everybody a chance to participate in the PR.

@opavlyuk
Copy link
Contributor Author

@arhadthedev Thanks for clearing things up! Then I'll watch out for the 3.12b1 release and return to the PR once all obstacles are cleared.
It's been waiting here for a while, so I'd like to kindly help it move along when the time is right 🙂

@opavlyuk
Copy link
Contributor Author

opavlyuk commented May 16, 2023

@arhadthedev it appears a month has passed since the last update to the PR, during which time we haven't received further feedback. Also, I noticed that version 3.12.0a7 was released just yesterday.
I understand everyone is likely quite busy. Given these circumstances, do you think it might be appropriate to send a gentle reminder to our PR, in order to possibly draw the attention of the core developers?

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Jun 1, 2023

@encukou as you may recall, we had a conversation during PyCon SK 2022 where you kindly offered your guidance and support. I'm now writing to kindly ask if you could take a look at this PR. I would greatly appreciate your review, or if you could suggest someone who might be the right person for this task. Thank you for your time and consideration!

@opavlyuk
Copy link
Contributor Author

opavlyuk commented Jul 6, 2023

@asvetlov could you please review this PR at your earliest convenience?
I understand that the ipaddress module has been without a maintainer for a while 😞
I'd appreciate if you could either take a look yourself or recommend someone appropriate for the task.

@asvetlov
Copy link
Contributor

The patch is good but I see a small problem.

>>> from ipaddress import IPv6Address
>>> str(IPv6Address("::1.2.3.4"))
'::102:304'

According to RFC 4291 section 2.2 part 3 the representation should be ::13.1.68.3.

Did I miss something?

@asvetlov
Copy link
Contributor

Ok, ::1.2.3.4 is not IPv4-Mapped IPv6 Address but IPv4-Compatible IPv6 Address.

The ipaddress library knows nothing about this subset of ipv6 addresses. I guess that the proper implementation should add .ipv4_compat property (similar to the already existing .ipv4_mapped) and check this property in __str()__ implementation.

It is doable but definitely not a part of this PR.

@asvetlov asvetlov enabled auto-merge (squash) July 31, 2023 13:30
@asvetlov asvetlov merged commit f22bf8e into python:main Jul 31, 2023
17 checks passed
@asvetlov
Copy link
Contributor

@opavlyuk thanks for the patch!

@opavlyuk
Copy link
Contributor Author

@asvetlov Apologies for the delayed response - I've just returned to my laptop after a long weekend and unfortunately missed your earlier comments.
I appreciate your support and find collaborating with you an absolute pleasure!

@opavlyuk
Copy link
Contributor Author

The ipaddress library knows nothing about this subset of ipv6 addresses. I guess that the proper implementation should add .ipv4_compat property (similar to the already existing .ipv4_mapped) and check this property in str() implementation.
It is doable but definitely not a part of this PR.

@asvetlov You're making good sense. Could you kindly provide some guidance on the best approach to address this, assuming it doesn't consume too much of your time?
Would it be proper to initiate a thread in python-ideas, or should we create a PR first before escalating the discussion? Perhaps there's an even more effective strategy?

@asvetlov
Copy link
Contributor

asvetlov commented Aug 1, 2023

I don't think that a thread in python-ideas will draw attention. It is a relatively rarely used thing.

You can just make a PR, and after a review it could be merged.

The PR should contain the new property definition, using it in all places where ipv4_mapped is accessed (__str__, .exploded, .is_private). Plus tests and documentation for the new property.

It doesn't sound like a lot of work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PR or inactive for long period of time. stdlib Python modules in the Lib dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants