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

Orders ipv6 addresses before v4 in resolution (needs a happy eyeballs implementation that supports feeding in resolved addresses) #233

Closed
wants to merge 3 commits into from

Conversation

tlm
Copy link

@tlm tlm commented Jul 4, 2022

When performing mdns or dns resolution for a given hostname v4
addresses are always returned for the connection over v6 when an address
supports dual stacks.

This allows for ipv6 transitioning and testing in a dual stack
environment. It also follows common practice when picking the right
address to try first.

Not addressed by this commit is the ability to fall back to other
resolved addresses should the first address fail. This feature is
addressed by other todo's in the project.

When performing  mdns or dns resolution for a given hostname v4
addresses are always returned for the connection over v6 when an address
supports dual stacks.

This allows for ipv6 transitioning and testing in a dual stack
environment. It also follows common practice when picking the right
address to try first.

Not addressed by this commit is the ability to fall back to other
resolved addresses should the first address fail. This feature is
addressed by other todo's in the project.
@tlm tlm force-pushed the ipv6-preference branch from e8c5615 to e2f5524 Compare July 4, 2022 22:23
@bdraco bdraco changed the title Orders ipv6 addresses before v4 in resolution. Orders ipv6 addresses before v4 in resolution Dec 14, 2022
@bdraco
Copy link
Member

bdraco commented Dec 14, 2022

Here is the logic we use in Home Assistant Core to get the preferred ip.

We tried to return IPv6 first but it caused too many problems for users

    for address in addresses:
        ip_addr = ip_address(address)
        if (
            not ip_addr.is_link_local
            and not ip_addr.is_unspecified
            and ip_addr.version == 4
        ):
            return str(ip_addr)
    # If we didn't find a good IPv4 address, check for IPv6 addresses.
    for address in addresses:
        ip_addr = ip_address(address)
        if (
            not ip_addr.is_link_local
            and not ip_addr.is_unspecified
            and ip_addr.version == 6
        ):
            return str(ip_addr)
    return None

1 similar comment
@bdraco
Copy link
Member

bdraco commented Dec 14, 2022

Here is the logic we use in Home Assistant Core to get the preferred ip.

We tried to return IPv6 first but it caused too many problems for users

    for address in addresses:
        ip_addr = ip_address(address)
        if (
            not ip_addr.is_link_local
            and not ip_addr.is_unspecified
            and ip_addr.version == 4
        ):
            return str(ip_addr)
    # If we didn't find a good IPv4 address, check for IPv6 addresses.
    for address in addresses:
        ip_addr = ip_address(address)
        if (
            not ip_addr.is_link_local
            and not ip_addr.is_unspecified
            and ip_addr.version == 6
        ):
            return str(ip_addr)
    return None

@tlm
Copy link
Author

tlm commented Dec 14, 2022

Thanks @bdraco for the information. Are you able to provide a link to the code in ha core?

Are you able to elaborate on the problems faced when preferring v6 over v4 first in a dual stack environment? This method is considered the best practice in RFC's.

I know from my own personal env I would prefer it if HA would be more v6 first. Makes it very hard to go completely single stack for IOT devices.

@bdraco
Copy link
Member

bdraco commented Jan 21, 2023

Thanks @bdraco for the information. Are you able to provide a link to the code in ha core?
https://github.com/home-assistant/core/blob/dev/homeassistant/components/zeroconf/__init__.py#L557

Are you able to elaborate on the problems faced when preferring v6 over v4 first in a dual stack environment? This method is considered the best practice in RFC's.

Lots of users have broken IPv6. The issue queue in HA is full of them.

I know from my own personal env I would prefer it if HA would be more v6 first. Makes it very hard to go completely single stack for IOT devices.

If we were to do IPv6 we would need to implement happy eyeballs https://en.wikipedia.org/wiki/Happy_Eyeballs

@bdraco
Copy link
Member

bdraco commented Jun 24, 2023

zeroconf now has a guarantee on ip address order return that matches the above pattern.

https://github.com/python-zeroconf/python-zeroconf/blob/d713a458daf6a57eea934b384cc9e534b33fd334/src/zeroconf/_services/info.py#L244

This should make it a lot simpler.

https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_connection

We can drop _connect_socket_connect and let asyncio make the connection in _connect_init_frame_helper with happy_eyeballs_delay of 0.25

@bdraco
Copy link
Member

bdraco commented Jul 7, 2023

@tlm Is this something you are still interested in pursuing?

@tlm
Copy link
Author

tlm commented Jul 8, 2023

@bdraco It is, but not something I have the time to finish up at the moment. Happy to close and I can breathe some more life into this another day.

@bdraco bdraco marked this pull request as draft July 9, 2023 23:15
@bdraco
Copy link
Member

bdraco commented Nov 6, 2023

We effectively have same problem as aio-libs/aiohttp#4451 (comment)

@bdraco bdraco changed the title Orders ipv6 addresses before v4 in resolution Orders ipv6 addresses before v4 in resolution (needs a happy eyeballs implementation that supports feeding in resolved addresses) Nov 6, 2023
bdraco added a commit that referenced this pull request Dec 10, 2023
replaces and closes #233
bdraco added a commit that referenced this pull request Dec 10, 2023
replaces and closes #233
@bdraco
Copy link
Member

bdraco commented Dec 11, 2023

@tlm Any interest in testing #789

@tlm
Copy link
Author

tlm commented Dec 11, 2023

Would love to @bdraco

@bdraco bdraco closed this in #789 Dec 12, 2023
@bdraco
Copy link
Member

bdraco commented Dec 12, 2023

#789 is available in 20.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants