Feature request (and offer for pull request): Environment Variable Support

[schrodingersket]

Hello!

I'm looking at integrating Sentry on-prem for a few of our business customers who have a hard requirement for on-site deployments (we use the Sentry SaaS for the bulk of our customer base), and I'm hoping to integrate that with our existing infrastructure; for all of our services, we do that via environment variable configuration. It looks like something similar to that was supported in 9.1 (https://github.com/getsentry/docker-sentry/blob/master/9.1/sentry.conf.py#L5-L39), but I did see that you intended to move away from environment configuration toward volume-mounted config files (https://github.com/getsentry/onpremise/pull/407 and https://github.com/getsentry/onpremise/issues/314).

Is there any chance you'd be open to a pull request that re-introduces environment-driven configuration which defaults back to the current configuration? I'd be happy to put in the work for the code/doc.

Thanks in advance!

[schrodingersket]

After digging a little deeper into the Sentry source, it looks like there's already environment variable support for Docker containers in the main Sentry repo: https://github.com/getsentry/sentry/blob/master/docker/sentry.conf.py

I think a great way to add support for this while still respecting the move toward volume-mounted config files would be to create another env-config.yml (or some such... don't really have an opinion on the name, other than being something other than docker-compose.override.yml, as that would be automatically loaded) file which eliminates the sentry folder volume mount and configures via environment variables instead. This ensures that using environment variables is opt-in, and doesn't at all impact the current install.sh paradigm.

Happy to add support for this too (:

[BYK]

This was a decision to simplify maintenance as any new config we add there needs to be mirrored to environment variables and we already have the sentry.conf.py and config.yaml files. I don't see the value of adding yet another mechanism with a continuous maintenance burden. Why do you need environment variables, or why does the current solution not work for you? :)

[schrodingersket]

For sure! I definitely understand wanting to avoid extra maintenance.

We use Docker Compose to run our on-site deployments as well but are highly discouraged from requiring the use of installer scripts (as such, sed-ing the daylights out of a conf file is discouraged), and are instead encouraged to add Docker Compose files driven by a single .env file. A big part of the motivation behind this this is to ensure our services and infrastructure are ready to move to something like Kubernetes or Docker Swarm down the road. It also helps ensure that our dataloaders are idempotent, facilitates cross-platform ease of use, etc.

That all said, I'm really only looking to expose the infrastructure connection configuration (host/port/db name/username/password/ for Postgres, Redis, Memcached, Kafka, Zookeeper, Clickhouse, and optionally RabbitMQ if I recall correctly), which is already supported in your official Docker image... it seems like that configuration is fairly unlikely to change or require much if anything in the way of maintenance since it pertains only to infrastructure configuration (and I suspect that if your infrastructure requirements change, there's going to be a LOT of code churn anyway).

Adding support for this also reduces the amount of work that the install.sh script has to do; since your bootstrap/migration commands are idempotent, I ran a proof-of-concept (commit 5197718) that added those as services which restart on failure with the deployment starts up. This in turn effectively reduced the installation steps for a fresh install to docker-compose build && docker-compose up -d. Hoping that introducing support for this might provide the opportunity to add some simplicity all around (:

[BYK]

@schrodingersket thanks a lot for the detailed response and apologies for the long turn around time!

We use Docker Compose to run our on-site deployments as well but are highly discouraged from requiring the use of installer scripts (as such, sed-ing the daylights out of a conf file is discouraged), and are instead encouraged to add Docker Compose files driven by a single .env file. A big part of the motivation behind this this is to ensure our services and infrastructure are ready to move to something like Kubernetes or Docker Swarm down the road. It also helps ensure that our dataloaders are idempotent, facilitates cross-platform ease of use, etc.

This is also what we strive in this repo, hence we switching to volume mounts for configs. The install script is mostly for beginners and to avoid documentation around permanent volumes going out of date. We also handle some migrations there as you may see. These need to be somewhere 😀

That all said, I'm really only looking to expose the infrastructure connection configuration (host/port/db name/username/password/ for Postgres, Redis, Memcached, Kafka, Zookeeper, Clickhouse, and optionally RabbitMQ if I recall correctly), which is already supported in your official Docker image... it seems like that configuration is fairly unlikely to change or require much if anything in the way of maintenance since it pertains only to infrastructure configuration (and I suspect that if your infrastructure requirements change, there's going to be a LOT of code churn anyway).

Adding support for this also reduces the amount of work that the install.sh script has to do; since your bootstrap/migration commands are idempotent, I ran a proof-of-concept (commit 5197718) that added those as services which restart on failure with the deployment starts up. This in turn effectively reduced the installation steps for a fresh install to docker-compose build && docker-compose up -d. Hoping that introducing support for this might provide the opportunity to add some simplicity all around (:

I am open to this but I honestly fail to see how these would help the install script to be simpler. It doesn't set anything besides the secret key if it is not set and if you are still going to use the config files, then what makes it hard to use the env variables from inside those config files? Can you clarify the problem you are trying to solve a bit more if you don't mind?

[schrodingersket]

(TL;DR at bottom)

Thanks for getting back to me! No worries on turnaround time - I'm sure you're crazy busy, so I deeply appreciate you taking time to respond (:

It doesn't set anything besides the secret key if it is not set and if you are still going to use the config files, then what makes it hard to use the env variables from inside those config files?

Using the environment variables from inside the existing config files is actually exactly what I'm proposing - not sure if you happened to take a look at the commit (5197718) I posted, but it simply changes the current config volume mounts from /etc/sentry to /etc/sentry/onpremise and sets the SENTRY_CONF environment variable accordingly. Then, setting SENTRY_CONF to /etc/sentry utilizes the conf files that are baked into the official Sentry Docker image which already use environment-driven configuration, where leaving everything at their defaults still mounts and uses the example conf files provided in this repository.

I am open to this but I honestly fail to see how these would help the install script to be simpler.

Honestly, it only removes the migration commands from the install script, so I suppose it's only trivially simpler. :sweat_smile:

Can you clarify the problem you are trying to solve a bit more if you don't mind?

My specific use-case is that I'm looking to include this repository as a submodule of a larger orchestration project, which we use both for dev and production (just with different .yml files for each). I'd like to be able to directly include the Docker Compose files from this repo into that larger deployment, and since the current paradigm overrides the default environment-driven conf files with local static files, that's not really feasible.

TL;DR: I'm not looking to override the conf files at all; rather, I'm looking to provide a way to not override the ones built into the official Sentry image. The current install script behaviour is to overwrite these files.

[BYK]

@schrodingersket okay, this clarifies your need pretty well and this is something we'd love to support and although I looked at your commit earlier, got distracted by the large file changes around the env files. I think having only the mount point change works well for us. I wish there was a way to remove volume mounts with compose file overrides :(

[BYK]

Btw, be warned that the config file baked into the image may not always be the best for on-premise but we'll try to keep it so.

[BYK]

Finally, I was planning to try using this: https://docs.docker.com/compose/compose-file/#configs

Maybe it makes your life easier for the PR?

[schrodingersket]

AH! Thanks for pointing that Docker Compose feature out! I've read over the reference a few times, but not since 3.1 I think. I'd love to extract out the configuration I added to .env into it's own file, as that's much cleaner and makes it much more obvious where the service connection configuration lives. Would you accept a PR with that config added to its own file and a dedicated override .yml file, or do you only want to stick with strictly just the mount point change?

I too wish volume mounts could be removed with overrides... that'd certainly make my life easier in some other places hehe.

EDIT: After looking at the Docker Compose configs documentation, I think I misunderstood what those do - was thinking it was a way to explicitly include a named .env sort of file.

Another option (which I think would be cleaner) would be to leave out the changes to the .env file, and instead include those values as defaults in docker-compose.env.yml. That still allows everything to run out-of-the-box without cluttering up the file-mounted configuration (assuming that you're amenable to including docker-compose.env.yml instead of only the volume mount change).

[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 🥀

[BYK]

Closing this issue as we have decided not to move forward with this approach for now.