Victoriametrics: Add support to environment variables in addition to flags

[hekmon]

Is your feature request related to a problem? Please describe.
Currently the only way to configure VM is thru launch flags, while this is not itself an issue it could be a bit problematic for some options, let's say -snapshotAuthKey, as it leaks in the processes table the auth key.

Describe the solution you'd like
Supporting environment files would allow to pass the auth key in a more "stealthy" manner. For example, by supplying an env file to a systemd unit file, the authKey could be passed on vmstorage without the need to be a flag.

Additional context
Supporting env file could bring more than just protect the authKey: it is the most standard way to configure docker containers for example and would simplify a lot starting VM with systemd. For example, we actually start our storage nodes this way:

[Unit]
Description=Victoria Metrics - cluster version: storage module
After=network-ready.target

[Service]
Type=simple
User=vmc-storage
EnvironmentFile=/etc/default/vmc-storage
ExecStart=/usr/bin/vmc-storage -bigMergeConcurrency "$BigMergeConcurrency" -dedup.minScrapeInterval "$MinScrapeInterval" -httpListenAddr "$HTTPListenAddr" -loggerLevel "$LoggerLevel" -memory.allowedPercent "$MemoryAllowedPercent" -precisionBits "$PrecisionBits" -retentionPeriod "$RetentionPeriod" -search.maxTagKeys "$SearchMaxTagKeys" -search.maxTagValues "$SearchMaxTagValues" -search.maxUniqueTimeseries "$SearchMaxUniqueTimeseries" -smallMergeConcurrency "$SmallMergeConcurrency" -snapshotAuthKey "$SnapshotAuthKey" -storageDataPath "$StorageDataPath" -vminsertAddr "$VMInsertAddr" -vmselectAddr "$VMSelectAddr" $Others
WorkingDirectory=/var/lib/vmc-storage
Restart=on-failure

[Install]
WantedBy=multi-user.target

With /etc/default/vmc-storage declaring all of theses variables (and thus acting as a config file). With env var support, the ExecStart would be much cleaner/nicer (just the binary with no flags) and adding an option/value to the env file would not require to edit the unit file (or use an ugly hack as the $Others var we use) as all vars declared in this file would automatically be passed to the binary.

Just as reference, some Golang cli libs support for a given option both launch flags and env vars:
https://github.com/urfave/cli/blob/master/docs/v2/manual.md#values-from-the-environment
This allow the customize the default value of flags from env but still allow overwrite thru flag.

I think that kind of approach would be ideal :)

[valyala]

Thanks for the detailed proposal, @hekmon !

While env vars have a disadvantage comparing to explicitly set command-line flags, since evn vars are implicitly applied to the app, I think their usability outweigh this disadvantage. So I'm going to add environment variables support into VictoriaMetrics in the near future according to your proposal.

The proposal is somewhat related to https://github.com/VictoriaMetrics/VictoriaMetrics/issues/138 .

[valyala]

@hekmon , support for setting flags via environment vars has been added in the following commits:

• Single-node version - 8466ab003423cc3a3cc7755eb9561bbaa4a49b87
• Cluster version - 1010a57882cd8ec97fadfcd3bd831f7f4679eab9

These commits will be included in the next release.

In the mean time you can build VictoriaMetrics from these commits and verify whether it works as expected. See instructions on how to build VictoriaMetrics:

• how to build single-node version from sources
• how to build cluster version from sources

[valyala]

Added -envflag.enable command-line flag, which must be explicitly set in order to enable reading flags from environment vars. This should protect from reading unexpected flag values from env vars if the -envflag.enable flag isn't set.

[hekmon]

It seems to work just fine !

✔ 16:32 ~/tmp/VictoriaMetrics [ :5d207b2 | ✔  ] $ loggerFormat="cthulhu" bin/victoria-metrics -envflag.enable
panic: FATAL: unsupported `-loggerFormat` value: "cthulhu"; supported values are: default, json

goroutine 1 [running]:
github.com/VictoriaMetrics/VictoriaMetrics/lib/logger.validateLoggerFormat(...)
        /home/hekmon/tmp/VictoriaMetrics/lib/logger/logger.go:66
github.com/VictoriaMetrics/VictoriaMetrics/lib/logger.Init()
        /home/hekmon/tmp/VictoriaMetrics/lib/logger/logger.go:34 +0x284
main.main()
        /home/hekmon/tmp/VictoriaMetrics/app/victoria-metrics/main.go:30 +0x41
✘ 16:32 ~/tmp/VictoriaMetrics [ :5d207b2 | ✔  ] $ loggerFormat="cthulhu" bin/victoria-metrics
[start logs]
^C
[stop logs]
✔ 16:33 ~/tmp/VictoriaMetrics [ :5d207b2 | ✔  ] $

