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

improve configure publish ports #407

Closed
wants to merge 1 commit into from

Conversation

centum
Copy link

@centum centum commented Jan 30, 2021

Related Issue:

New Behavior

Docker published port defined in docker-compose.override.yml only.

Contrast to Current Behavior

Defined new published ports in config docker-compose.override.yml don't overlap definition it in docker-compose.yml. The docker-compose concatenates them in the one config.

For the multi-value options ports, expose, external_links, dns, dns_search, and tmpfs, Compose concatenates both sets of values

https://docs.docker.com/compose/extends/#adding-and-overriding-configuration

Discussion: Benefits and Drawbacks

Denied publish HTTP port on an unexpected port.

Proposed Release Note Entry

Default disabled publish HTTP port.

Double Check

  • I have read the comments and followed the PR template.
  • I have explained my PR according to the information in the comments.
  • My PR targets the develop branch.

@cimnine cimnine added the discussion This issue requires further input from the community. label Jan 31, 2021
@cimnine
Copy link
Collaborator

cimnine commented Feb 8, 2021

Thank you for this suggestion. I think it's time that this change can be implemented. In the beginning, we didn't advertise too much for the docker-compose.override.yml. Instead, we told users to use docker ports netbox and find the correct port or edit the docker-compose.yml file.

Since a while this has changed and it's afaik the default for most user to create the docker-compose.override.yml. At least judging from the questions and bugs I've seen this seems to be the case. (I.e. only very few struggle to connect to Netbox anymore.)

@tobiasge what do you think? I'd target this for 1.1.0, as I wouldn't tax it as breaking change. Do you agree or disagree?

@mpibpc-mroose
Copy link

I've had this problem, too and thought what a good solution would be. Problem with this fix is, that your distributed docker-compose.yml does not provide a ready to run configuration as the web GUI would not be accessible. From my point of view a good compromise would be to define

  ports:
    - "127.0.0.1:8000:8000"

Using this a local access to the GUI is possible plus the GUI is not exposed to "the internet" (which is the crucial part with the current docker-compose.yml from my point of view). Then a NGINX container or whatever could get attached to that local listening port.

@cimnine
Copy link
Collaborator

cimnine commented Feb 8, 2021

ports:
  - "127.0.0.1:8000:8000"

Sorry to say this, but this suggestion would be even worse than the current solution. The reason is, as explained by @centum, that it's just not possible to override ports from the docker-compose.override.yml file which are defined in the main docker-compose.yml. So when a user does not want NetBox Docker on port 8000, e.g. because a service is already using that port, that user has no other option than to edit docker-compose.yml. And I'd like to avoid that.

# docker-compose.yml
...
netbox:
  ports:
    - "127.0.0.1:8000:8000"

# docker-compose.override.yml
...
netbox:
  ports:
    - "8080:8080

# the result will be
...
netbox:
  ports:
    - "127.0.0.1:8000:8000" # it's not possible to get rid of this
    - "8080:8080"

@tobiasge
Copy link
Member

tobiasge commented Feb 8, 2021

I don't like the idea of removing the ports completely, because it will make a first start harder for a user. At the moment a user can start the system without any additional files needed.
A good compromise would be to get docker-compose to select a random port but make it only accessible from localhost. But i don't know if this is possible at the moment.

@mpibpc-mroose
Copy link

I see the point: it should be easy to get to a production ready configuration usering an override (as it was before with the included NGINX). On the other hand we should avoid exposing the users to a security risk as we would do when exposing the Netbox GUI directly to the internet as the current docker-sompose does. Plus this currently could not "swicted off" with the compose.

I think "in production" nobody will use the netbox container as endpoint for GUI requests. One always would take some webserver to distribute the netbox endpoint "to the world", especially as TLS is required nowadays. So from my point of view the purpose of the docker-compose.yml is to give people an easy going possibility to get Netbox running for testing purposes ("to try out the product"). We should avoid that people expect this docker-compose as production ready. And the easiest solution is to terminate the GUI on localhost by default.

One could then provide a docker-compose_production.yml example... But maybe this should be in the responsibility of whoever want's to run Netbox in production. The end-user needs to integrate that into his/her IT...

@centum
Copy link
Author

centum commented Feb 8, 2021

I don't like the idea of removing the ports completely, because it will make a first start harder for a user. At the moment a user can start the system without any additional files needed.
A good compromise would be to get docker-compose to select a random port but make it only accessible from localhost. But i don't know if this is possible at the moment.

The current instruction says to create a docker-compose.override.yml specifying the published port.
If the user, following the instructions, adds his own port mapping at 8000, then he may not even guess about port 8080, which is registered in the docker-compose.yml. In addition, this port binds to the 0.0.0.0:8080 interface, to which you need to block it with a firewall. The user may or may not do this. This increases the risk of being hacked.

@cimnine
Copy link
Collaborator

cimnine commented Feb 8, 2021

One always would take some webserver to distribute the netbox endpoint "to the world", especially as TLS is required nowadays.

Even despite our recommendation to do so, this assumption does – as per my experience talking to some users – not hold, unfortunately.

So from my point of view the purpose of the docker-compose.yml is to give people an easy going possibility to get Netbox running for testing purposes ("to try out the product").

This is mostly what I have in mind when working on Netbox Docker. There is another important purpose, which initially triggered this project: Development for and on Netbox. And this may require to have multiple instances of Netbox Docker running in parallel, something that was – out of the box – easily achieved with the current ports: 8080 solution. Also, it would still work with the initially suggested ports: [] solution.

And the easiest solution is to terminate the GUI on localhost by default.

IMO this is achieved by the Use docker-compose.override.yml-instructions anyway. We clearly document this in all the Getting Started sections.

@cimnine
Copy link
Collaborator

cimnine commented Feb 27, 2021

I don't like the idea of removing the ports completely, because it will make a first start harder for a user. At the moment a user can start the system without any additional files needed.

@tobiasge IMO this only holds true for very experience Docker users, as one has to figure out the port on which Netbox Docker binds (through docker-compose ps or docker-compose ports netbox 8080) without a docker-compose.override.yml. This is not 'simple' either for novice users. Therefore our docs all recommend the docker-compose.override.yml.

I also see it as a benefit that folks are advised to create a docker-compose.override.yml file, because we reference it for a lot of other things as well. So they learn early on what we think is a good way to customize the installation.

@tobiasge
Copy link
Member

tobiasge commented Mar 1, 2021

How about adding a docker-compose.override.yml.example file? I think that would be a good compromise.

@tobiasge tobiasge mentioned this pull request Apr 17, 2021
3 tasks
@tobiasge tobiasge closed this Apr 19, 2021
@cimnine cimnine mentioned this pull request Apr 23, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue requires further input from the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants