Skip to content
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

Add support for passing a file path as the value for DB password #186

Merged
merged 3 commits into from
Nov 17, 2016
Merged

Add support for passing a file path as the value for DB password #186

merged 3 commits into from
Nov 17, 2016

Conversation

mdlinville
Copy link
Contributor

@mdlinville mdlinville commented Nov 14, 2016

Allows use of Docker secrets to store these credentials
when WordPress is run as a Docker service. If the secret
has been granted to the container, the
password will be available within the container as
an unencrypted string in /run/secrets/.

An example incantation:

docker service create --name="wordpress" \
                            --network="wp_network" \
                            --replicas="3" \
                            --publish 30000:80 \
                            --secret="mysql_password" \
                            -e WORDPRESS_DB_USER="root" \
                            -e WORDPRESS_DB_PASSWORD="/run/secrets/mysql_password"
                            -e WORDPRESS_DB_HOST="mysql:3306" \
                            wordpress

Signed-off-by: Misty Stanley-Jones [email protected]

…SWORD

Allows use of Docker secrets to store these credentials.
If the secret has been granted to the container, the
password will be available within the container as
an unencrypted string in /run/secrets/.

Signed-off-by: Misty Stanley-Jones <[email protected]>
@cyli
Copy link

cyli commented Nov 15, 2016

This is awesome @mstanleyjones, thanks so much for getting this rolling!
Having never used wordpress before, I don't know very much about how to set it up, but looking at https://hub.docker.com/r/library/wordpress/, would it make sense to add secret support for some of the other env vars too? For instance (although I'm not sure what these do): WORDPRESS_AUTH_KEY, WORDPRESS_LOGGED_IN_KEY.

Also, do these changes also have to be made to all the docker-entrypoint.sh files in all the different php apache/fpm subdirectories? (not sure if there's some other script that will update them automatically)

@tianon
Copy link
Member

tianon commented Nov 15, 2016

IMO it'd be cleaner if we add _FILE versions of these environment variables so that we're being explicit that we want their contents to come from a file

I'm thinking that creating a helper to make this less error prone is likely prudent so that this could be implemented en masse across the file, something like this:

env_val() {
    local var="$1"
    local fileVar="${var}_FILE"
    local def="${2:-}"
    if [ "${!var}" -a "${!fileVar}" ]; then
        echo >&2 "error: both $var and $fileVar specified (which are exclusive)"
        exit 1
    fi
    local val
    if [ "${!var}" ]; then
        val="${!var}"
    elif [ "${!fileVar}" ]; then
        val="$(< "${!fileVar}")"
    fi
    echo "${val:-$def}"
}

# ...
: ${WORDPRESS_DB_PASSWORD:=$(env_val 'WORDPRESS_DB_PASSWORD' "$MYSQL_ENV_MYSQL_PASSWORD")}
: ${WORDPRESS_DB_NAME:=$(env_val 'WORDPRESS_DB_NAME' "${MYSQL_ENV_MYSQL_DATABASE:-wordpress}")}
# ...

Thoughts?

Or perhaps instead doing something like this at the start to simply handle the redirection right up front:

for env in \
    WORDPRESS_DB_PASSWORD \
    WORDPRESS_DB_NAME \
; do
    fileEnv="${env}_FILE"
    if [ "${!fileEnv}" ]; then
        if [ "${!env}" ]; then
            echo >&2 "error: both $env and $fileEnv specified (which are exclusive)"
            exit 1
        fi
        val="$(< "${!fileEnv}")"
        eval "export $env=\"\$val\""
    fi
done

cc @yosifkit for thoughts too (since I think whatever we do here will likely be used as a template for how to handle this elsewhere 😄)

@cyli
Copy link

cyli commented Nov 16, 2016

👍 _FILE would definitely be more explicit.

@tianon
Copy link
Member

tianon commented Nov 16, 2016

@mstanleyjones would it be alright with you if @yosifkit and I carry this one from here? 🙏 😇

@mdlinville
Copy link
Contributor Author

Absolutely, feel free to commit into the PR branch or move it to a new PR, whatever works for you.

@tianon
Copy link
Member

tianon commented Nov 17, 2016

Thanks @mstanleyjones! ❤️ ❤️

@yosifkit, I've updated to add a file_env helper (whose usage is simplified slightly further from the example I pasted above to hopefully help cut down on accidental mistakes) 👍

@yosifkit yosifkit merged commit 8ce1e14 into docker-library:master Nov 17, 2016
tianon added a commit to infosiftr/stackbrew that referenced this pull request Nov 17, 2016
- `cassandra`: 3.0.10
- `docker`: 1.13.0-rc1 (currently failing the `override-cmd` test; see moby/moby#28329), `docker daemon` -> `dockerd` (docker-library/docker#31)
- `drupal`: 8.2.3
- `java`: alpine 7.121.2.6.8-r0, 8.111.14-r0
- `openjdk`: alpine 7.121.2.6.8-r0, 8.111.14-r0
- `python`: 3.6.0b3 (docker-library/python#157), pip 9.0.1 (docker-library/python#154)
- `ruby`: 2.3.2, 2.2.6
- `tomcat`: 6.0.48, 7.0.73, 8.0.39, 8.5.8, 9.0.0.M13
- `wordpress`: add `WORDPRESS_*_FILE` support for passing parameters via files especially for [Docker secrets](moby/moby#27794) (docker-library/wordpress#186)
@tianon
Copy link
Member

tianon commented Nov 21, 2016

cc @ltangvald -- think we should add some _FILE support to mysql? 😄 👍

@cyli
Copy link

cyli commented Nov 21, 2016

That would be awesome! A couple others that could also use the update (I think) are postgres, sentry, and mariadb. :) I am not sure if there are others as well.

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.

4 participants