Update protocol examples to support all network types #13249 (IDFGH-12196)
This updates the protocol examples to support all network types, without any reconfiguration.
See related issue for details: https://github.com/espressif/esp-idf/issues/13249
The default configuration now supports IPv4-only, IPv6-only, and dual-stack networks (either with or without DNS64). This means it works without needing to know the target network (the examples could be run by anyone).
Both IPv4 and IPv6 are enabled, with IPv6 fully enabled for SLAAC, RDNSS, and DHCPv6, however neither IPv4 nor IPv6 are mandatory (the old example treated enable as meaning also mandatory, so required reconfiguration based on the network, so would not necessarily work out of the box).
The default behaviour for each network type is explained in the readme.
The change has been tested using ESP32 with WiFi for:
- Dual stack network -- connecting to IPv6 only, IPv4 only, and dual stack servers
- Dual stack network without DNS64 -- connecting to IPv4 only server, showing DNS lookup fallback
- IPv6 only network with DNS64 -- connecting to dual stack and IPv4 (using DNS64)
- IPv4 only network -- connecting to dual stack server
- IPv4 only network -- connecting to IPv6 server (fails as expected, as no IPv4 address available)
The change is broken up into the following commits:
- Add an example function for DNS lookup that if there is a global scope IPv6 address then it tries IPv6 first, then falls back to IPv4 (for IPv4-only servers); if there is no global IPv6 then it only checks IPv4.
- Turn on full IPv6 support by default (RA for address, RDNSS and DHCPv6 for DNS), and update wifi connect to wait for any preferred (global) address, not both addresses. This means it will work in IPv4-only, IPv6-only, and dual-stack.
- Update the http_request example to use the example DNS function, so that both IPv6 and IPv4 DNS lookups are tried (depending on available addresses). Test destination addresses were also updated to use IPv6 test endpoints that provide IPv4-only, IPv6-only, and dual-stack addresses, and that return the observed address (useful to check NAT44 and NAT64).
- Apply the changes to eth_connect (ppp_connect already support ANY address).
- Fix to the PPP code that failed compilation if LWIP_IPV4 is turned off. There was a call to a macro that only exists if certain config was enabled, so I wrapped the call in the same flags.
Assistance required
I have made corresponding changes to the Ethernet connect, but I only have WiFi so have not been able to test. I have checked the code compiles, but can't deploy and run. PPP did not require any changes, it was already configured to wait for ANY (optional), not ALL (mandatory), but still needs testing.
If there is someone who could assist with verifying the branch on Ethernet and PPP that would be appreciated.
| Messages | |
|---|---|
| :book: | You might consider squashing your 5 commits (simplifying branch history). |
π Hello sgryphon, we appreciate your contribution to this project!
π Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.
ποΈ Please also make sure you have read and signed the Contributor License Agreement for this project.
Click to see more instructions ...
This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.
DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.
Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (π) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.
Review and merge process you can expect ...
We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.
This GitHub project is public mirror of our internal git repository
1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.
Generated by :no_entry_sign: dangerJS against 55880ab766521eb8615aee85456224931c63b664
Based on feedback for my other PR, I used sdkconfig.defaults for setting defaults (instead of repeating configs in KConfig).
@sgryphon Thanks, for your contribution. We saw the PR and will need some time to evaluate it.
Code / documentation has been updated based on feedback received.
@sgryphon Thanks, for your contribution. We saw the PR and will need some time to evaluate it.
@espressif-abhikroy It has been 4 Months since your last reply, any progress about this PR?
Hi @sgryphon
Thank you for the comprehensive work and the PR with detailed description. We appreciate the effort you've put into enhancing the protocol examples to demonstrate handling DNS in IPv6/only network types.
I apologize that it has taken this much time to process -- it fell behind the radar due to its complexity and the potential impact on other components and other code owners.
After careful consideration, I have some concerns regarding the default configurations, particularly with backward compatibility and the fact that not all users are on IPv6 networks, especially IPv6-only networks; and as the common connect component is widely used in our protocol tests (where we require the mandatory aspect of waiting for addresses) and, at the same time this component is not a good fit for connecting in real life applications:
https://github.com/espressif/esp-idf/blob/d7ca8b94c852052e3bc33292287ef4dd62c9eeb1/examples/protocols/README.md?plain=1#L13
To maintain the current stability and ensure a smoother experience for all users, I would suggest the following approach:
- squash these two commits: 7ffd2320a7fcdef699c12760a14bc4a6a3b5faef f4cf43525545a82fb1e65b784b05c68d58a89c35
- move
example_getaddrinfo()from common connect component to the http request example - rename
examples/protocols/http_request/sdkconfig.defaultstosdkconfig.ci.full_ipv6and document it in theREADME.md- the defaults would be unchanged
- users could still choose the preconfigured, full IPv6 stack with
idf.py ... -DSDKCONFIG_DEFAULTS="sdkconfig.ci.full_ipv6" ...
- and drop all other changes (you can create a followup PR adding support for the "ANY" address, but I'd suggest taking a similar approach to the PPP scheme and keeping the defaults as they are)
Such a change would make about 100 lines in one example only with no impact to other components/tests, thus much easier for us to accept. That would also cover 99.9% of the usecases (with the default common connect config, i.e. waiting for IPv4 and LL IPv6, so people with global address capabilities would first get an IPv4 and continue with the example, while getting the global address later -- exactly the same behavior as the 'ANY' option you've introduced). The remaining 0.1% (correct me if I'm wrong) would represent IPv6 only networks, for which we'd require users to disable IPv4 (wouldn't be needed for the 'ANY' option you've proposed) -- which I think is acceptable.
PS: Here's my feedback to the commits:
- 7ffd232 feat(example/protocols): Add DNS fuction to check IPv6 first if relevant
Nice idea to demonstrate running
getaddrinfo()with preferred IPv6 addresses, but I think it's better to create a separate example with that or move it outside of common-connect component
- 5752122 feat(example/protocols): Enable full IPv6 stack; default wait for anyβ¦
As mentioned above, we'd still like to keep the dualstack option with preferred IPv4, as it's simply (still) the most common scenario. Good idea to introduce this 'ANY' address config, just don't think it should be enabled by default.
- f4cf435 feat(example/protocols): Update http_request to support all networks
:+1: Nice work, this could be accepted immediately, just suggest making the changes as outlined above.
- 3b8596c feat(example/protocols): Add default wait for any address to eth_connect
Same as 5752122
- 55880ab fix(ppp): Only call peer DNS function if configured
:+1: Thanks for the fix, this issue has been already fixed on master by addressing unrelated PPP issue:
https://github.com/espressif/esp-idf/blob/d7ca8b94c852052e3bc33292287ef4dd62c9eeb1/components/esp_netif/lwip/esp_netif_lwip_ppp.c#L234-L236
The getaddrinfo() system call in lwIP within ESP-IDF has a limitation when using AF_UNSPEC, as it defaults to returning only an IPv4 address in dual-stack mode.
This issue has been addressed in commit https://github.com/espressif/esp-idf/commit/e2ae81a101da18dc28fef1db3a76039cf4629d82. When enabled, the behavior is now consistent with Linux, supporting both IPv4 and IPv6 resolutions as expected.
However, the feature is currently disabled by default.
To enable it, you can turn on the CONFIG_LWIP_USE_ESP_GETADDRINFO option. This can be configured in the menuconfig under:
Component config -> LWIP -> DNS -> Enable esp_getaddrinfo() instead of lwip_getaddrinfo()
Closing as the problem's been addressed differently in https://github.com/espressif/esp-idf/pull/13250#issuecomment-2516063870