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

Allow port forwarding for udp ports #234

Closed
kunerd opened this issue Nov 14, 2020 · 5 comments · Fixed by #655
Closed

Allow port forwarding for udp ports #234

kunerd opened this issue Nov 14, 2020 · 5 comments · Fixed by #655
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@kunerd
Copy link

kunerd commented Nov 14, 2020

I'm trying to test a DNS server I build and for this to work I need to forward upd ports from inside the container to my host system.
The docker cli syntax to forward udp ports is -p 5353:53/udp. Currently, this is not possible with testcontainer, because Port is defined as:

pub struct Port {
    pub local: u16,
    pub internal: u16,
}

I'm willing to provide a PR for this, but I think we should discuss possible implementations first, because it will most probably result in a breaking change.

@thomaseizinger
Copy link
Collaborator

Hi @kunerd !

I am happy to support this feature!

Minimizing breaking changes is obviously nice but I wouldn't worry about it too much. The primary interface that consumers interact with is Container#get_host_port.

If we first deprecate that one in favor of one that takes the protocol into account somehow, consumers should have a reasonable easy way to upgrade. Feel free to making breaking changes on Port and all the machinery around it!

I am also happy to review a PoC PR that doesn't fully work yet but only outlines the idea :)

Thank you for contributing!

@kunerd
Copy link
Author

kunerd commented Nov 27, 2020

I think wrapping port in an Enum like below could work:

enum PortMapping {
    Tcp(Port),
    Udp(Port),
    Sctp(Port)
}

After looking up the Docker docs I realized that the -p option also allows to bind against a specific host address. Should we take this into account for this PR, too?

@thomaseizinger
Copy link
Collaborator

After looking up the Docker docs I realized that the -p option also allows to bind against a specific host address. Should we take this into account for this PR, too?

I think for now we should be fine with no caring about specific hosts. Limiting the port to a specific host seems to be more of a feature that is used when running containers in production somewhere and hence is rather of small relevance for testcontainers.

The PortMapping idea sounds good. Would you be willing to put up a draft PR so we can see how it pans out?

@DDtKey DDtKey added enhancement New feature or request help wanted Extra attention is needed labels May 30, 2024
@estigma88
Copy link
Contributor

Hi everyone, I will move this one forward, based on #246 work, let me know if it makes sense or any other strategy you have in mind

@estigma88
Copy link
Contributor

Created a initial PR to grab early feedback

@DDtKey DDtKey added this to the 0.18.0 milestone Jun 13, 2024
@DDtKey DDtKey closed this as completed in 7789466 Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
4 participants