Add udp and sctp support to port mappings #246

kunerd · 2021-01-25T21:13:58Z

Resolves #234

kunerd · 2021-01-25T21:40:02Z

Maybe a better solution for the PortMapping would look like this:

enum Protocol {
    Tcp,
    Udp,
    Sctp
}

struct Ports {
    host,
    container
}

struct PortMapping {
    protocol: Protocol,
    ports: Ports
}

Do we really need the lookup of the mapped host port for an internal port?
https://github.com/testcontainers/testcontainers-rs/blob/dev/src/core/docker.rs#L82

thomaseizinger · 2021-01-27T03:26:28Z

Do we really need the lookup of the mapped host port for an internal port?
https://github.com/testcontainers/testcontainers-rs/blob/dev/src/core/docker.rs#L82

Yes we need that otherwise we don't know what ports the OS assigned to us if we start the containers with -P.

Also, have you tried running the tests locally with cargo test? You seem to have some compilation errors :)

kunerd · 2021-01-27T14:44:26Z

I ok, now this makes sense. I could imagine two interface to get the mapped host port. Either something like:

enum Port {
     Tcp(u16),
     Udp(u16),
     Sctp (u16)
}
....
pub fn get_host_port(&self, internal_port: Port) -> Option<u16> {
    ...
}

or

pub fn get_host_port_tcp(&self, internal_port: u16) -> Option<u16> {
    ...
}
pub fn get_host_port_udp(&self, internal_port: u16) -> Option<u16> {
    ...
}

Edit the single functions just need to resturn Option<u16>.
Edit 2 I meant the get_host_port function
I should work on this in the evening / at night :)

thomaseizinger · 2021-01-28T07:50:08Z

Thanks for your proposal!

I think it will be valuable if the default case (a TCP port mapping) is easily accessible. As such, what do you think about an API along the lines of:

enum Protocol {
	Tcp,
	Udp,
	Sctp
}

pub fn map_to_host_port(&self, internal_port: u16) -> Option<(u16, Protocol)> {
    ...
}

A caller could then simply discard the protocol if they don't care about it (or if they know that it is always a specific one) like this:

let (port, _) = container.map_to_host_port(8080).unwrap();

This actually got me thinking, do we ever care about which protocol this port was mapped on? I feel like in the context of testcontainers, we actually have an expectation in that regard.

The postgres container for example will always expose a TCP port mapping on port 5432. The information of Protocol::Tcp that would be returned by the above function is pretty useless. I already know that from the context of the test.

What I am trying to say is: Can we maybe just leave the mapping API as is and simply search for the mapped port in the PortMapping response and discard the protocol information?

kunerd · 2021-01-28T21:32:16Z

What I am trying to say is: Can we maybe just leave the mapping API as is and simply search for the mapped port in the PortMapping response and discard the protocol information?

That would be nice, but unfortunately it's possible to bind different protocols to the same port. Something as -p 8080:80 -p 8080:80 udp is totally valid. I also updated my post above, because I meant the get_host_port function and it should return an Option with a u16 instead of a Port.

Another possible solution could be to use the type system. For the TCP case one could just call the functions (e.g. map_to_host_port or get_host_port) with an u16 as it was before. If one would like to set/get a UDP or SCTP port the calling would look something like this:

let port = container.get_host_port(UdpPort(8080)).unwrap()

thomaseizinger · 2021-02-02T06:48:37Z

What I am trying to say is: Can we maybe just leave the mapping API as is and simply search for the mapped port in the PortMapping response and discard the protocol information?

That would be nice, but unfortunately it's possible to bind different protocols to the same port. Something as -p 8080:80 -p 8080:80 udp is totally valid. I also updated my post above, because I meant the get_host_port function and it should return an Option with a u16 instead of a Port.

That is interesting! Thanks for sharing!

I think we might be able to achieve a nice API with some trait magic:

Imagine get_host_port with a signature like:

fn get_host_port<P>(&self, internal_port: P) -> u16 where P: Into<(u16, Protocol)> {
	let (port, protocol) = internal_port.into();
	// ...
}

We can then implement the following traits:

