Extend docker/net utils to allow port checks for UDP ports#7955
Merged
Extend docker/net utils to allow port checks for UDP ports#7955
Conversation
3b45671 to
4118d8d
Compare
4118d8d to
abcde65
Compare
simonrw
approved these changes
Mar 24, 2023
Contributor
simonrw
left a comment
There was a problem hiding this comment.
Looks good, a minor suggestion about the API surface of localstack.utils.net but not blocking
| port: int, http_path: str = None, expect_success=True, retries=10, sleep_time=0.5 | ||
| ): | ||
| """Ping the given network port until it becomes available (for a given number of retries). | ||
| """Ping the given TCP network port until it becomes available (for a given number of retries). |
Contributor
There was a problem hiding this comment.
Perhaps the functions in this file could all be updated to take IntOrPort (or Port if they are not part of the public API), then the entire utility file would be protocol agnostic. What do you think? Perhaps an update for a future PR.
Member
Author
There was a problem hiding this comment.
Good point, agreed. Would probably leave that for a future iteration.. 👍
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
4 participants
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.
Extend docker/net utils to allow port checks for UDP ports. Addresses an issue from a support channel where starting up the LS container fails due to UDP port 53 already being bound.
This PR extends
container_ports_can_be_bound(..)to allow us to make port availability checks also for UDP ports, which will allow us to selectively add the port bind for UDP port 53, based on availability.Tried to make the changes minimally invasive, and backwards-compatible. Ports can be specified as an
int(in which case we assume TCP as the default protocol), or as aPortinstance that contains the protocol explicitly.The existing
TestDockerPortstest has been parameterized to also cover UDP ports, and a new simple test has been added to cover the UDP-based logic forport_can_be_bound(..).Debatable whether this is a new feature or a fix 🤷 - but since it addresses a support case, arguably this could still be considered for the pre v2 merges (despite the feature freeze). To be discussed..
/cc @simonrw @dfangl