The opt in env flag is a good compromise :ok_hand:

Just a thought: usualy env vars are prefixed to avoid potential collisions down the road (for example vm_, like grafana does). While it is safer, it is less straightforward and might need a specific documentation section, ur call :)

[valyala]

Let's leave env vars without vm_ prefix. IMHO, this is more clear and more user-friendly than applying various transformations to env var names :) If VictoriaMetrics-specific env var clashes with third-party env vars, then the conflict may be resolved by setting the corresponding flag via command line. Additionally, by default reading flag values from env vars is disabled, so clashing env vars shouldn't bother users, who don't use env vars.

[valyala]

Flags can be read from env vars starting from v1.33.1. Closing this feature request as implemented.

[hekmon]

Currently playing with env vars (and updating my .deb and puppet classes) and the following limitations came to my mind:

• repeatable flags like -storageNode won't play well with env vars. Should you disable env vars for theses as multiples declaration will only lead to having only the last one set ?
• flags with a dot like -memory.allowedPercent would translate to an invalid variable name. While it is still possible to hack around to set a var with such a name, I was not sure how systemd would handle it.

✔ 16:43 ~ $ cat /etc/systemd/system/test.service 
[Unit]
Description=Test env

[Service]
Type=simple
EnvironmentFile=/etc/default/test
ExecStart=/bin/bash -c 'env'

[Install]
WantedBy=multi-user.target
✔ 16:47 ~ $ cat /etc/default/test
plop.plip=yay
✔ 16:49 ~ $ systemctl daemon-reload
✔ 16:43 ~ $ systemctl start test
✔ 16:43 ~ $ journalctl -u test.service 
-- Logs begin at Tue 2019-12-10 10:45:52 CET, end at Fri 2020-02-14 16:43:26 CET. --
févr. 14 16:43:26 ddy systemd[1]: test.service: Ignoring invalid environment assignment 'plop.plip=yay': /etc/default/test
[...]
févr. 14 16:43:26 ddy systemd[1]: Started Test env.
✔ 16:43 ~ $

févr. 14 16:43:26 ddy systemd[1]: test.service: Ignoring invalid environment assignment 'plop.plip=yay': /etc/default/test

TL;DR: it does not :(

Not sur 'how' or 'if' you want to deal with this but here it is :)

[hekmon]

I think I would just remove the .: you already do it on some cases.

Ex:
-httpListenAddr
-http.disableResponseCompression -> -httpDisableResponseCompression

But that kind is a (semver) breaking change which could need to wait for a v2.0.0 :/

[valyala]

Sorry for late response - I was busy with vmagent.

repeatable flags like -storageNode won't play well with env vars. Should you disable env vars for theses as multiples declaration will only lead to having only the last one set ?

There is an alternative syntax for repeatable flags, which can be used in environment vars:

-storageNode=foo -storageNode=bar can be substituted with -storageNode=foo,bar

flags with a dot like -memory.allowedPercent would translate to an invalid variable name. While it is still possible to hack around to set a var with such a name, I was not sure how systemd would handle it

How about substituting dots with underscores in env vars? I.e. -memory.allowedPercent=60 could be set as memory_allowedPercent=60 via env vars.

I think I would just remove the .: you already do it on some cases.

Yup, the inconsistency is due to historical and backwards compatibility reasons. I'd prefer consistently using dots in command-line flags, but it is too late to make changes :(

[hekmon]

Sorry for late response - I was busy with vmagent.

:O

There is an alternative syntax for repeatable flags, which can be used in environment vars:

-storageNode=foo -storageNode=bar can be substituted with -storageNode=foo,bar

I definitely missed that, might be worth adding to the readme or in the wiki, especially with the use of env vars ? (if it already is, my mistake)

How about substituting dots with underscores in env vars? I.e. -memory.allowedPercent=60 could be set as memory_allowedPercent=60 via env vars.

I think it is the simpliest solution with le lowest impact without renaming the flags... But this should definitely go in the readme/wiki with the alternative syntax (may be a dedicated "env vars configuration" section ?).

Thanks again for the great work 👍

[valyala]

I definitely missed that, might be worth adding to the readme or in the wiki, especially with the use of env vars ? (if it already is, my mistake)

This was undocumented. Added brief docs at fed2959658f541bebf03d264cfb60a63ce4418be .

I think it is the simpliest solution with le lowest impact without renaming the flags... But this should definitely go in the readme/wiki with the alternative syntax (may be a dedicated "env vars configuration" section ?).

Added this solution with brief docs in the same commit fed2959658f541bebf03d264cfb60a63ce4418be . Feel free sending a pull request that improves these docs and moves them into a separate section in REAMDE.md.

[valyala]

The fix, which substitutes dots with underscores in env var names has been included in v1.34.0. Closing the issue.