Skip to content

Add "file_env" support, especially for Docker secrets#237

Merged
yosifkit merged 1 commit into
docker-library:masterfrom
infosiftr:secrets
Nov 22, 2016
Merged

Add "file_env" support, especially for Docker secrets#237
yosifkit merged 1 commit into
docker-library:masterfrom
infosiftr:secrets

Conversation

@tianon
Copy link
Copy Markdown
Member

@tianon tianon commented Nov 21, 2016

This adds explicit support for the following:

  • MYSQL_ROOT_PASSWORD_FILE
  • MYSQL_DATABASE_FILE
  • MYSQL_USER_FILE
  • MYSQL_PASSWORD_FILE

See also:

I've only updated 8.0/docker-entrypoint.sh here so that this can serve as a straw-man for discussion -- once the discussion concludes, I'll update the PR with the result and push the functionality across the board to all supported versions. 👍

@tianon
Copy link
Copy Markdown
Member Author

tianon commented Nov 21, 2016

I saw this was added to mysql/mysql-server in mysql@0473555, but IMO it's cleaner to add an explicit "get the password from this specific file" option than to overload the existing option. For example, if someone uses a bad -w/--workdir on their container, their password might be accidentally interpreted as a file to read, and there will be no external indication that it's happened and the admin will be scratching their head wondering why their password isn't working. 😅

(That being said, we defer to you, @ltangvald -- however you think this should be implemented here is the direction we'll go. 👍)

@tianon
Copy link
Copy Markdown
Member Author

tianon commented Nov 21, 2016

cc @cyli @mstanleyjones FYI

@ltangvald
Copy link
Copy Markdown
Collaborator

Hm, yeah. I'm not 100% sure why we used the same variable (other than a general desire to keep the number of extra env variables down). We considered the chance of accidentally setting the password to a valid file path low, but I agree it's cleaner to make it explicit.
The file_env function looks a bit complex to me, but it's nice to have the functionality and I can't think of a simpler way off the top of my head, so overall I think this looks good :)

This adds explicit support for the following:

- `MYSQL_ROOT_PASSWORD_FILE`
- `MYSQL_DATABASE_FILE`
- `MYSQL_USER_FILE`
- `MYSQL_PASSWORD_FILE`
@tianon
Copy link
Copy Markdown
Member Author

tianon commented Nov 22, 2016

Yeah, the goal of the function was to abstract the behavior in a clean, generic way so we don't have "this or that" logic peppered all over the file (which is error prone on top of verbose). 😅

I've updated to push this change to all the versions now. 😄 👍

@yosifkit yosifkit merged commit 400ca81 into docker-library:master Nov 22, 2016
@yosifkit yosifkit deleted the secrets branch November 22, 2016 18:25
tianon added a commit to infosiftr/stackbrew that referenced this pull request Nov 23, 2016
- `elasticsearch`: 1.7.6, 2.4.2
- `mongo`: 3.4.0-rc5
- `mysql`: add `file_env` support (docker-library/mysql#237)
- `percona`: 5.5.53-rel38.5-1.jessie
- `postgres`: add `tzdata` to alpine variant (docker-library/postgres#226), add `file_env` support (docker-library/postgres#225)
- `python`: 3.6.0b4
- `rabbitmq`: 3.6.6
- `redmine`: add `file_env` support (docker-library/redmine#43)
- `rocket.chat`: 0.46.0
TFenby added a commit to TFenby/docs that referenced this pull request Apr 9, 2017
Adds a section in the docs for the new capabilities added by docker-library/mysql#237.
dmundra added a commit to dmundra/docker-mysql-backup that referenced this pull request Nov 13, 2017
…he root password (see docker-library/mysql#237). If the variable exists, cat the contents into the MYSQL_ROOT_PASSWORD field before running the mysqldump command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants