[RFC][DX] Defaulting missing env variables to empty string #31497

Jean85 · 2019-05-14T08:48:24Z

Description
I'm hitting a constant issue while deploying my Symfony apps with Docker: during the Docker build I want to issue the cache:warmup command, and in that environment a lot of env variables are missing, because I'm not actually in the prod environment.

This forces me to define in the parameters a bunch of default values for env variables, so the container dump process will not complain about those. And those values are normally defaults or empty string.

I would like to suggest a new behavior: not complaining about missing env vars, at least during cache warmup: this would allow us to avoid defining all those variables as empty, and concentrate on having them just in the real prod environment.

Example
This could be done in some way like bin/console cache:warmup --ignore-missing-envs. WDYT?

The text was updated successfully, but these errors were encountered:

ciaranmcnulty · 2019-05-14T08:53:37Z

I think this makes sense if it's explicitly enabled by a flag like you suggest - we similarly don't know the runtime values in advance so can't set placeholders

nicolas-grekas · 2019-05-14T09:18:44Z

To me the best practice would be to list them in the .env file, instead of using parameters.
That allows documenting them and ease discovery of what your app needs. Using the flag might as well hide mistakes. Another best practice is that warmup should not reference any env var, so I'm not totally sure this builds on a legit root issue.

Jean85 · 2019-05-14T09:26:47Z

Yes @nicolas-grekas, but .env using is discouraged in prod environments, and I'm having this issue while building for that env.

Also, as rightly pointed out by @linaori on Slack, it seems that the errors stems from the fact that we have a "chicken and egg" kind of issue, with bin/console that requires to boot up the container, and it's itself used to run cache:warmup, so you can't run it with a warmed up cache. So, if you use any env variable in something that is boostrapped while starting bin/console cache:warmup, you're stuck with errors.

nicolas-grekas · 2019-05-14T10:17:08Z

It's not discouraged anymore, it's actually always loaded now. See composer dump-env prod from flex.

ciaranmcnulty · 2019-05-14T10:26:49Z

@nicolas-grekas is there somewhere I can catch up on the discussion around that change?

teohhanhui · 2019-05-14T18:02:41Z

@nicolas-grekas See for example here: https://github.com/symfony/recipes/blob/3b19bd94886555df1997b441b6cf6a1fab088f3a/doctrine/doctrine-bundle/1.6/config/packages/doctrine.yaml#L2-L6

Workarounds like this are necessary for a successful docker build, if we do bin/console cache:clear in the Dockerfile. Not warming up the cache during docker build is bad, as it means a slow container start.

nicolas-grekas · 2019-05-14T19:39:20Z

@ciaranmcnulty I think https://symfony.com/blog/new-in-symfony-flex-1-2 could be a good start
@teohhanhui sure, that's not strictly required as an empty DATABSE_URL in .env would be good now (this should maybe be removed from the recipe actually)

teohhanhui · 2019-05-14T22:27:06Z

@nicolas-grekas If I understand correctly, you mean we should put the default value for DATABASE_URL in .env, instead of the yaml file, right? Yeah, that sounds good.

nicolas-grekas · 2019-05-15T11:31:10Z

@teohhanhui yep that's what I mean - and it looks like there is already a default value, so we could just remove these lines the yaml file: https://github.com/symfony/recipes/blob/3b19bd94886555df1997b441b6cf6a1fab088f3a/doctrine/doctrine-bundle/1.6/manifest.json PR welcome

Jean85 · 2019-05-15T14:01:26Z

@nicolas-grekas to recap: the (updated) suggested way to setup in a Docker prod env is then:

use dotEnv in prod too
copy .env into the container image during the build
use composer dump-env prod to fill in the missing env variables

Is that correct?

teohhanhui · 2019-05-15T20:59:20Z

It's only safe to use composer dump-env prod if it doesn't dump actual env vars. If it does, I'd see it as a bug and a security risk.

Edit: That seems to be correct based on the blog post.

dunglas · 2019-06-12T19:20:03Z

This change causes issues with Docker and other container systems (including for the API Platform distribution).

Previously, it was possible to build the container without having environment variables set. A hack was already necessary, but it was a one easy to maintain:
RUN echo '<?php return [];' > .env.local.php

Now even this hack fails, and the workarounds aren't nice:

The first one:

remove .env* from .dockerignore (excluding such files is best practice)
copy the main .env file in the container
run composer install
generate an autoloader
run composer dump-env
remove the copied file .env file from the container

Here is the related diff in API Platform: api-platform/api-platform#1167

The other one, even uglier, is to generate the file manually:

RUN echo "<?php return ['DATABASE_URL' => ''];" > .env.local.php

dunglas · 2019-06-12T19:22:00Z