impl Into<(u16, Protocol)> for &str {
	fn into(self) -> (u16, Protocol) {
		// parse self as '{port}/{udp,tcp,sctp}'
		// panic in parsing code if necessary, we are only within tests so panicking is fine
		// return tuple of port and protocol
	}
}

impl Into<(u16, Protocol)> for u16 {
	// default to TcpProtocol for implementations of port
}

The user can then call the function in various ways:

container.get_host_port(8080);
container.get_host_port("8080/tcp");
container.get_host_port("8080/udp");

Note that I changed the get_host_port API recently to panic if the port is not mapped to make it more ergonomic.

kunerd · 2021-02-20T00:14:49Z

Hi, the current solution should be fully backward compatible. I think it still needs some refactoring and some test, but then it should be ready to go. Would like to here your thoughts.

thomaseizinger

Thanks for working on this!

Please note that parts of the codebase changed quite a bit with the recent work on an async interface and the shiplift integration. Can you rebase this PR onto latest dev please?

I've also adapted the parsing code for the ports a bit so it is hopefully easier to adapt to extract the protocol.

I think there are two things we need to still adapt here:

Users need to be able to specify the protocol when resolving the port. Currently the trait bound is only Into<u16>.
We should also adapt the API of RunArgs such that users can configure arbitrary port mappings there.

Let me know how the rebase goes. If it is too cumbersome, it might be easier to just start again from current dev! Sorry for the inconvenience.

thomaseizinger · 2021-02-20T01:32:14Z

src/core/container.rs

-    pub fn get_host_port(&self, internal_port: u16) -> Option<u16> {
+    pub fn get_host_port<T>(&self, internal_port: T) -> Option<u16>
+    where
+        T: Into<u16> + Copy,


If this is only Into<u16>, how are we going to map a non-TCP port? As far as I understand, the user needs to able to specify the protocol here.

We might need to create our own trait here due to coherence rules.

The Into<u16> is misleading here, it only is necessary for the debug print, I think it would be better to switch to {:?} there. The next line is the important one, because it let us forward the call to the type specific implementation of map_to_host_port. You can try it in this Playground example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=100daefbe4f085548bfc3c620f522b82
I should even change the return value into Option<T>, so that the user couldn't accidentally mix up different port types - missed that here, because it was a bit late as a have done the commit.
I think the same idea should work for the configuration part.

kunerd · 2021-02-22T20:36:06Z

Now, only additional documentation and some test are missing.

kunerd · 2021-02-25T08:18:55Z

I have tried it with my original use case and it seems to work as expected. But, I think the RunArgs::with_mapped_port() needs to be more ergonomic to use.

thomaseizinger

Thank you!

I think we still need to iron out a few details before this can merge :)

thomaseizinger · 2021-03-07T22:35:15Z

