openspeedtest / Docker-Image

OpenSpeedTest Docker Image
124 stars 26 forks source link

Use of EXPOSE causes unnecessary ports being opened when changing ports #22

Open Nbelles opened 6 months ago

Nbelles commented 6 months ago

Hello,

I'm having some issues with using the default ports that are exposed. Once these ports are exposed, there is no way to unexpose them or change them. It is much preferable for a project to choose an arbitrary port (like the standard 80 and 443) for inside the container that users can then map to any port they want outside the container (using the standard docker run -p 80:1234 or docker compose). By using the EXPOSE command in the docker file, even if you map another port through the docker command line or in a docker compose, you still get extra exposed ports which might collide with other containers running. https://github.com/openspeedtest/Docker-Image/blob/fc86f9f4bd240fec5eb7670836e37252527afab7/Dockerfile#L73

openspeedtest commented 6 months ago

To assist in troubleshooting, could you please describe the steps required to replicate this issue?

Nbelles commented 6 months ago

Using the following docker-compose.yml, you can see in the following docker ps output that even though I only selected to expose port 10000, ports 3000 and 8080 are still exposed.

docker-compose.yml

version: "3"

services:
  openspeedtest:
    image: openspeedtest/latest
    ports:
      - "0.0.0.0:10000:3001"

docker ps

PORTS                                      
3000/tcp, 8080/tcp, 0.0.0.0:10000->3001/tcp
openspeedtest commented 6 months ago

What's your proposed solution? If this isn't a breaking change, please consider submitting a pull request (PR). Alternatively, could we specify a different unused port to avoid potential problems? I'm curious to understand the issue this behavior might create. I haven't personally encountered any problems, and you're the first to report it. This has me stumped!

Nbelles commented 6 months ago

Simply removing the line mentioned in the original post should do the trick*.

*Although it may be more intuitive to set the HTTP_PORT default to 8080 so CHANGE_CONTAINER_PORTS isn't required unless HTTPS_PORT is needed.

All the instances I have found in your documentation already have specific port mappings called out in both the docker run ... and docker-compose.yml methods so it shouldn't break any compatibility if people followed your documentation. If the proposed line is removed, only port 8080 will be exposed (I believe because of the base image you chose that exposes port 8080). If people want to remap this to something else, they can map port 10000 (or any random port outside the container) to 8080 (overwriting the default internal port) using something like docker run -p 10000:8080 ... and then setting CHANGE_CONTAINER_PORTS=True in the environment variables and setting either HTTP_PORT or HTTPS_PORT to 8080 (depending on their preference for http/https and this is why I recommend changing the default port from 3000 to 8080). If they want http and https, they can map 10000 to 8080 and 10001 to 8081 using something like docker run -p 10000:8080 -p 10001:8081 ... and then setting CHANGE_CONTAINER_PORTS=True in the environment variables and setting HTTP_PORT to 8080 and HTTPS_PORT to 8081.

Feel free to try this and verify this behavior is as described.