Also, having things looking like secrets stored (even if they aren't production values) in the container will trigger warnings from many security systems.

nicolas-grekas · 2019-06-13T07:23:07Z

@dunglas as we discussed onSlack, this change in the recipe was a hack around the root issue, which is that the DATABASE_URL env var is read at build time. This should be fixed in DoctrineBundle I suppose.
I think we can close this issue, right?

teohhanhui · 2019-06-13T07:49:30Z

@dunglas I believe we should use composer dump-env prod in our Dockerfile. Unfortunately it's not possible to delete a file once we COPY it. I'll look into that.

bendavies · 2019-06-13T09:23:30Z

It's only safe to use composer dump-env prod if it doesn't dump actual env vars. If it does, I'd see it as a bug and a security risk.

Edit: That seems to be correct based on the blog post.

@teohhanhui is does? symfony/flex#507

Ocramius · 2020-01-02T10:46:00Z

@Jean85 this is indeed still an issue: hitting it as cache:clear complains due to a missing SENTRY_DSN. Setting SENTRY_DSN= in a .env works, but it is not something I'd commit.

It's not discouraged anymore, it's actually always loaded now. See composer dump-env prod from flex.

@nicolas-grekas .env is not supposed to exist during docker build operations, since the image is to be used for publishing (keyword public), and then adapted with different .env. Providing a .env to be added to the image (with fake/invalid values) is also a problem, as @dunglas said:

(excluding such files is best practice)

In a production system following 12FA principles (Dokku, Heroku, K8s, Swarm), .env files won't exist at all, as secrets as mounted as individual environment variables that will only ever exist at runtime, and never at build time.

@Jean85 I think the problem is more to the core: the application should not be bootstrapped to run things like cache:clear, as it can only be bootstrapped when all environment and configurations are set. The fact that bin/console is used in composer hooks is the root of the issue, so we're hitting a bit of a chicken-egg problem.

teohhanhui · 2020-01-02T10:49:31Z

You should use an empty value in .env:

SENTRY_DSN=

The .env file is checked in to the repository. During Docker image build, you should do composer dump-env prod, after which you can remove the .env file.

Ocramius · 2020-01-02T10:52:37Z

Yes, I just clarified that this (the empty value) should not be put in .env, as it is also misleading and also confuses security scanners.

For now, I'm working around the issue in key components of the application that need to run without environment by force-feeding variables where needed, such as in my Dockerfile:

FROM ...

ADD ...

RUN SENTRY_DSN= composer install -a

teohhanhui · 2020-01-02T11:04:42Z

Yes, I just clarified that this (the empty value) should not be put in .env, as it is also misleading and also confuses security scanners.

But the .env file is meant for default values. It's the security scanners which have got it wrong.

Ocramius · 2020-01-02T11:05:12Z

.env.dist is meant for default values :)

teohhanhui · 2020-01-02T11:06:05Z

.env.dist is meant for default values :)

That was before https://symfony.com/doc/current/configuration/dot-env-changes.html

#28533

Ocramius · 2020-01-02T11:09:25Z

Let me be more clear then: in applications (in general, not symfony), .env is not committed, and .env.dist is committed instead.

What symfony does is then certainly questionable (and I am questioning it).

teohhanhui · 2020-01-02T11:10:55Z

See the discussion in #28533, specifically #28533 (comment)

(I was questioning the change too lol... But that's how things work now.)

Ocramius · 2020-01-02T11:16:06Z

Yeah, in the current project I'm currently getting rid of .env completely: to be used for development only (and only loaded only by docker-compose would be ideal).

The fact that symfony does also consider that file in index.php/console is making everything much more complex and error-prone, since now we have a PHP process potentially running with different environment than what's expected to be set in the hypervisor (docker, in my specific case), due to files being loaded.

I built up at least a dozen hours of work because of issues where docker (or a process) had an environment variable set, and that variable was then being replaced by DotEnv. I stumbled on this issue because a developer couldn't figure it out, and it only arised during CI/build runs (where .env is not present by design)

teohhanhui · 2020-01-02T11:18:17Z

issues where docker (or a process) had an environment variable set, and that variable was then being replaced by DotEnv.

(where .env is not present by design)

Sounds like symfony/recipes#611 if you're talking about .env.local.php.

Jean85 · 2020-01-03T11:57:52Z

Two notes:

Sentry DSN is public now since version 9 of the server: it no longer contains any secret, so no issues in publishing it; that is due to the fact that JS SDK uses the same, so it has to be public anyway
I agree with @Ocramius on the questionable stuff, but it seems strange to me that the DotEnv is overriding the env variables: they should always take precedence over any file.

gmorel · 2020-11-10T22:07:21Z

@Jean85, @Ocramius, I had the same issue with MAILER_DSN from the Mailer component (and with DATABASE_URL too).

I can't explain why but if you explicitly tell Docker to load the missing vars. You will no more have empty values.

DockerFile

ARG APP_ENV
ARG MAILER_DSN

docker-compose.yml

services:
    citadel:
        build:
            args:
                ENV: 'prod'
                APP_ENV: 'prod'
                MAILER_DSN: ${MAILER_DSN}

And now composer dump-env prod (without prefixing vars) will work perfectly during the Docker build.

Might help other people to have a little hint it in the Exception message when building from a DockerFile ?.

Keep up the good work ! 🚀

carsonbot · 2021-05-11T13:04:22Z

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

carsonbot · 2021-05-25T13:08:40Z

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

Jean85 · 2021-05-26T11:04:41Z

It seems to me that we still have to find consensus about the solution for this issue...
Surely we need to push the bundle maintainers to check how their configuration handles placeholders for env variables, but I don't know how to proceed...

carsonbot · 2021-11-27T13:04:04Z

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

carsonbot · 2021-12-11T13:06:19Z

Could I get a reply or should I close this?

carsonbot · 2021-12-25T13:08:52Z

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

javiereguiluz added DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels May 15, 2019

ismail1432 mentioned this issue May 16, 2019

Remove default database URL as it already defined in .env symfony/recipes#588

Merged

dunglas mentioned this issue Jun 12, 2019

Fix builds api-platform/api-platform#1167

Merged

carsonbot added the Stalled label May 11, 2021

carsonbot removed the Stalled label May 26, 2021

carsonbot added the Stalled label Nov 27, 2021

carsonbot closed this as completed Dec 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][DX] Defaulting missing env variables to empty string #31497

[RFC][DX] Defaulting missing env variables to empty string #31497