src/core/container.rs

            .unwrap_or_else(|| {
                panic!(
-                    "container {} does not expose port {}",
+                    "container {:?} does not expose port {:?}",


Why do we need to print self.id as Debug now?

thomaseizinger · 2021-03-07T22:37:39Z

src/core/ports.rs

+                    "sctp" => {
+                        mappings.sctp.insert(Sctp(internal), Sctp(external));
+                    }
+                    _ => panic!("Not a valid port mapping."),


Suggested change

_ => panic!("Not a valid port mapping."),

p => panic!("Unknown protocol '{}'", p),

thomaseizinger · 2021-03-08T04:38:38Z

src/core/container.rs

@@ -128,13 +133,17 @@ impl<'d, I> Container<'d, I> {
    /// This method panics if the given port is not mapped.
    /// Testcontainers is designed to be used in tests only. If a certain port is not mapped, the container
    /// is unlikely to be useful.
-    pub fn get_host_port(&self, internal_port: u16) -> u16 {
+    pub fn get_host_port<T>(&self, internal_port: T) -> T


How does one go about calling this API? From what I can see, everything that implements MapToHostPort is actually private and can't be used by users outside of the library.

To make sure this doesn't happen, can you add an example in examples/ (dir doesn't exist yet). Rust examples are compiled as part of the tests so that will be a good way of ensuring people can actually use the API :)

thomaseizinger · 2021-03-08T04:39:23Z

src/core/ports.rs

            .into_iter()
            .filter_map(|(internal, external)| {
-                // internal is '8332/tcp', split off the protocol ...
-                let internal = internal.split('/').next()?;
+                // internal is ')8332/tcp', split off the protocol ...


A typo got into this comment :)

Suggested change

// internal is ')8332/tcp', split off the protocol ...

// internal is '8332/tcp', split off the protocol ...

DDtKey · 2024-04-06T17:11:09Z

Closing due to inactivity and outdated state.
Feel free to open a new PR, but keep in mind that we also going to deprecate existing cli client as part of #563

thomaseizinger reviewed Feb 20, 2021

View reviewed changes

Add udp, sctp, port mappings to container

9581810

kunerd force-pushed the add_udp_sctp_port_mappings branch from 4b2644d to 9581810 Compare February 22, 2021 19:41

Add support for udp, sctp to the image and client

6e70ec8

kunerd added 4 commits February 22, 2021 21:45

Fix unit tests

b715f9c

Fix code style

7fdedf2

Fix code style

f86e039

Fix code style

95a4dff

thomaseizinger requested changes Mar 8, 2021

View reviewed changes

DDtKey closed this Apr 6, 2024

estigma88 mentioned this pull request Jun 11, 2024

Allow port forwarding for udp ports #234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add udp and sctp support to port mappings #246

Add udp and sctp support to port mappings #246

kunerd commented Jan 25, 2021

kunerd commented Jan 25, 2021 •

edited

Loading

thomaseizinger commented Jan 27, 2021

kunerd commented Jan 27, 2021 •

edited

Loading

thomaseizinger commented Jan 28, 2021 •

edited

Loading

kunerd commented Jan 28, 2021 •

edited

Loading

thomaseizinger commented Feb 2, 2021 •

edited

Loading

kunerd commented Feb 20, 2021

thomaseizinger left a comment

thomaseizinger Feb 20, 2021

thomaseizinger Feb 20, 2021

kunerd Feb 22, 2021 •

edited

Loading

kunerd commented Feb 22, 2021

kunerd commented Feb 25, 2021

thomaseizinger left a comment

thomaseizinger Mar 7, 2021

thomaseizinger Mar 7, 2021

thomaseizinger Mar 8, 2021

thomaseizinger Mar 8, 2021

DDtKey commented Apr 6, 2024 •

edited

Loading

	_ => panic!("Not a valid port mapping."),
	p => panic!("Unknown protocol '{}'", p),

	// internal is ')8332/tcp', split off the protocol ...
	// internal is '8332/tcp', split off the protocol ...

Add udp and sctp support to port mappings #246

Add udp and sctp support to port mappings #246

Conversation

kunerd commented Jan 25, 2021

kunerd commented Jan 25, 2021 • edited Loading

thomaseizinger commented Jan 27, 2021

kunerd commented Jan 27, 2021 • edited Loading

thomaseizinger commented Jan 28, 2021 • edited Loading

kunerd commented Jan 28, 2021 • edited Loading

thomaseizinger commented Feb 2, 2021 • edited Loading

kunerd commented Feb 20, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Feb 20, 2021

Choose a reason for hiding this comment

thomaseizinger Feb 20, 2021

Choose a reason for hiding this comment

kunerd Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

kunerd commented Feb 22, 2021

kunerd commented Feb 25, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Mar 7, 2021

Choose a reason for hiding this comment

thomaseizinger Mar 7, 2021

Choose a reason for hiding this comment

thomaseizinger Mar 8, 2021

Choose a reason for hiding this comment

thomaseizinger Mar 8, 2021

Choose a reason for hiding this comment

DDtKey commented Apr 6, 2024 • edited Loading

kunerd commented Jan 25, 2021 •

edited

Loading

kunerd commented Jan 27, 2021 •

edited

Loading

thomaseizinger commented Jan 28, 2021 •

edited

Loading

kunerd commented Jan 28, 2021 •

edited

Loading

thomaseizinger commented Feb 2, 2021 •

edited

Loading

kunerd Feb 22, 2021 •

edited

Loading

DDtKey commented Apr 6, 2024 •

edited

Loading