Let port binding management to docker-compose.override.yml instead of variable usage

[soullivaneuh]

Summary

Port binding of the nginx server is currently defined with a env variable: https://github.com/getsentry/onpremise/blob/ae9378862caead70b620bb347bf5510833889478/docker-compose.yml#L199-L200

Instead, we should remove the port binding completely and let the user set it up on docker-compose.override.yml like this:

version: '3.4'

services:
  nginx:
    ports:
      - 80 # Do whatever you want here.

Motivation

Because of a lack of functionnality of docker-compose we are currently not able to remove the port binding.

As you suggested on your documentation I would like to setup a reverse proxy by adding a Traefik router service on the docker stack.

In that case, I do not want to expose the nginx port directly for security reason and because it will become totally useless.

I technically can not do that expect if I modify your docker-compose.yml directly, but this is not the proper way and I will always have a git diff to stash at each update.

Additional Context

Any other context or screenshots or API request payload/responses that pertain to the feature.

[soullivaneuh]

Here is my working docker-compose.override.yml with Traefik setup:

version: '3.4'

services:
  router:
    # Private image based on Traefik
    image: registry.gitlab.com/company/traffic:1.1.5
    restart: always
    volumes:
      - router:/data
      - /var/run/docker.sock:/var/run/docker.sock:ro
    labels:
      - traefik.http.routers.api.rule=Host(`router.sentry.company.net`)
    ports:
      - "80:80"
      - "443:443"

  nginx:
    labels:
      - traefik.enable=true
      - traefik.docker.network=company
      - traefik.http.routers.sentry.rule=Host(`sentry.company.net`)
      - traefik.http.routers.sentry.tls.certresolver=letsencrypt

networks:
  default:
    name: company

volumes:
  router: ~ 

With this config, I can access Sentry on https://sentry.company.net but also on http://sentry.company.net:9000 that I don't want to.

[BYK]

Hmm, this is actually a good solution. I think the best non-breaking way forward would be to handle this in the install script: detect if there is an already existing docker-compose.override.yml file and if not, create one with the current port binding.

If there is an already existing override, try to see if it has anything related to nginx and patch it if it doesn't. Otherwise issue a big warning saying you need a port binding.

What do you think? Also, would you be willing to submit PR for this?

[BYK]

Ping @soullivaneuh

[soullivaneuh]

detect if there is an already existing docker-compose.override.yml file and if not, create one with the current port binding.

I am not sure about this solution because it will be a problem for possible future BC breaking change.

Plus, some users may already have a docker-compose.override.yml but are using the current port binding.

However, as you strongly recommend to put sentry behind a reverse proxy, why let the default possibility to bind a port?

Also, would you be willing to submit PR for this?

I unfortunately can't guarantee to have the time to do it for now.

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Accepted, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Accepted, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[netsandbox]

I would suggest to just ship a docker-compose.override.yml.example file with the port binding, and instruct the users in the documentation to copy this file to docker-compose.override.yml if they want to use the port binding.

This is already used in project https://github.com/netbox-community/netbox-docker.

[chadwhitacre]

@netsandbox We'd still need to remove the env var, right? That's the breaking change that makes this complicated.

Ftr here is a pinned link to the docs that @soullivaneuh referenced in the OP:

https://github.com/getsentry/self-hosted/blob/ae9378862caead70b620bb347bf5510833889478/README.md#securing-sentry-with-ssltls

Not sure why that was removed from the README. Was it migrated to the dev docs or removed entirely for some reason? 🤔

[netsandbox]

@chadwhitacre Yes the env var SENTRY_BIND is then not needed anymore. As this is in fact a breaking change this should be targeted for the next major release.