Add udp and sctp support to port mappings

[kunerd]

Resolves #234

[kunerd]

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]

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]

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]

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]

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]

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]

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.

[kunerd]

Now, only additional documentation and some test are missing.

[kunerd]

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.

[DDtKey]

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