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

Add config providers #8957

Merged
merged 23 commits into from Apr 18, 2019

Conversation

Projects
None yet
9 participants
@josevalim
Copy link
Member

commented Apr 15, 2019

This PR adds config providers to Elixir releases (see #8612).

It is a large-ish PR broken in two parts:

  1. Adding the Config, Config.Reader and Config.Provider modules
  2. Adding config provider support to releases

I will explore the rationale for each part next. Note this is still WIP (docs and tests missing) but I have submitted it so we can get the CI running and open up for feedback.

Adding the Config, Config.Reader and Config.Provider modules

Since we need to use config files during releases and releases do not necessarily have Mix available, we have moved Mix.Config to Config. We also used this opportunity to refactor the API.

The first change is to use import Config instead of use Mix.Config, since import is more idiomatic. We also kept only the user config in Config and move all of the operations to manipulate config files to the Config.Reader module. Here is a summary table:

Old New
use Mix.Config import Config
Mix.Config.merge/2 Config.Reader.merge/2
Mix.Config.read!/2 Config.Reader.read!/2
Mix.Config.eval!/2 Config.Reader.read_imports!/2
Mix.Config.persist/1 Application.put_all_env/2

We have also added the Config.Provider module which outlines the behaviour to be implemented by config providers. It is simply two functions. The Config.Reader module itself is a provider that reads a single configuration file.

Adding config provider support to releases

We have added config providers support to releases using a restart-once approach. This is different from distillery and relx that boot the VM twice. Instead, we only boot the VM once and restart the Erlang system once it has been configured. The trick is to rewrite the sys.config during after running config providers.

Therefore, to use config providers, we need writable permissions somewhere (which can be customizable with RELEASE_TMP). If you don't use providers, you don't need write permission. Also, since config providers run when only a small part of the VM is started, they need to explicitly start any application they may need.

By default there is a single config provider which checks for config/releases.exs. If config/releases.exs is present, it will be automatically copied to your release and executed at runtime, the first time your release boots.

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Unfortunately the current approach doesn't work on Windows because Windows only allows one -config file. One approach is to copy the sys.config itself to another location and override it. I have opened an issue in Erlang issues tracker and we will resolve accordingly based on that.

josevalim added some commits Apr 15, 2019

@michalmuskala

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I like this very much, except for one thing - caching of the provider results. I think it will be very surprising and error prone.

Let's imagine a situation where you deploy to heroku with the new releases and providers that reads from system env, and the FOO var in particular. This means that when you run the command heroku config:set FOO=new_value, heroku will restart the app with the new value in system env, but because the evaled config file already exists, providers won't execute again and the new value won't be picked up. This is in no way a desired behaviour. Not only that, but even after some googling I didn't find a reliable way to tell heroku to discard this file in all dynos so that a new version could be generated.

@sorentwo

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Caching the compiled configuration seems like a no-go to me as well.
Outside of the tmp folder Heroku provides a read only file system, so I’m not sure you could even get the release to boot.

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@sorentwo not though we need to have write permissions to something, regardless if caching or not.

@sorentwo

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

@josevalim I double checked, there are write permissions inside the application directory, so that isn't an actual problem.

Caching the initial config and then not overwriting it when the environment changes, as @michalmuskala mentioned, would be enough to prevent Heroku users from using releases though.

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Caching the initial config and then not overwriting it when the environment changes, as @michalmuskala mentioned, would be enough to prevent Heroku users from using releases though.

Yes, I agree. I was just commenting on the second part. :D

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

So I pushed a new commit that uses a new random file per boot. The following commit adds automatic pruning of those config files (although it means you can't call another System.restart or perform hot code upgrades unless you disable the pruning).

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

And thank you @umamaistempo, @michalmuskala, and @sorentwo for the feedback!

Show resolved Hide resolved lib/elixir/lib/config.ex Outdated

fertapric and others added some commits Apr 15, 2019

Update lib/mix/lib/mix/tasks/release.ex
Co-Authored-By: josevalim <jose.valim@gmail.com>
Update lib/elixir/lib/config.ex
Co-Authored-By: josevalim <jose.valim@gmail.com>
@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

I pushed docs for reader and provider. Now I need to:

  • Tests for provider
  • Tests for Mix.Release
  • Integration tests for mix release
  • Docs for mix release
@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Show resolved Hide resolved lib/elixir/lib/config.ex Outdated
Show resolved Hide resolved lib/elixir/lib/config.ex Outdated
Show resolved Hide resolved lib/elixir/lib/config.ex Outdated
Show resolved Hide resolved lib/elixir/lib/config.ex
@ggcampinho
Copy link
Contributor

left a comment

Really nice :) I really like the idea, I'm still trying to wrap my head around this, but one concern that I have is providing the current configuration back to the providers. The configuration can have sensitive data and this could be exposed by some malicious code in some cases, I believe.

My suggestion would be to always merge the configs provided by the provider instead of giving the freedom to the user to do that, but maybe I'm not foreseeing some user case.

Show resolved Hide resolved lib/elixir/lib/config.ex Outdated
Show resolved Hide resolved lib/mix/lib/mix/release.ex

britto and others added some commits Apr 18, 2019

Update lib/elixir/lib/config.ex
Co-Authored-By: josevalim <jose.valim@gmail.com>
Update lib/elixir/lib/config.ex
Co-Authored-By: josevalim <jose.valim@gmail.com>
@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

The configuration can have sensitive data and this could be exposed by some malicious code in some cases, I believe.

I am not really worried about this. A config provider is regular Elixir code and as such it has access to absolutely everything. if you are worried about a bad config provider, the current config is the least of your concerns. :)

josevalim added some commits Apr 18, 2019

@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Tests are done, now let's wait for CI and once it is green it will be merged!

@josevalim josevalim marked this pull request as ready for review Apr 18, 2019

@josevalim josevalim force-pushed the jv-config branch 3 times, most recently from 7b43db3 to 27d3cca Apr 18, 2019

@josevalim josevalim force-pushed the jv-config branch from 27d3cca to 702c746 Apr 18, 2019

@josevalim josevalim merged commit 8faa40e into master Apr 18, 2019

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@josevalim

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

❤️ 💚 💙 💛 💜

@josevalim josevalim deleted the jv-config branch Apr 18, 2019

@josevalim josevalim referenced this pull request Apr 19, 2019

Closed

Releases #8612

6 of 6 tasks complete
@Qqwy

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I just took some time to read through the code and documentation, and would like to say: this is really cool stuff. I think the configuration story in Elixir will drastically improve with this. Awesome job! 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.