-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Support for environment variable substitution in configuration file #2357
Comments
We've previously discussed several times and decided against. For simplicity we keep to one way to configure a given thing, otherwise we'd end up with 4-5 ways. |
This could be a particularly powerful way to configure Prometheus in a cluster without the burdensome task of a separate config file for every configuration you can ever deploy. |
It's a nice to have. |
Environment variable substitution would allow to keep sensitive data like passwords used for remote read/write out of the config file, too. InfluxData recently mentioned environment variable substitution in config files would be a useful feature in their eyes. |
I don't think it would cause usability issues guys, its pretty common to use environment variable substitution in config files like these. Any chance to revise that decision? Here are some benefits:
Am I missing something here? |
Any update on this? Related to #2664 Eventually we just ended up having separate configuration files per cluster, which was annoying since it was an exception among other services deployed within our clusters. PS: another option is to provide small wrapper script doing this cluster specific substitions (sed -i prometheus.yml like) before actually starting prometheus. You will need a proper signal handling mechanism like dump-init however (https://github.com/Yelp/dumb-init), since you're prometheus will not be running as PID 1 anymore in this case.
... and within your entrypoint script:
The disadvantage of this approach is you're introducing an external component dumb-init, breaking single process per container best practices, which some might not be willing to do. |
We have similar issues when we run Prometheus in 2-replica statefulset which reuses single configmap. To support HA we have
This is not fun and this solution screams for better approach. But I feel like this might be rather Kubernetes issue not enabling that, not necessarily Prometheus one. |
Maybe moving |
A small tip: When you are using the shell like this where application is the last thing you are running do I'm unsure of the wisdom of using a hostname as an external label, that's going to make any remote storage endpoint have labels vary as Prometheus moves around over time. Two separate statefulsets with different label values like mydc1 and mydc2 might be better. |
Cool, thanks for the advice! Our host name is actually pod name so I think this is fine (: Won't change easily. |
Yea, the name for stateful sets is deterministic in contrast to deployments, where pods get a random suffix. So it will always be |
I need to configure azure_sd_configs with sensitive info like client_secret and etc. |
Environment variables are a bad idea for secrets. Ansible has its own secret and templating support, which is the standard way to approach this. |
FWIW, the prometheus-config-reloader for prometheus-operator looks to have implemented a solution for this for the statefulset use case recently: prometheus-operator/prometheus-operator#1132 prometheus-operator/prometheus-operator@608e714#diff-a2dba0afa9edde401541fc9054415bba |
If you use Thanos you have that out-of-the-box as well: https://github.com/improbable-eng/thanos/blob/master/cmd/thanos/sidecar.go#L73 |
One reason to have this is so that you can have better devops for SWARMS If you are doing lots of builds, you want to be able to template your SWARMS and this does NOT make that possible. I build 4-50 node swarms constantly based on customer demands and being able to dynamically configure them would be IDEAL and save me hours. |
B"H We solved this by creating an entrypoint script that preprocesses environment variables before calling the binary. Example showing how to set
Here are the Prometheus and Alertmanager images, with sources linked there. Multiple tags are available depending on the version you need. These are super bare-bones wrappers around the official images, simply working around this particular issue and then getting out of the way. The Prometheus authors made a great product and are making sound arguments for not including this functionality natively. The above images, however, might help people whose infrastructures or policies allow for taking a more relaxed approach. |
What about using Docker secrets? My understanding is the expected way of controlling secret values in containers is to add a Docker secret, which generates a file inside your container, then within the container read that file into environment variables at runtime. |
@djbingham That is still putting secrets in environment variables. |
@brian-brazil Yes it is, but I'm led to believe (from the official documentation for Docker) that this is the expected way to handle such things. If that is actually a bad idea, please could you elaborate on why? |
See http://movingfast.io/articles/environment-variables-considered-harmful/ and https://diogomonica.com/2017/03/27/why-you-shouldnt-use-env-variables-for-secret-data/ for the reasoning. Environment variables were never designed with secrets in mind, nor are they traditionally a place to put secrets. |
Good explanations, thanks. |
Secrets are being used as an excuse to avoid adding some pretty basic and expected functionality, methinks. http://docs.grafana.org/installation/configuration/#using-environment-variables Justify what's wrong with that, as not just Grafana but NPM and others have the exact same regime of overrides. |
So we have to put environment-specific proxy settings into a configuration file that can otherwise be kept environment-independent? That doesn't seem like good solution. |
@brian-brazil but storing them statically in config-files in your git-repo is better? 👎 👎 👎
@brian-brazil but running "config-pre-render-scripts" to set imho sometimes you have no clue what you are talking about, sorry. |
Env variables make so much sense here... Can't check in my AWS creds for an ec2_sd scrape job, and mounting them in as secrets to ENV vars and then leaving in the scrape config $ACCESS_KEY for example would be perfect. |
Also, in case feature parity matters to anyone - TICK stack components are adding this functionality in recent commits - its not in a release yet, but it will be soon. influxdata/telegraf#5648 |
@brian-brazil I don't quite understand why your personal objection should make life difficult for everyone else. Environment variables are not used only for secrets. Refusing an industry standard method of configuration seems quite unnatural in this industry. Even closing this thread after 2 years won't make people stop asking for it. |
…hey do not want to support it (prometheus/prometheus#2357) and I do not want to put secrets into the checked in config file Changed line endings from CRLF to LF
I'd like to leave a note here that might help others mired by the ignorance of some foolish crusade against environment variables. Our systems utilize stack and we were able to get some of our configuration of this project accomplished by adding an "entrypoint" subsection to the prometheus section of the configuration. Everything else we could do with a "configs" subsection using version 3.6 of compose that inserts the prometheus.yml file into place. In discussions with my colleagues I found that we are unanimous in our belief that the stubbornness exhibited here doesn't bode well for the future of the Prometheus project. When someone offers up their work for others to save them time it is a truly wonderful and generous thing. In doing so you want your efforts to have mass appeal, be easy to use and versatile enough to wedge in just about anywhere. Good folks of Prometheus please reconsider your desire to dictate the infrastructure of those who might make use of your project. You will save yourselves countless future threads full of angry developers who after hours of attempting to force a square peg in a round hole, unfortunately, decide to vent their frustrations upon you. This thread is a messed up situation that never had to happen, and I'm sure everyone involved would much rather be hailing you as heroes than reviling you as villains. |
I'm trying to implement Prometheus on several kubernetes clusters, and I just want to change remote write URL in each of them. Because Prometheus developers thinks that env variables are dangerous or only for sensitive data (all @brian-brazil replies are only about dangerous of putting sensitive data on them), I have to rethink about my implementation. Please rethink about this feature. I read all the comment of this topic, and a lot of people ask for this feature. I think it will be good for Prometheus to have it. The best part of this topic:
It's still in discuss for a lot of people. |
I don't see a point in even responding/using prometheus if @brian-brazil is just going to let this issue rot and refuse to discuss anything with users. |
Seems that there are plenty of users with this requirement. Why not just put it up for a poll? A good product is built by requirements, not individual opinions. |
I appreciate the staunch requirement of security, actually. I think a poll could end disatrously if the wrong things were polled for. However in this case, there is little to no evidence environment variables in sandboxed environments pose much threat, if any. It's just a case of the BDFL not caring about what people say. |
There was a solution posted in this issue already: #2357 (comment) |
@kron4eg - Thanks, but that is only for K8s. Not sure it will work for ECS. |
|
Tbh the only solution to obstinate stubborness like this is a fork. |
Wow. This thread is, baffling. |
I'll leave a vote in favor of allowing configuration from environment variables. It's a standard approach in cloud deployments, is well-supported by existing deployment tooling, and like others I am not very convinced by the arguments against using the environment to pass secrets. The ability to read secrets from mounted files would make it possible for people who are convinced by those arguments to avoid that vector. As it stands now the various proposed work-arounds add complexity and are worse than the alternatives imo. I'd also like to put in a strong vote for civility toward the project maintainers. Prometheus is an amazing tool that we get to use for free, thanks entirely to the volunteer efforts of the maintainers. Ad hominem attacks on them are childish and unproductive imo. |
+1 for env vars |
I'm working on a federated deployment, and the inability to easily fill values on things like |
@kinghuang Having read the response to your issue, the idea that configuring Prometheus is something that needs to be handled outside Prometheus is something I find utterly baffling. On a personally relevant note, I've actually been migrating my organization away from Prometheus and over to Telegraf. The fact the environment variable substitution is supported out of the box is something I consider critical to a distributed monitoring solution. It's a shame that the current CNCF graduated monitoring solution considers something like environment variable substitution for configs outside the scope of the tool itself, when so many other tools both within and without CNCF consider such a thing a standard feature. Hopefully at some point this attitude will change. |
How has Grafana managed to provide support for environment config without becoming a full blown configuration management service? I'll add my use case to the pile! I'm using Docker compose + Amazon ECS to set up a Prometheus / Grafana / Alertsmanger. For the Grafana portion of our setup we are able to deploy semi-configured images stored on dockerhub (the images and config files are fully are public), and configure secrets using environment variables passed into the container. I'd like to add alertmanager to our stack but without support for env configuration I'm left unsure how I might configure alertmanager without baking the secret into the image (which is a bad practice and would mean I have to either use Amazon's proprietary container registry or paying for private dockerhub). I imagine I'll face the same problem when I get around to configuring the prometheus image to be more secure. It sounds like I'm going to have to write a script to generate a config file within the container based on passed-in environment variables. I'm left unsure why the Prometheus team demands this of me. Update: Good to know that Telegraf exists @rbkaspr, I will definitely check that out for future projects. |
For ayone following this issue: https://groups.google.com/forum/#!topic/prometheus-developers/tSCa4ukhtUw Please stay polite and respectful in the discussions. |
Thanks @roidelapluie for beating me to it. It was my intention to backlink from here. For context: The linked mail thread is a formal voting thread to get an explicit statement from the 22 members of the Prometheus team. (See the governance document to see who is a team member and what that means.) |
Doesn't matter, they're all voting no without giving any information. Time to fork. |
I get the frustration surrounding this issue, but let's wait until we have a majority of nay votes before we start talking forks. So far we only have 2 nays and one who seems to be leaning hard into a nay vote. Even folks who have previously closed issues relating to this feature request are at least having a reasoned discussion about implications, and not just slapping it down. That said, I do feel that some of the arguments against environment variable expansions are head-scratching, to say the least. The main argument I really take issue with is the idea that somehow adding environment variable expansion as a config option for Prometheus will somehow turn it into a CM tool. As someone who's day-to-day job involves working with actual CM tools, this feature is not magically going to turn Prometheus into Salt, Chef, Ansible, or Puppet. If someone were to request Prometheus suddenly start pushing configurations out to other tools and systems, I'm confident that no one in the user community would bat an eye at such a request being rejected. When you have developers closing issues about how to configure Prometheus as out of scope for the Prometheus team, that's where people start having issues. Especially when (as has been mentioned before), so many other projects both in and out of CNCF have managed to solve this issue, and Kubernetes makes it so simple to take advantage of. With that out of the way, I appreciate @beorn7 and @roidelapluie linking the developer discussion here, it's good to see that the team is actually having a serious discussion about this request internally rather than just silently rejecting it without explanation. Hopefully we'll land on a positive outcome once January 6th rolls around. |
The vote is complete, with results tallied at https://groups.google.com/forum/#!msg/prometheus-developers/tSCa4ukhtUw/J-j0bSEYCQAJ You can read the whole thread for full details and discussion, but the brief summary is that the vote was 11:2 in favour of keeping the status quo of not supporting environment variable substitution. Our governance means that this outcome is final, barring a future vote overturning this. I'll now be locking this issue. You are always free to bring things up on the -users/-developers list. |
A second vote happened this week and the special use-case of external labels was considered for env variable substitution. Starting Prometheus 2.27, we will be able to substitute env variables in external labels if you use This can be tested with the |
I think that would be a good idea to substitute environment variables in the configuration file.
That could be done really easily using os.ExpandEnv on configuration string when loading configuration string.
That would be much easier to substitute environment variables only on configuration values. go -ini provides a valueMapper but yaml.v2 doesn't have such mechanism.
The text was updated successfully, but these errors were encountered: