testcontainers / testcontainers-java

Testcontainers is a Java library that supports JUnit tests, providing lightweight, throwaway instances of common databases, Selenium web browsers, or anything else that can run in a Docker container.
https://testcontainers.org
MIT License
7.99k stars 1.64k forks source link

Automatically map exposed ports defined in Dockerfile #1448

Open aguibert opened 5 years ago

aguibert commented 5 years ago

Typically a Docker image will declare what ports it exposes using: EXPOSE <ports...>

Why is it also necessary to duplicate this information by default for GenericContainer?

Take the quick start example:

@Rule
public GenericContainer redis = new GenericContainer<>("redis:5.0.3-alpine")
                                        .withExposedPorts(6379);

If the user does not explicitly state what ports to expose via withExposedPorts(), could we run DockerClientFactory.instance().client().inspectImageCmd(this.getDockerImageName()).exec(); to run a Docker image inspection and automatically read/set the ports exposed by the Docker image? I know there are situations where a user may not want to expose all (or any) ports of a container, but I think this would be the minority case for Testcontainers users, right?

If we were to do this, the above code example could simply be:

@Rule
public GenericContainer redis = new GenericContainer<>("redis:5.0.3-alpine");
kiview commented 5 years ago

Hey @aguibert, I think this is a good point and we should adapt the getters (or add some) to also make use of this opinionated behavior.


GenericContainer redis = new GenericContainer<>("redis:5.0.3-alpine");
redis.start();

redis.getDefaultMappedPorts();
redis.getFirstDefaultMappedPort();

Not sure if it makes sense to add those new methods or just adapt the current behavior, I think adding would be a counter productive increase of the API surface and make usage more confusing compared to just being opinionated.

I know there are situations where a user may not want to expose all (or any) ports of a container, but I think this would be the minority case for Testcontainers users, right?

I agree, I can't even come up with an example here.

bsideup commented 5 years ago

@kiview I would not do that: 1) inspect command adds to the container's startup time 2) we use exposed ports to do the port waitings. If we make them implicit, the users will be surprised why their images fail (because there are some debug ports exposed or something, and some images expose all potential ports, but listen on a few)

redis.getDefaultMappedPorts(); redis.getFirstDefaultMappedPort();

What's the practical value of it?

aguibert commented 5 years ago

@bsideup, for point (1), the inspect command does take about 9 or 10ms on my macbook inspecting a local docker image. I think that time is mostly negligible when you consider container starts can take on the order of seconds.

For point (2), that is a valid point. The images I was working with exposed ports 9080 (http) and 9443 (https), but my app only listened on 9080. So when I tried to implement the "automatically listen on exposed ports" on my machine, I ran into this issue because my container timed out waiting for 9443.

To work around this, I think we could do 2 things to help port assignment be opinionated:

  1. If we inspect an image and it only exposes 1 port, then automatically expose it. Remember that today if we try to start a container that exposes 0 ports, we just blow up right away.
  2. If an image exposes multiple ports, we could automatically expose the first port. For the images I've seen, this heuristic would work >80% of the time.
bsideup commented 5 years ago

Remember that today if we try to start a container that exposes 0 ports, we just blow up right away.

Hmm, it shouldn't be like that. AFAIR we have a check, and if there are no ports exposed, we do not wait on ports. Otherwise that's a bug and should be fixed

take about 9 or 10ms on my macbook

There are envs where running even a trivial Docker command takes 70-80ms, we have to be careful with the latency assumptions.


TBH I like the explicit approach. It is just one line for GenericContainer and no lines for specific ones (because they expose the ports already), but it keeps things clear and less magical (especially given that it is not easy to figure which ports are defined as EXPOSED in a Dockerfile, and some images are not even automatically built from a Dockerfile and one has to look up the source of the image)

aguibert commented 5 years ago

Hmm, it shouldn't be like that. AFAIR we have a check, and if there are no ports exposed, we do not wait on ports. Otherwise that's a bug and should be fixed

Oh, it may be if we choose a port/http wait strategy, so that makes sense.

TBH I like the explicit approach.

I will admit that the current behavior is consistent with how the docker CLI works (i.e. you still have to do -p 1234:1234 to expose ports), but I've always thought it would be nice if Docker containers exposed all ports that were marked EXPOSE by default. Maybe that would have been a security risk, but for Testcontainers I don't think we have that same concern.

but it keeps things clear and less magical

Not needing to know or care about parts is one of my favorite things about Testcontainers. Compared to the rest of development experience, having to always do withExposedPorts() on my GenericContianer feels like a very low-level detail.

especially given that it is not easy to figure which ports are defined as EXPOSED in a Dockerfile

I agree, and this is precisely why I think it would be nice if we could automatically figure out EXPOSED ports for the user =)