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

Update protocol examples to support all network types #13249 (IDFGH-12196) #13250

Closed

Conversation

sgryphon
Copy link

@sgryphon sgryphon commented Feb 24, 2024

This updates the protocol examples to support all network types, without any reconfiguration.

See related issue for details: #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:

  1. 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.
  2. 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.
  3. 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).
  4. Apply the changes to eth_connect (ppp_connect already support ANY address).
  5. 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.

@CLAassistant
Copy link

CLAassistant commented Feb 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

github-actions bot commented Feb 24, 2024

Messages
📖 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 🚫 dangerJS against 55880ab

@espressif-bot espressif-bot added the Status: Opened Issue is new label Feb 24, 2024
@github-actions github-actions bot changed the title Update protocol examples to support all network types #13249 Update protocol examples to support all network types #13249 (IDFGH-12196) Feb 24, 2024
@sgryphon sgryphon force-pushed the sgryphon/update-protocol-examples branch 3 times, most recently from 9341632 to 67e5c6f Compare February 24, 2024 08:39
@sgryphon sgryphon force-pushed the sgryphon/update-protocol-examples branch from 67e5c6f to 495183a Compare February 24, 2024 10:23
@sgryphon sgryphon force-pushed the sgryphon/update-protocol-examples branch 3 times, most recently from 2b88b53 to df60cc4 Compare March 13, 2024 13:28
@sgryphon
Copy link
Author

Based on feedback for my other PR, I used sdkconfig.defaults for setting defaults (instead of repeating configs in KConfig).

@espressif-abhikroy
Copy link
Collaborator

@sgryphon Thanks, for your contribution. We saw the PR and will need some time to evaluate it.

Copy link
Collaborator

@krzychb krzychb left a comment

Choose a reason for hiding this comment

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

@sgryphon, thank you for preparing detailed documentation!

@sgryphon sgryphon force-pushed the sgryphon/update-protocol-examples branch from df60cc4 to 60a5fec Compare March 29, 2024 03:15
If we have an IPv6 global scope address (on any interface), then check IPv6
first, and if that fails fall back to IPv4. Global scope includes ULA (if
ULA is configured, similar to private address ranges, DNS is expected). If
we don't have a global IPv6, then only check IPv4.
… address

Enable full IPv6 networking (SLAAC, RDNSS, DHCPv6), so that the example will
work properly with IPv6 (and IPv4) if available. However by default wait for
any (either IPv4 or IPv6) global address, not both, so the example runs on
the first address received.

This allows the example to run in IPv4-only, IPv6-only, and dual-stack
networks, as we have no way to know which network the user will run the
example in we need to support all of them.

Config settings can still be used so these defaults can be turned off if you
want to run single stack, e.g. Matter devices only require IPv6 so you can
save space and effort by using IPv6-only.
The ppp_set_usepeerdns macro is only defined if both flags are enabled, so
breaks compilation trying to call it if not. Fix is to add the same flags
around the call, so the code now compiles.
@sgryphon sgryphon force-pushed the sgryphon/update-protocol-examples branch from 60a5fec to 55880ab Compare March 29, 2024 03:17
@sgryphon
Copy link
Author

Code / documentation has been updated based on feedback received.

@AxelLin
Copy link
Contributor

AxelLin commented Jul 27, 2024

@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?

@david-cermak
Copy link
Collaborator

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:

The simple `example_connect()` function does not handle timeouts, does not gracefully handle various error conditions, and is only suited for use in examples. When developing real applications, this helper function needs to be replaced with full Wi-Fi / Ethernet connection handling code. Such code can be found in [examples/wifi/getting_started/](../wifi/getting_started) and [examples/ethernet/basic/](../ethernet/basic) examples.

To maintain the current stability and ensure a smoother experience for all users, I would suggest the following approach:

  • squash these two commits: 7ffd232 f4cf435
  • move example_getaddrinfo() from common connect component to the http request example
  • rename examples/protocols/http_request/sdkconfig.defaults to sdkconfig.ci.full_ipv6 and document it in the README.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

👍 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

👍 Thanks for the fix, this issue has been already fixed on master by addressing unrelated PPP issue:

#if PPP_IPV4_SUPPORT
ppp_set_usepeerdns(ppp_obj->ppp, 1);
#endif

@espressif-abhikroy
Copy link
Collaborator

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 e2ae81a.
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()

@david-cermak
Copy link
Collaborator

Closing as the problem's been addressed differently in #13250 (comment)

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Won't Do This will not be worked on and removed Status: In Progress Work is in progress labels Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Won't Do This will not be worked on Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants