Skip to content
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

Configurable command flags through environment variables #4277

Closed
marcaurele opened this issue Jan 25, 2022 · 15 comments · Fixed by #6508
Closed

Configurable command flags through environment variables #4277

marcaurele opened this issue Jan 25, 2022 · 15 comments · Fixed by #6508

Comments

@marcaurele
Copy link

What part of OPA would you like to see improved?

I would like to avoid having to overwrite the container command line to customize the execution for an environment. For example, I would like to simply switch the logging level based on an environment variable, instead of editing the command line with --log-level debug.

Describe the ideal solution

The idea would be to expose all command flags as environment variables so that by setting any of them to a value, it would have the same effect as specifying a custom flag for the container execution. Maybe there is a generic solution that can be applied to all of them at once, which would be ideal to turn all flags to corresponding environment variables.

Describe a "Good Enough" solution

The most important command is run to address as it's the one used for execution. Looking at the list of flags, I would say that --log-level and --log-format are 2 important ones to start with, as well as the one for the config file --config-file. The change can be incremental and add flags to environment variable mapping based on perceived importance of them.

Additional Context

It is not possible to build a container image by providing an argument in CMD1 that will be interpolate based on an environment variable. The workaround is to go through sh which is not available in the image (and should not).

Unlike the shell form, the exec form does not invoke a command shell. This means that normal shell processing does not happen.

For example this won't work:

ENV LOG_LEVEL=info

CMD ["run", "--log-level", $LOG_LEVEL]

resulting in (Docker tries to access /bin//sh to interpolate the variable itself):

Error: unknown command "/bin/sh" for "opa"
Run 'opa --help' for usage.
unknown command "/bin/sh" for "opa"

Footnotes

  1. https://docs.docker.com/engine/reference/builder/#cmd

@anderseknert
Copy link
Member

Thanks for filing this @marcaurele 👍

I could imagine doing something like what Kafka does, where any config value can be provided as a prefixed environment variable, like KAFKA_ADVERTISED_LISTENERS, etc. That would help avoiding clashes with other environment variables set.

As for a solution, I think something like this could be accomplished with Viper, but if we only used this for log level/format initially, we might be better off doing a simple check without that.

@marcaurele
Copy link
Author

Yes, using a prefix is also recommended, I forgot to mention it.

@stale
Copy link

stale bot commented Feb 24, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Feb 24, 2022
@suneelkumarch
Copy link

tagging myself here, as I am also looking for an option to configure command flags through environment variables, esp. --log-level

@anderseknert just curious, any update or workaround?

@marcaurele

@stale stale bot removed the inactive label May 13, 2022
@anderseknert
Copy link
Member

No update or workaround here that I'm aware of @suneelkumarch. If you'd like to work on this, that'd be much welcome :)

@stale
Copy link

stale bot commented Jun 13, 2022

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

@stale stale bot added the inactive label Jun 13, 2022
@colinjlacy
Copy link
Contributor

@anderseknert I'd like to work on this if that's ok.

@anderseknert
Copy link
Member

Of course! 👍

@fioreale
Copy link

fioreale commented Dec 4, 2023

@anderseknert I'd like to work on this if that's ok.

I need this feature too. Keep me updated if you need two more eyes on this!

@anderseknert
Copy link
Member

@fioreale if you'd want to help review / test the PR later, that'd be welcome!

@colinjlacy
Copy link
Contributor

@anderseknert which would we want to take precedence if both the env var and the flag value are set? My gut tells me the flag value in the command line should take precedence, since it's more explicit and closer to the execution, but I'd like to hear other thoughts.

@marcaurele
Copy link
Author

This is what I would expect, as you could have env variables set in a container for example, but for specific execution you want to be able to override them with flags.

@anderseknert
Copy link
Member

Agreed!

@colinjlacy
Copy link
Contributor

Wrapping up this branch for a PR - I have to get approval at work, ensuring that this doesn't violate any OS contribution restrictions. In the mean time, just a heads up on the approach I took.

There's a new internal package in the cmd package called env, and it:

  • creates a new viper instance for each command
  • when it's called, looks up each flag on a command and finds a matching env var:
    • e.g. opa fmt --write would be OPA_FMT_WRITE
  • if the flag value is unchanged from the default, and the env var is set, it takes the env var value and applies it to the flag

I added this to the PreRunE hook on each command in the cmd package, so that it only runs for the specific command that's called, and doesn't throw errors for other commands that are registered. In cases where that hook was already in place, I added this as the last possible error returned, as the other error checks (e.g. validateArgs()) seemed to be higher priority error checks.

Copy link

stale bot commented Jan 20, 2024

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

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

Successfully merging a pull request may close this issue.

5 participants