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

Fix passing of database credentials to containers. #366

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Conversation

mgruner
Copy link
Collaborator

@mgruner mgruner commented Aug 23, 2023

The current state has serious issues. Database env vars from .env don't get passed to the containers, and thus it will only work if the defaults are used - they are also assumed by docker-entrypoint.sh. If you attempt to change database username, host etc. it will fail to even install Zammad.

This update corrects that and makes sure all containers that potentially execute Zammad / Rails code get all required variables. With this, commands can be executed like: docker-compose run --rm zammad-railsserver rails r 'pp Setting.count'.

MrGeneration
MrGeneration previously approved these changes Aug 23, 2023
Copy link
Member

@MrGeneration MrGeneration left a comment

Choose a reason for hiding this comment

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

LGTM

@MrGeneration
Copy link
Member

MrGeneration commented Aug 23, 2023

So I've tested it and even though the environments are now available as required, there's one environment variable that is missing: DATABASE_URL which rails and rake seem to be looking for (if they can't find it they'll fallback to config/database.yml.

The only way I can get that variable set in a "quick" way by entering the container with docker exec -ti zammad-docker-compose-zammad-railsserver-1 bash followed by source /docker-entrypoint.sh which is suboptimal. One could add .bashrc with

ESCAPED_POSTGRESQL_PASS=$(echo "$POSTGRESQL_PASS" | sed -e 's/[\/&]/\\&/g')
export DATABASE_URL="postgres://${POSTGRESQL_USER}:${ESCAPED_POSTGRESQL_PASS}@${POSTGRESQL_HOST}:${POSTGRESQL_PORT}/${POSTGRESQL_DB}"

for a dirty workaround to get rid of the second step. However: The disadvantage on this approach clearly is that you always have to call bash and from there call rails.

Directly calling rails does not work (yet). I'm still looking for a solution on that.


Docker by default uses sh which also by default doesn't invoke as login shell. For this reason one can't use /etc/profile or /etc/profile.d/someenv.sh.

A very dirty workaround I found on stackoverflow was to overwrite sh with bash which I personally don't like too much.

@mgruner
Copy link
Collaborator Author

mgruner commented Aug 23, 2023

@MrGeneration thanks a lot for the testing. I'm aware of this limitation. That's why I suggested to use docker-compose run ... instead. This will go through the entrypoint, and thus provide the DATABASE_URL.

Alternatively, docker exec ... /opt/zammad/contrib/docker-entrypoint.sh rails ... can be used to achieve the same environment from the outside. Just pass the command to call to the entrypoint, and it will invoke that. This is made possible by a recent contribution to docker-entrypoint.sh.

We'd have to make sure this is clearly documented. What do you think?

@MrGeneration
Copy link
Member

My versions are

Docker version 24.0.5, build ced0996
Docker Compose version v2.20.2

I dug deeper and noticed that the .env suggestion right now is tag version 66 which is fairly old (we're at 119 right now). Suddendly it works. The image version is very important and should be bumped together with this PR to reduce confusions for others just like I had.

With this the entrypoint variant with exec also works as expected:

root@dockertest:/opt/zammad-docker-compose# docker exec -ti zammad-docker-compose-zammad-railsserver-1 /docker-entrypoint.sh rails r "pp Setting.count"
I, [2023-08-23T18:53:15.098150 #32]  INFO -- : ActionCable is using the redis instance at redis://zammad-redis:6379.
I, [2023-08-23T18:53:15.114664#32-5380]  INFO -- : Using memcached as Rails cache store.
I, [2023-08-23T18:53:15.114785#32-5380]  INFO -- : Using the Redis back end for Zammad's web socket session store.
I, [2023-08-23T18:53:16.487167#32-5380]  INFO -- : Setting.set('models_searchable', ["Chat::Session", "KnowledgeBase::Answer::Translation", "Organization", "Ticket", "User"])
251

Sorry for the confusion on my end here.


We'd have to make sure this is clearly documented. What do you think?

Definitely should be documented. It's not straight forward.

We also could implement some kind of wrapper like we have in package installations zammad run .... Just as an idea for lazy people like me. That variant is still shorter than the entrypoint variant. But that's just me possibly.


Btw I also build a local container to test my bash variant by defining bash to be the shell to be in the container.
That variant however was not solving my above points so I discarded it.

@42web-ch
Copy link

oh! A big thanks, now its works for me. I'm a new Zammad User, great Work.

@mgruner mgruner merged commit 7f31a9a into master Aug 24, 2023
4 checks passed
@mgruner mgruner deleted the fix-container-env branch August 24, 2023 07:41
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