Add optional port_range to restrict host candidate local ports - #112
Open
junkerderprovinz wants to merge 2 commits into
Open
Add optional port_range to restrict host candidate local ports#112junkerderprovinz wants to merge 2 commits into
junkerderprovinz wants to merge 2 commits into
Conversation
Add an optional `port_range` keyword argument to `Connection`, allowing callers to restrict the local UDP ports used for host candidates to an inclusive `(min_port, max_port)` range. This is useful when the local ports must be known in advance, for example to set up static port forwarding on a NAT or to publish a fixed set of ports for a container. When `port_range` is set, each host candidate socket is bound by trying ports in the range in ascending order until a free one is found; if the whole range is exhausted an `OSError` is raised (and handled like any other bind failure). When `port_range` is `None`, an ephemeral port is used exactly as before. The range is validated at construction time and a `ValueError` is raised for invalid input. Only host candidate binding is affected; STUN, TURN and mDNS local ports are left untouched. Fixes aiortc#47.
|
The min_port should be Also I don't think this is going to be merged anytime soon since there's already a PR with similar changes; let's wait for few days, hopefully someone would come in for rescue. |
Per review: raise the port_range lower bound from 1 to 1024 so the system-reserved ports (0-1023) cannot be requested. Invalid ranges are rejected with a ValueError rather than clamped. Docstring and tests updated.
Author
|
Good call, done. The range now has to be within 1024-65535, and an out-of-range or inverted range is rejected with a No rush on the merge, understood. I'll leave it here for whenever it fits, and if the other PR lands first that's fine too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds an optional
port_rangekeyword argument toConnection, so callers can pin the local UDP ports used for host candidates to a fixed inclusive(min_port, max_port)range instead of letting the OS pick an ephemeral port.This is the feature requested in #47 and supersedes the now stale and conflicting #63, which I used as a starting point. It is rebased on current
mainand addresses the review feedback from that thread.Use cases (from #47 and #63):
Behavior:
port_rangeisNone(the default), an ephemeral port is used exactly as before, so existing callers are unaffected.port_rangeis set, each host candidate socket is bound by trying the ports in the range in ascending order until a free one is found. If the whole range is exhausted the bind fails like any other host bind failure, so that address simply yields no host candidate (this reuses the existingOSErrorhandling inget_component_candidates).ValueError.Addressing the review comments on #63:
get_component_candidatesuses an ephemeral local port. mDNS binds explicitly to 5353, and the STUN and TURN paths do not set a local address, so the change is confined to a single bind site. The server reflexive candidate reuses the host socket, so it inherits the restricted port automatically.API note: the 2022 review suggested
ephemeral_ports: Optional[Iterable[int]]. I went with aport_rangetwo-tuple because the concrete use cases (static NAT forwarding, container port publishing) are naturally contiguous ranges, it is trivial to validate, and it matches the port range option found in comparable ICE stacks. The internal binding loop does not depend on the shape, so if you prefer theIterable[int]form I am happy to switch.Tests (tests/test_ice.py):
test_invalid_port_range: an out of bounds or inverted range raisesValueError.test_gather_candidates_port_range: every gathered host candidate is bound within the configured range.test_gather_candidates_port_range_exhausted: when the range is fully occupied, no host candidate is produced. The tests mock host address discovery to stay on loopback and remain hermetic.