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

Add udp and sctp support to port mappings #246

Closed
wants to merge 6 commits into from

Conversation

kunerd
Copy link

@kunerd kunerd commented Jan 25, 2021

Resolves #234

@kunerd
Copy link
Author

kunerd commented Jan 25, 2021

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
Copy link
Collaborator

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
Copy link
Author

kunerd commented Jan 27, 2021

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
Copy link
Collaborator

thomaseizinger commented Jan 28, 2021

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
Copy link
Author

kunerd commented Jan 28, 2021

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
Copy link
Collaborator

thomaseizinger commented Feb 2, 2021

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
Copy link
Author

kunerd commented Feb 20, 2021

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.

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Users need to be able to specify the protocol when resolving the port. Currently the trait bound is only Into<u16>.
  2. 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.

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@kunerd kunerd Feb 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

kunerd commented Feb 22, 2021

Now, only additional documentation and some test are missing.

@kunerd
Copy link
Author

kunerd commented Feb 25, 2021

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.

Copy link
Collaborator

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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

.unwrap_or_else(|| {
panic!(
"container {} does not expose port {}",
"container {:?} does not expose port {:?}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

.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 ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

DDtKey commented Apr 6, 2024

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

@DDtKey DDtKey closed this Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow port forwarding for udp ports
3 participants