Network refactoring - fix some IPv6 DNS issues #9439
Conversation
👋 Hello sgryphon, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
sgryphon
force-pushed
the
sgryphon/refactor-dns-getaddrinfo-ipv6
branch
from
fd77166
to
44940e4
Compare
Can you open a PR for this in the lib-builder? Options need to be added to this file. Then in the PR, the CI will generate new libs that you can use to test the feature |
Thanks. I have built the libs locally with the needed config, but need to test it out. Also, there may be some corresponding changes in Arduino-ESP32, e.g. not sure if it reports DNS servers correctly. |
So, it had finished building the libs overnight, and it worked. Well, internally LWIP is using the IPv6 DNS servers, and the IPv6-only to IPv6-only scenario works, but I need to updates to Arduino-ESP32 as well:
To properly fix this second issue requires my patch to LWIP, espressif/esp-lwip#66 In the meanwhile, I can add a workaround in Arduino-ESP32, based on the code in my ESP-IDF examples updates: espressif/esp-idf#13250 Here is IPv6 - IPv6 working: |
The related PR for libs is here: espressif/esp32-arduino-lib-builder#166 Note that the changes are independent, i.e. even with the old DNS the libs update will help IPv6-only scenarios. |
|
I have added a work around for IPv6-only networks, by, if you have a public IPv6 address, then check IPv6 first. Most scenarios now work, with only failing on TLS from an IPv6-only network to a dual-stack host:
The third commit implements the work around, and is independent of fixing libs, but not much use if the libs don't have IPv6 DNS enabled (it will change the preference in dual-stack). I am not sure if you want the first commit as part of this pull request, or separate. Separate might make it easy to review (and back out once LWIP is patched). Some examples: IPv6-only to dual-stack, that fails without the workaround: IPv6-only to IPv4-only, also works via DNS64+NAT64, by using a synthentic IPv6 DNS64 address -- effectively making the destination dual-stack. From the point of view of the destination the source address appears to be the IPv4 NAT address (same as for NAT44). Failure, IPv6-only via HTTPS/TLS to dual-stack. Because TLS needs to use the hostname (for checking certificates), I presume it resolves internally in LWIP, similar to ESP-IDF. This will only be fixed when ESP-LWIP is updated to fix IPv6 DNS resolution (patch is being reviewed). |
|
Failures are related to change of |
sgryphon
force-pushed
the
sgryphon/refactor-dns-getaddrinfo-ipv6
branch
from
8287cf8
to
f8336de
Compare
| @@ -82,8 +45,9 @@ static esp_err_t wifi_gethostbyname_tcpip_ctx(void *param) | |||
| */ | |||
| int NetworkManager::hostByName(const char* aHostname, IPAddress& aResult, bool preferV6) | |||
There was a problem hiding this comment.
looks like preferV6 is no longer needed
There was a problem hiding this comment.
I'm not really sure what preferV6 was supposed to be for. It seemed to be a way to say in advance what type of address you want, except the whole point of DNS is to only know the name and not the address.
i.e. www.google.com -- yesterday it was IPv4 only, today it is dual stack, tomorrow it will be IPv6 only. But the code shouldn't care.
The programmer won't know where the device/code will be used, so can't specify what to use in advance.
The rules for destination addresses of RFC 6724 are to use whatever matches, i.e. if you have a public IPv6 and the destination is public IPv6, then use that; otherwise if you only have IPv4 then use that. This needs to be determined dynamically at run time. (what the patch to ESP-LWIP does, or the work around does as a short cut).
There was a problem hiding this comment.
Note that in some utility programs you might want to display all DNS lookups. Unfortunately LWIP is limited to only one result, although you could get one IPv6 and one IPv4.
Although in that case you probably want at least 3 options -- the default to give whatever matches (4 or 6), and then an explicit way to look up either v4 or v6.
Note that a utility program might be running in an IPv4-only network, so the default will give a v4 address, but they might want to show the user both available addresses.
Although at that point, for such a utility, probably Arduino is not the right solution (i.e. go to underlying ESP-IDF).
I will remove the option (I think it was only added as part of the original refactoring).
There was a problem hiding this comment.
it was there exactly so some that want to prefer using V6 address, can say so. Then the DNS will return the addresses in the requested order. By default it will return the v4 if available.
There was a problem hiding this comment.
Okay, but I never understood how a programmer would know in advance what type of address is going to be needed.
Dynamic determination is much better, because that allows a device, or sample code, to run in any network.
Now that I have that working, there is no need for a programmer to have to use the option (and change configuration/recompile every time they change networks). The dynamic code will work in any network, for any destination.
There was a problem hiding this comment.
yes, I am fine with that. Please remove the argument :)
| if (!aResult.fromString(aHostname)) { | ||
| Network.waitStatusBits(NET_DNS_IDLE_BIT, 16000); | ||
| Network.clearStatusBits(NET_DNS_IDLE_BIT | NET_DNS_DONE_BIT); |
There was a problem hiding this comment.
Removing any reference to NET_DNS_IDLE_BIT and NET_DNS_DONE_BIT is also necessary. And if Network.[get|set|wait]StatusBits are only used for those two flags, then we can probably get rid of that too.
There was a problem hiding this comment.
I'll look at cleaning up.
| @@ -82,8 +45,9 @@ static esp_err_t wifi_gethostbyname_tcpip_ctx(void *param) | |||
| */ | |||
| int NetworkManager::hostByName(const char* aHostname, IPAddress& aResult, bool preferV6) | |||
| { | |||
| // IDEA: Rename to getAddressInfo() ? | |||
There was a problem hiding this comment.
let's keep this for historic reasons
There was a problem hiding this comment.
I'll remove the comment
|
@sgryphon any clues when yo can finish this? We can take over also to finalize the changes |
I do have a day job, but get some time on weekends and the occasional evening. One thing that is probably key if you can help at all, if you know the right people, is the change for ESP-LWIP, which has not been assigned to anyone, with no comments or feedback or anything. espressif/esp-lwip#66 |
If the result from esp_netif_get_dns_info is an IPv6 address, then parse to an IPAddress.
|
I can't comment on IDF's PR policies, as that is a totally separate project with it's subprojects and so on. My question was about this particular PR that is not connected to your PR in IDF. If you can wrap it up over that weekend that would be great. We kinda want to be able to release RC1 soon |
sgryphon
force-pushed
the
sgryphon/refactor-dns-getaddrinfo-ipv6
branch
from
f8336de
to
22034bf
Compare
Replace hostbyname with getaddrinfo for better IPv6 support. The API is also simpler, as it has no callbacks (they are handled internally). Allows dual-stack networks to connect to IPv6-only destinations. Still does not work for IPv6-only networks, as IPv6 DNS is not enabled in the pre-built ESP-IDF libraries.
sgryphon
force-pushed
the
sgryphon/refactor-dns-getaddrinfo-ipv6
branch
from
22034bf
to
5634137
Compare
By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes
Checklist
This entire section above can be deleted if all items are checked.
Description of Change
Allows dual-stack networks to connect to IPv6-only destinations.
Replace hostbyname with getaddrinfo for better IPv6 support. The API is also simpler, as it has no callbacks (they are handled internally).
Note: Still does not work for IPv6-only networks, as IPv6 DNS is not enabled in the pre-built ESP-IDF libraries.
Also includes an update to
dnsIP()to correctly return IPv6 addresses (they do work internally, when enabled, just weren't being returned correctly).Tests scenarios
Tested on M5Stack Core2 (ESP32), on dual-stack, IPv4-only, and IPv6-only networks (with DNS64/NAT64), for dual-stack. IPv4-only, and IPv6-only destination servers.
The IPv4-only and dual-stack networks work for all destination servers, although due to a limitation in LWIP the dual-stack to dual-stack scenario uses the IPv4 address (instead of IPv6).
I have a patch submitted for ESP-LWIP to fix this issue, and in the meanwhile also have a workaround I can implement for Arduino-ESP32.
Note that IPv6-only networks do not work yet. They are reporting no-DNS, probably because the underlying ESP-IDF libraries do not have IPv6 DNS enabled.
Example outputs:
(These are from my own test app, at https://github.com/sgryphon/iot-demo-build/tree/feature/arduino-esp32-v3-update/m5stack/m5unified_wifi_https -- I will look at making sure a simpler test example in Arduino-ESP32 also works.)
Dual-stack network, to IPv6-only destination:
Failure - IPv6 only network, to dual-stack host. No IPv6 DNS available. (LWIP does support IPv6 DNS, but not enabled in library build)
Related links
Partially addresses the issues in #9143, and in the IPv6 discussion.