Skip to content

Support for reading the default password from a file (i.e. docker secret) #143

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 24, 2017
Merged

Support for reading the default password from a file (i.e. docker secret) #143

merged 3 commits into from
Mar 24, 2017

Conversation

vovimayhem
Copy link
Contributor

@vovimayhem vovimayhem commented Feb 23, 2017 •
edited
Loading

What does this PR do?

Supports configuring the default password via a docker secret (#141):

  • Optionally reads the contents of the file configured using the RABBITMQ_DEFAULT_PASS_FILE to set up the RABBITMQ_DEFAULT_PASS env var

@vovimayhem
Copy link
Contributor Author

I blatantly copied the shell function used in the entrypoint from the Wordpress entrypoint... :(

@vovimayhem
Copy link
Contributor Author

vovimayhem commented Feb 23, 2017 •
edited
Loading

@yosifkit, @tianon your thoughts?

yosifkit
yosifkit reviewed Feb 23, 2017
View reviewed changes
Copy link
Member

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall sounds like a good plan. We should probably accept RABBITMQ_DEFAULT_USER_FILE too as suggested in #141 (comment). Any other environment variables we should accept in files?

debian/docker-entrypoint.sh Outdated
unset "$fileVar"
}

file_env 'RABBITMQ_DEFAULT_PASS'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be after line 33 (exec gosu rabbitmq "$BASH_SOURCE" "$@") otherwise it could run twice and get grumpy that the variable env is set and the file env is set (line 13).

Copy link
Member

@yosifkit yosifkit Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch the upset part, there is an unset "$fileVar". So maybe it doesn't need to move?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather see this line down closer to where it's actually read, like right after all the lists of config keys and defaults, but right before the loop which sets haveConfig=....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I should move the calls to the file_env function down right before haveConfig=, I'm getting this right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that'd be my preference. Alternatively, we could simply re-use the arrays we've got and add this for all variables, but then we'll technically support several XYZ_FILE_FILE variables, which is a tad on the nutty side (so maybe later we can consider doing it for all vars which don't match *_FILE already, but that's definitely something I'm fine waiting on until someone asks for more variables to be supported). 😄 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather 'whitelist' the variables we want to read from a file - we wouldn't want to read TLS certs files into variables...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...actually that's the case on several of my app containers that use RabbitMQ with TLS support

@vovimayhem
Copy link
Contributor Author

I can't think of any other right now... There are a lot of env vars, but none requires the "read from docker secret" functionality IMHO

@vovimayhem vovimayhem force-pushed the story/default_pass_file branch from 588926d to 6a95e08 Compare March 1, 2017 19:04
vovimayhem
vovimayhem commented Mar 1, 2017
View reviewed changes
alpine/docker-entrypoint.sh Outdated
@@ -23,9 +23,6 @@ file_env() {
unset "$fileVar"
}

file_env 'RABBITMQ_DEFAULT_USER'
Copy link
Contributor Author

@vovimayhem vovimayhem Mar 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved these down below to a new loop...

vovimayhem
vovimayhem commented Mar 1, 2017
View reviewed changes
alpine/docker-entrypoint.sh
@@ -44,6 +41,12 @@ fi
: "${RABBITMQ_MANAGEMENT_SSL_CERTFILE:=$RABBITMQ_SSL_CERTFILE}"
: "${RABBITMQ_MANAGEMENT_SSL_KEYFILE:=$RABBITMQ_SSL_KEYFILE}"

# Allowed env vars that will be read from mounted files (i.e. Docker Secrets):
fileEnvKeys=(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this new list for keys that will be read from files (i.e. docker secrets)

vovimayhem
vovimayhem commented Mar 1, 2017
View reviewed changes
alpine/docker-entrypoint.sh
@@ -86,6 +89,7 @@ declare -A configDefaults=(
haveConfig=
haveSslConfig=
haveManagementSslConfig=
for fileEnvKey in "${fileEnvKeys[@]}"; do file_env "RABBITMQ_${fileEnvKey^^}"; done
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the list from files into vars

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tianon @yosifkit Is this better?

@vovimayhem
Copy link
Contributor Author

We're waiting on a RabbitMQ release, do we?

@yosifkit
Copy link
Member

😕 I am not sure what happened on this. It all seems good to me. Ping @tianon

@tianon tianon merged commit 0c14528 into docker-library:master Mar 24, 2017
@tianon
Copy link
Member

tianon commented Mar 24, 2017

👍

tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 29, 2017
@tianon
Update docker-library images
e8e6e16 
- `docker`: 17.03.1-ce
- `haproxy`: 1.7.4
- `kibana`: 5.3.0
- `logstash`: 5.3.0
- `mongo`: simplify entrypoint for 3.4 (docker-library/mongo#156)
- `mysql`: fetch `--socket` during initdb (docker-library/mysql#266)
- `percona`: `5.7.17-12-1.jessie`, `5.6.35-81.0-1.jessie`, `5.5.54-rel38.7-1.jessie`
- `rabbitmq`: `RABBITMQ_DEFAULT_USER_FILE` & `RABBITMQ_DEFAULT_PASS_FILE` support esp. for Docker secrets (docker-library/rabbitmq#143)
- `rocket.chat`: 0.54.2
@tianon tianon mentioned this pull request Mar 29, 2017
Merged
tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 30, 2017
@tianon
Update docker-library images
38f8b4e 
- `docker`: 17.03.1-ce
- `haproxy`: 1.7.4
- `kibana`: 5.3.0
- `logstash`: 5.3.0
- `mongo`: simplify entrypoint for 3.4 (docker-library/mongo#156)
- `mysql`: fetch `--socket` during initdb (docker-library/mysql#266)
- `percona`: `5.7.17-12-1.jessie`, `5.6.35-81.0-1.jessie`, `5.5.54-rel38.7-1.jessie`
- `rabbitmq`: `RABBITMQ_DEFAULT_USER_FILE` & `RABBITMQ_DEFAULT_PASS_FILE` support esp. for Docker secrets (docker-library/rabbitmq#143)
- `rocket.chat`: 0.54.2
@tianon tianon mentioned this pull request Apr 10, 2017
Closed
@letientai299 letientai299 mentioned this pull request Nov 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tianon tianon tianon left review comments

@yosifkit yosifkit yosifkit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vovimayhem @yosifkit @tianon