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

upstream upnp work #5272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stormshield-frb
Copy link
Contributor

Description

As discussed at the last maintainer call @jxs, I'm upstreaming our work on UPNP.

Our work began to allow us to listen on the unspecified address which was not working correctly since a single ListenerId can give multiple listen addresses and this was not correctly handled in the UPNP behaviour. However we also experienced some Pending blocking for ever but I don't remember the exact cause. That's why we ended up doing a more important change that we had expected. We also removed custom events from UPNP to replace them with the generic ExternalAddrConfirmed and ExternalAddrExpired.

This PR wants to be a starting point to integrate this work. IMHO, I don't thinks it is ready to merge as is, that's why I'm opening this so we can discuss what could/should be integrated and what should not.

As I have also mentioned during the call, there is some know issues:

  • this solution does not handle multiple gateways.
  • if you have multiple interfaces and some of them do not correspond to the gateway, we still ask the gateway to map it, which always results in an error.
  • UPNP behaviour could be a little more smart and if a port is already mapped on the Gateway (resulting in an error then), it could be interesting to send a new request to the Gateway asking it to give us a random port.

Notes & open questions

N/A

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link

mergify bot commented Apr 15, 2024

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

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

Successfully merging this pull request may close these issues.

None yet

2 participants