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

support environment interpolation in the config file #16

Closed
bkcsfi opened this issue Nov 18, 2015 · 12 comments
Closed

support environment interpolation in the config file #16

bkcsfi opened this issue Nov 18, 2015 · 12 comments

Comments

@bkcsfi
Copy link

bkcsfi commented Nov 18, 2015

It would be nice to be able to reference environment variables within the configuration file. e.g. if I want to one Dockerfile that can be used on a 'dev consul' and later in production, I might choose to change (future) tag support or use a different consul master port.

@tgross
Copy link
Contributor

tgross commented Nov 23, 2015

I like this idea.

From an implementation standpoint we'd probably want to walk the Config struct after we've parsed it. For each string in the config, we'd need to:

  • parse out "things that look like environment variables"
  • ask os.Getenv for their values
  • inject the new values

Complications from a design standpoint are:

  • Go's os.Getenv doesn't make any distinction between if the environment doesn't have a value, and if it was never set in the first place. If os.Getenv returns an empty string, do we not replace the variable with empty string or do we not do the replacement?
  • How to we pass environment variable literals to the underlying applications? Ex. if our user-defined health check is curl -s --fail localhost:8000/api/health?${SOME_CONFIG_VALUE} then it looks like we'll need a way to escape those values.

Seems doable though. I'm going to mark this as an open enhancement where we'd encourage a PR.

That being said and just in case it's not obvious, we do currently support passing the entire Containerbuddy configuration in as an environment variable (ref https://github.com/joyent/containerbuddy/blob/master/src/containerbuddy/config.go#L65-L69). You can pass in either a config file path or JSON (as I've done in this other work-in-progress project). I realize that's not quite what you're asking for here but there's a bit of runtime configuration available in that way.

@bkcsfi
Copy link
Author

bkcsfi commented Nov 23, 2015

instead of adding support for environment interpolation directly in containerbuddy, could you add a command line option something like --configfromexec "some_program_path some_args more_args"

where some_program_path is a program in the container that will print to stdout the configuration file

if some_program_path returns non-zero rc you can exit the container with that error.. also if some_program_path prints to stderr that could go to the container log.

I like the idea of containerbuddy handling signals, logging, etc.. and this would provide some mechanism for users to generate a config file on-the-fly in whatever format they prefer (e.g. load from environment could become something like 'echo $ENVVARIABLE')

@tgross
Copy link
Contributor

tgross commented Nov 30, 2015

instead of adding support for environment interpolation directly in containerbuddy, could you add a command line option something like --configfromexec "some_program_path some_args more_args"

If we were to do this at all, I'd think the better option would be to use the existing -config flag and just give it additional format handlers. Right now it handles file:// but maybe we could extend that to include http:// or (spit-balling here) exec:?

@bkcsfi
Copy link
Author

bkcsfi commented Nov 30, 2015

My use-case that prompted this issue is pretty limited (dev vs production selection w/o building a new container by setting ENV vars).

I wonder if this idea is getting too complex. I can't see how http:// support would be practical because trying to manage deployment of containers and keeping them sync'd with a central web server from which config info could be downloaded would be vary hard.. and you'd still need to be able to pass in an ENV var for the command line to be passed to the web server indicating which config to download.

while exec: gives a lot of flexibility it then requires users to add another program to their container.

Maybe focusing on the Env variable expansion would be better. I think expanding variables in the entire config file before parsing would probably be simplest.

Is it worth looking at how Dockerize uses text.template w/ Env variables?

https://github.com/jwilder/dockerize

@tgross
Copy link
Contributor

tgross commented Nov 30, 2015

I wonder if this idea is getting too complex.

Yeah, I think you're right there.

Is it worth looking at how Dockerize uses text.template w/ Env variables?

We could really probably just get away with {{ MY_VARIABLE }} style escaping.

@j0hnsmith
Copy link

Seems like telling a container that uses Containerbuddy where Consul is should be trivial, but isn't. I could set a hosts file entry via docker run --add-host but how can I change the port at runtime? I don't want to have to generate the file myself and pass it in via -v.

Variable interpolation in the config file would be good (docker compose does this for yaml files, but that's a Python project).

Containerbuddy looks great BTW.

@j0hnsmith
Copy link

For anyone who's interested, I made a hack that enables docker run -d -e "CB_CONSUL_HOST=1.2.3.4" -e "CB_CONSUL_PORT=12345" container-using-containerbuddy ..., it overrides the app.json consul value

j0hnsmith@74f28fe

@tgross
Copy link
Contributor

tgross commented Dec 9, 2015

@j0hnsmith I'd like to avoid having backend-specific environment variables baked into the application because as we add discovery backends we'll have an explosion of configuration. You can currently set the Containerbuddy json entirely by stuffing it into the CONTAINERBUDDY environment variable (ref https://github.com/tgross/triton-nginx/blob/master/example/common-compose.yml#L11 for an example where I've done this).

I think interpolating environment variables is the right design for this. I'm personally a bit backed up in the next couple weeks but I'd certainly be open to anyone who wants to submit a PR for this if I can't get to it first.

@j0hnsmith
Copy link

I agree with your reasoning entirely, my hack is not a solution by any means, it was just something that allowed me to do some testing.

This is only the second thing I've done in go, I think I'd struggle to implement the variable interpolation solution.

justenwalker added a commit to justenwalker/containerpilot that referenced this issue Dec 10, 2015
@justenwalker
Copy link
Contributor

I've got a PR for this at #46 which I'm implementing this feature as a Go template.

For environment variables, this would mean doing something like: {{ .ENV_VAR_NAME }}

@tgross tgross removed their assignment Dec 14, 2015
@tgross tgross added the open PR label Dec 14, 2015
@tgross
Copy link
Contributor

tgross commented Dec 15, 2015

This PR was merged in and we'll package it into the next release. (Which we should do this week, maybe once #48 is ready.)

@tgross tgross closed this as completed Dec 15, 2015
@tgross
Copy link
Contributor

tgross commented Dec 16, 2015

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

No branches or pull requests

4 participants