PerlDancer / Dancer2

Perl Dancer Next Generation (rewrite of Perl Dancer)
http://perldancer.org/
Other
542 stars 274 forks source link

Replace strings in Dancer2 config with environment variables #1628

Open mikkoi opened 3 years ago

mikkoi commented 3 years ago

I want to run my Dancer2 application in a docker but I want to run the container without mounting any local directories and I want to run the same once-built container in all environments, not compile it separately for different environments.

I would like to pass parts of the configuration as environment variables, e.g. DB_HOST and SITE_URL, and then apply these over the values in config.yaml when I start Dancer2 application.

Suggestion: Change the command config (or the underlying file reader function) so that it can replace environment variables in config strings, e.g.

plugins->database->connections->main->host: ${ENV:DB_HOST}
mikkoi commented 3 years ago

A script which creates config_local.yaml on the fly and then starts Dancer2 app would otherwise work but it means that container won't be immutable. Immutable is one of the architectural best practices listed by Google.

As it happens, this container is planned to run on Google Container Engine.

Using a tmpfs is also not an option because it would mean that container is dependent on the way it is being run (the runtime environment).

Substituting variables in config.yaml is not overly complicated in my mind. Many platforms use environment variables to pass information to containers, docker or others, e.g. Java jar. I have used Openshift platform and it gave me a very good example of how env vars are governed in order to provide secrets to the container.

By stating the env vars in the configuration, we make it clear to the programmer and to operations what is expected from the runtime environment. Besides this, it also makes local development easier if you "practice dotenv" - what The Twelve-Factor App recommends. I keep my environment in a .env file and I also use the same file when testing the container with local docker.

See:

racke commented 2 years ago

Yes, I can see the need to override Dancer config with environment variables without a start script. From my view a better place for the transformation would be a Dancer plugin if that is technically possible instead of changing Dancer's core.

mikkoi commented 2 years ago

Plugin would be a great way to do it. Then there wouldn't be any compatibility issues. But the configuration system is not "pluggable", i.e. there is no hooks where a plugin could attach, AFAIK.

cromedome commented 2 years ago

On Mon, Aug 23, 2021, at 3:12 AM, Mikko Koivunalho wrote:

Plugin would be a great way to do it. Then there wouldn't be any compatibility issues. But the configuration system is not "pluggable", i.e. there is no hooks where a plugin could attach, AFAIK.

This is something I plan to remedy... one day :) Hopefully sooner rather than later. I'd like the base configuration to just return a hashref, and ship our current config system as a plugin that is enabled by default. I need to clear the deck of a few other things before that happens though.

-- Jason A. Crome / CromeDome CPAN: https://metacpan.org/author/CROMEDOME GitHub: https://github.com/cromedome Twitter: https://twitter.com/cromedome Blog: https://cromedome.net

mikkoi commented 2 years ago

Great to hear. I'd like to volunteer for that project.

veryrusty commented 2 years ago

How much "configuration" does your app have ? You can define the settings inline:

package MyApp;
use Dancer2;

set foo => $ENV{FOO} // 'default-foo';
set bar => { baz => $ENV{BUZZ} };

1;
mikkoi commented 2 years ago

It has quite a lot of configuration. It interfaces with two web applications, there is JWT configuration and customizable urls. And it also talks with a database, using Dancer2::Plugin::Database.

xsawyerx commented 2 years ago

I think this is doable in core. It is unlikely to break any existing applications because I don't imagine many of them would be using something like $ENV{...} as a value. If any of them did, it is probably because they then eval that as code to run in the application itself to get the value - which is a major security issue.

The way I think this should work is by specifying in clear syntax how to retrieve something from ENV and not be an eval'ed piece of code. That's how security issues like Log4Shell happen.

plugins->database->connections->main->host: $ENV:DB_HOST

Here's how I see it:

I can imagine only two situations in which this is still a problem:

  1. Someone has "$ENV:FOO" in their config and do something with that string. We can release a version in which we warn of stuff like that, but I doubt this exists.

  2. If you have a system that processes the configuration and does a very simple substitution like:

{
    no strict;
    s/ ( \$ \B+  ) / *{$1} /xmseg;
}

(or something crazy like that)

It will not catch that this $whatever was actually \$whatever or that it was in single quotes.

In that case, I think you're doing something so wrong, there is no expectation for us to make it work.

mikkoi commented 2 years ago

I got a suggestion from @cromedome to change the ConfigReader role so that it would be possible for user to implement a hook which could do what I suggest above. I tried it but it was impossible or required more and deeper changes because using a hook created a circular dependency. I think the hooks are only usable after the App is setup and dancing.

So I went with a different approach. Configs are loaded by classes which implement the role Dancer2::Core::Role::ConfigReader. User can create custom classes to use instead of Dancer2::ConfigReader::FileSimple. Custom classes are activated via env var DANCER_CONFIG_READERS. If the env var is not set, app behaves like before.

There is an example where user's custom class is extending Dancer2::ConfigReader::FileSimple.

mikkoi commented 2 years ago

https://github.com/PerlDancer/Dancer2/pull/1636

xsawyerx commented 2 years ago

I can promise to review it this week, but not on which day.

I think having the config reader be a role is a good idea, but I'm also - aside from that - don't mind this functionality (populate config from an environment variable) be part of the core.

cromedome commented 2 years ago

I will review this week, but like @xsawyerx I am not sure when yet.