Page MenuHomePhabricator

[keyserver] Pass all variables in .env to Node keyserver
ClosedPublic

Authored by ashoat on Jun 1 2022, 1:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 18, 5:03 PM
Unknown Object (File)
Wed, Sep 18, 5:03 PM
Unknown Object (File)
Wed, Sep 18, 5:03 PM
Unknown Object (File)
Tue, Sep 17, 6:34 AM
Unknown Object (File)
Mon, Sep 16, 5:07 PM
Unknown Object (File)
Sat, Sep 14, 12:44 AM
Unknown Object (File)
Fri, Sep 13, 6:39 PM
Unknown Object (File)
Thu, Sep 12, 8:21 PM

Details

Summary

Otherwise only the ones listed in the environment: section will be included. I think it's safe to forward all of these, and I figure it's not worth individually listing every JSON config name here.

You might ask what the point of leaving COMM_MYSQL_DATABASE, COMM_MYSQL_USER and COMM_MYSQL_PASSWORD is. I figure it's better for discoverability to list these explicitly here, as they are required for the database to initialize and the Node keyserver to run.

Depends on D4173

Test Plan

I set up a keyserver/.env that looks like this:

COMM_MYSQL_DATABASE=commdev
COMM_MYSQL_USER=commdev
COMM_MYSQL_PASSWORD=pass
COMM_JSONCONFIG_facts_landing_url='{"baseDomain":"http://localhost","basePath":"/commlanding/","baseRoutePath":"/commlanding/","https":false}'
COMM_JSONCONFIG_facts_commapp_url='{"baseDomain":"http://localhost","basePath":"/comm/","https":false,"baseRoutePath":"/comm/"}'

Without this diff, the COMM_JSONCONFIG environmental variables weren't being passed in.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat requested review of this revision.Jun 1 2022, 2:00 AM

You might ask what the point of leaving COMM_MYSQL_DATABASE, COMM_MYSQL_USER and COMM_MYSQL_PASSWORD is. I figure it's better for discoverability to list these explicitly here, as they are required for the database to initialize and the Node keyserver to run.

If I'm understanding correctly, we're passing in environment variables in "two ways" here.

  1. By specifying keys and values in the .env file
  2. By specifying keys and maybe values in the environment "section" of the YAML config file

My understanding is that for (2), if the values aren't specified they're pulled in from the shell in which docker compose is run. If that's the case, have we looked into the precedence of which "value" is actually forwarded in the case of a collision?

eg do we know what happens in the following scenario?

.env:

COMM_MYSQL_DATABASE=hello

~/.profile:

export COMM_MYSQL_DATABASE=goodbye

.docker-compose.yml:

...
env_file:
  - .env
environment:
  - COMM_MYSQL_DATABASE
...

(this could be a naive question... I haven't spent much time with Docker)


My instinct is that generally it would be best to handle all of the environment variables in "one place" and I'm biased towards that being in the .env file. If I understand correctly, this would remove the developer's shell "from being a factor" and would make things more predictable? On the other hand maybe there's some benefit to having the required keys explicitly spelled out in the docker-compose.yml file?

This revision is now accepted and ready to land.Jun 1 2022, 7:10 AM

From the docs:

When you set the same environment variable in multiple files, here’s the priority used by Compose to choose which value to use:

  1. Compose file
  2. Shell environment variables
  3. Environment file
  4. Dockerfile
  5. Variable is not defined

I'm not sure this really answers the question you presented, which is ultimately about whether env_file or environment sections in a Docker Compose file will take precedence. I think the env_file qualifies as 2 above rather than 1, but I'm not sure, since the env_file is being specified in the Compose file. So my guess would be that in that case, the variable resolves to goodbye.

My instinct is that generally it would be best to handle all of the environment variables in "one place" and I'm biased towards that being in the .env file. If I understand correctly, this would remove the developer's shell "from being a factor" and would make things more predictable?

Since environment variables defined in the developer's shell only directly impact the Docker Compose file, and don't bleed through into containers unless explicitly referenced via the environment section, I am not that worried about config in the developer's shell accidentally leaking through.

On the other hand maybe there's some benefit to having the required keys explicitly spelled out in the docker-compose.yml file?

The env_file approach doesn't allow the specification of defaults, so the most we could do is remove COMM_MYSQL_DATABASE, COMM_MYSQL_USER, and COMM_MYSQL_PASSWORD. As suggested above, I think leaving these in is good for making these required variables more discoverable.