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

Export all *_FILE variables (Improve docker secret files usage) #234

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

pastakhov
Copy link

No description provided.

@@ -1029,7 +1029,11 @@ ENV MW_AUTOUPDATE=true \
MW_DB_SERVER=db \
MW_DB_NAME=mediawiki \
MW_DB_USER=root \
MW_DB_PASS_FILE="/run/secrets/db_password /run/secrets/db_root_password" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

here and for MW_DB_INSTALLDB_PASS_FILE the name of the secret suggests that the expectation is that the user is going to be root - I'd prefer to avoid assuming that. Can we have the installdb password at db_install_password instead?

Copy link
Author

Choose a reason for hiding this comment

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

You should probably discuss this with Alexey. If I'm not wrong, he prefers to use the root user with a common hardcoded password everywhere.

I can change it, for example, as MW_DB_INSTALLDB_PASS_FILE="/run/secrets/db_install_password /run/secrets/db_root_password", but I don't believe we will create a user with create privileges to install the MediaWiki database. I agree, in some specific cases, when we use an external database, it probably can be required as not to be embarrassed that this is not actually root password, but even in that case we can write in the docker-compose.yml file: MW_DB_INSTALLDB_PASS_FILE=/run/secrets/db_install_password

I changed everything here and below to be fully compatible with the previous solution. I introduced nothing (except the main feature)

MW_DB_INSTALLDB_USER=root \
MW_DB_INSTALLDB_PASS_FILE=/run/secrets/db_root_password \
MW_ADMIN_USER_FILE=/run/secrets/mw_admin_user \
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the name of the admin account really meant to be a secret? I know it was included in #192 but I don't really agree with that - was there a discussion about this that I missed?

Copy link
Author

Choose a reason for hiding this comment

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

no one forces us to use the docker secret for this, it's just an opportunity.
I don't use it for new wikis. But if I'm not wrong, we use it for some. So, let's don't break them.

&& ln -s $MW_VOLUME/extensions/Widgets/compiled_templates $MW_HOME/extensions/Widgets/compiled_templates
&& ln -s $MW_VOLUME/extensions/Widgets/compiled_templates $MW_HOME/extensions/Widgets/compiled_templates \
# Modify /etc/bash.bashrc (loaded for `docker-compose exec web bash` command)
&& echo '. /etc/profile.d/load-env-vars.sh' >> /etc/bash.bashrc
Copy link
Collaborator

Choose a reason for hiding this comment

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

would adding this to ~/.bashrc here instead of /etc/bash.bashrc fix the issue Alexey pointed out on the task?

Copy link
Author

Choose a reason for hiding this comment

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

No, it doesn't. The difference only that /etc/bash.bashrc is global and runs ~/.bashrc for the current user. I guess we need it for all the users (to be run globally)

MW_ADMIN_PASS_FILE=/run/secrets/mw_admin_password
MW_SENTRY_DSN_FILE=/run/secrets/mw_sentry_dns
```

Note that the WikiTeq team, which maintains Taqasta, also maintains a dedicated
Copy link
Collaborator

Choose a reason for hiding this comment

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

This note about the branch of Canasta is part of the section that started before the discussion of the _FILE env variables - we shouldn't put this new section in the middle

@@ -27,3 +28,36 @@ make_dir_writable() {
')' \
-exec chgrp "$WWW_GROUP" {} \; -exec chmod g=rwX {} \;
}

export_vars_from_docker_secret_files() {
[ -f /etc/environment_secrets ] && rm /etc/environment_secrets
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this line about /etc/environment_secrets meant to be doing, and why? Please document, it isn't obvious from context

Copy link
Author

Choose a reason for hiding this comment

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

documented

Copy link

github-actions bot commented Jan 3, 2025

🐳 The image based on 10dc079b commit has been built with 1.43.0-20250103-234 tag as ghcr.io/wikiteq/taqasta:1.43.0-20250103-234

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.

2 participants