Fix database container - Githubissues

ctmbl commented 1 year ago

close #94

We should however take care of choosing an updated version of bitnami/mongodb image, 4.4 is pretty old... https://hub.docker.com/r/bitnami/mongodb

:warning: IMPORTANT from bitnami documentation

NOTE: As this is a non-root container, the mounted files and directories must have the proper permissions for the UID 1001.

meaning rw I think (but maybe x too didn't test it) This is part of this debate and I think we should take greater care to use unprivileged users in our images... we should open an issue about that At the moment it only means that when deploying we must ensure that this condition above is true, but a deployment script could take care of this.

ctmbl commented 1 year ago

Personally, I don't see any problem with latest, but if you want to choose a fixed version, let's update to 6.0 at least!

We're not in a big project so this shouldn't be a problem, especially because 6.0 has recently been released, so 7.0 should be far.

On one hand we should keep in mind that latest always points to the more recent tag and a major version update in semantic versioning means that some changes are not backward compatible https://semver.org/ so we're taking the risk to break the system without introducing a bug ourselves. On the other hand latest ensure a better security because we'll update the system almost automatically, and currently 6.0 has been released quite recently and that could come with several bug discovery.

To conclude this is a complicate choice to make, and I personally don't like using latest but for this one time the security-risk/broken-api-risk tip the scales in latest's favour I think! EDIT: I added it in b61bd31 don't hesitate to object if you do not agree finally!

About the permissions, did you manage to test the persistence with this PR?

I manage to... but as I discussed with some of you in private about the blog bot logs the permissions on the host must match the uid used in the container, this is something that must be taken into account either on the host side (by pre-creating the mounted folders and setting them owned by the userid of the container user) or at the container start time (by running the container as the userid that owns the folder on the host) both should work, in either way this will be addressed in another PR by writing a run.sh script I think!

Finally, we definitely need to solve this root-container issue you're right :)

Absolutely let's open an issue!

ctmbl commented 1 year ago

sorry for the force push I add foolishly pushed a wrong commit (you know the revert of #34 to deploy locally with docker while #86 is not merge ahah)

gbrivady commented 1 year ago

Works with the npm running method, commits and discussions/decisions you discutted seems sensible to me, so good to go

Edit : can't approve tho

ctmbl commented 1 year ago

@gbrivady yeah I knew that but I think it's time to change it so I promoted you to the iscsc.fr-maintainers team now your approval will allow this PR to be merged!

However, how did you start the DB if you've tried it with the npm command?

Also one important think you must test is the DB persistence, to properly test it you must also properly setup your ./mongodb folder, don't know if you've already done it but here are the steps (implicitly explained in my first message):

mkdir -p mongodb/prod
mkdir mongodb/dev
chmod -R 664 mongodb
sudo chown -R :1001 mongodb

The goal of these lines is to make the mongodb folder (which will be mounted so shared by the container) owned and writable by the user 1001 in the container!

PS: if you'd like to test it properly with docker, without #86 being merged the best way is to :

git revert 8087def

unluckily you'll have a conflict, to solve it:

in docker-compose.yml keep only the network section change, get rid of everything else, it's about SSL certifcates
other conflicting files aren't code so add them as is we don't matter
```
git revert --continue
```
the you can start the docker
```
docker-compose --env-file .env.production up --build
```
you may have some env var wrongly setup but it's easily solvable

gbrivady commented 1 year ago

@gbrivady yeah I knew that but I think it's time to change it so I promoted you to the iscsc.fr-maintainers team now your approval will allow this PR to be merged!

However, how did you start the DB if you've tried it with the npm command?

Also one important think you must test is the DB persistence, to properly test it you must also properly setup your ./mongodb folder, don't know if you've already done it but here are the steps (implicitly explained in my first message):
mkdir -p mongodb/prod
mkdir mongodb/dev
chmod -R 664 mongodb
sudo chown -R :1001 mongodb
The goal of these lines is to make the mongodb folder (which will be mounted so shared by the container) owned and writable by the user 1001 in the container!

PS: if you'd like to test it properly with docker, without #86 being merged the best way is to :
git revert 8087def
unluckily you'll have a conflict, to solve it:
in docker-compose.yml keep only the network section change, get rid of everything else, it's about SSL certifcates
other conflicting files aren't code so add them as is we don't matter
git revert --continue
the you can start the docker
docker-compose --env-file .env.production up --build
you may have some env var wrongly setup but it's easily solvable

Yeah I might definitely have not checked it correctly, will try again tomorrow

ctmbl commented 1 year ago

@gbrivady sorry for the messy testing process #86 and #95 are changing many things in the dev workflow so things are a bit messy right now, when you'll be testing it don't hesitate to ping me on discord to solve the problems you may have!

gbrivady commented 1 year ago

Add to fix some stuff in my .env, but got the site and DB running according to your comments, so I guess good to go?

ctmbl commented 1 year ago

perfect! tks a lot for testing it!

iScsc / iscsc.fr

Fix database container #95