Skip to content

Add ability to override configuration settings using environment variables #2147

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

Merged
merged 4 commits into from
Feb 20, 2020

Conversation

Wing924
Copy link
Contributor

@Wing924 Wing924 commented Feb 18, 2020

What this PR does:

You can use environment variable references in the config file to set values that need to be configurable during deployment.
To do this, use:

${VAR}

Where VAR is the name of the environment variable.

Each variable reference is replaced at startup by the value of the environment variable.
The replacement is case-sensitive and occurs before the YAML file is parsed.
References to undefined variables are replaced by empty strings unless you specify a default value or custom error text.

To specify a default value, use:

${VAR:default_value}

Where default_value is the value to use if the environment variable is undefined.

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Could you describe your use case please? Cortex configuration can be overridden via CLI flags and CLI flags are usually the expected way to override some config using values from environment variables.

@Wing924
Copy link
Contributor Author

Wing924 commented Feb 18, 2020

@pracucci
Now I use kustomize to manage k8s manifest. As I asked on the slack, it's better to put all configs on one config file (as a configmap) compared to set them by flags. The problem is that we can't change/override values on configmap per overlays.
Some values like db-url are different from each overlay. Some values like password are secrets, which don't want to put on plaintext.

As you say, put them on flags with environment variables is an option, but I don't like this because many components need to set the same flags:

# cassandra settings
- -cassandra.addresses=$(CASSANDRA_ADDRESSES)
- -cassandra.username=$(CASSANDRA_USERNAME)
- -cassandra.password=$(CASSANDRA_PASSWORD)

it looks very redundant to me.

@tomwilkie
Copy link
Contributor

I'm not averse to this myself; having multiple ways of overriding config settings (flags and environment variables) is a oft requested feature in the Prometheus world, and the "config management is someone else's problem" approach has gone down particularly badly. Plus this PR includes docs and tests, so I can't find anything to nit pick!

That being said I can see this being a little divisive, so I want to get a feeling from the other maintainers. But I'm +1 on this.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree you did a great job and I'm not against this change as well. I can see a value. However, I've the feeling it may be a possible breaking change (see the comment) reason why I'm wondering if we should enable this only via CLI flag.

CHANGELOG.md Outdated
@@ -15,6 +15,7 @@
* `--experimental.distributor.user-subring-size`
* [FEATURE] Added flag `-experimental.ruler.enable-api` to enable the ruler api which implements the Prometheus API `/api/v1/rules` and `/api/v1/alerts` endpoints under the configured `-http.prefix`. #1999
* [FEATURE] Added sharding support to compactor when using the experimental TSDB blocks storage. #2113
* [FEATURE] Add ability to override configuration settings using environment variables. #2147
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace configuration settings with YAML config file settings to be more clear.

@Wing924 Wing924 requested a review from pracucci February 20, 2020 05:43
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did a very good job! LGTM.

I'm going to ping other maintainers as well, to see if there's any strong opinion about not doing env vars expansion in the config.

Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++1

@pracucci pracucci merged commit 3510de1 into cortexproject:master Feb 20, 2020
@Wing924 Wing924 deleted the expand-env-vars-in-config branch February 20, 2020 11:43
timbyr added a commit to timbyr/loki that referenced this pull request Oct 29, 2020
Inspired by cortexproject/cortex#2147

Fixes grafana#2311

Adds -config.expand-env as flag to loki and promtail.
cyriltovena pushed a commit to grafana/loki that referenced this pull request Nov 5, 2020
* Support environment expansion in configuration

Inspired by cortexproject/cortex#2147

Fixes #2311

Adds -config.expand-env as flag to loki and promtail.

* Disable env expansion on logcli

* Small nit updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants