Skip to content

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

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

Merged
merged 1 commit into from
Nov 22, 2016

Conversation

tianon
Copy link
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
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
Member Author

tianon commented Nov 21, 2016

cc @cyli @mstanleyjones FYI

@ltangvald
Copy link
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
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