Parisson / TimeSide

scalable audio processing framework and server written in Python
https://timeside.ircam.fr/docs/
GNU Affero General Public License v3.0
369 stars 59 forks source link

Fix #180 managing permissions #182

Closed Tointoin closed 4 years ago

Tointoin commented 4 years ago

Add uid and gid options to the celery command.

Ownership of celery and app logs and of media are set to www-data in app_base.sh so that for both worker and app have access to them whichever one is up first.

Any Django admin command (python3 $manage) before app start is run with this uid:gid to avoid creation of var/log/app/timeside_debug.log file with the wrong rights.

It has been tested from scratch.

Tointoin commented 4 years ago

Please remove / resolve conflicting blocks

sorry I forgot to... save file after managing conflicts 😓

Tointoin commented 4 years ago

I really cannot apply this one:

1. the user running uwsgi in production is already www-data

2. in development, it is common that the regular user runs the django commands, nobody else

3. Fix permissions like this automatically on log dirs has proved to be a headache to manage, they should be done manually case by case.

Problem is without those changes, a first "up" from scratch would raise permission errors on /var/log/app/timeside_debug.log as we experienced yesterday.

gnuletik commented 4 years ago
  1. the user running uwsgi in production is already www-data

Yes, but as @Tointoin said, worker.sh may create /var/log/app/timeside_debug.log with root permission if the manage.py commands are not prefixed with su. This would raise an error from the app container.

  1. in development, it is common that the regular user runs the django commands, nobody else

Yes, if a developer runs a docker-compose exec python manage.py shell, it will be run as root. Then, the developer may run commands that create files with wrong permissions. In this case, this is the user's responsibility to fix the permissions of create file.

We can also work around that by creating a script to launch a manage.py command with the appropriate user. However, I'm not found of this solution as it adds complexity where this issue can be solved at a lower level (see below).

  1. Fix permissions like this automatically on log dirs has proved to be a headache to manage, they should be done manually case by case.

I agree that this solution is cumbersome. However, this PR follows the previous behavior of the app.sh script that is fixing the media permissions (at this line https://github.com/Parisson/TimeSide/pull/182/files#diff-18f0e1496972e9e1474ffdd1b08fc78dR37).

A cleaner way to handle those permissions issues would be to :

However, this solution requires additional time. But if you have time to go with it, that would be way better IMO. I don't see any other fast / simple solution to fix the current issue than this PR. If you have any other idea @yomguy, I'd be glad to hear it.

yomguy commented 4 years ago

After our live discussion, we agreed that the solution proposed by Martin and Antoine is the best solution at this stage although we should find a better one later for managing each django command execution.

gnuletik commented 4 years ago

@yomguy thanks for the feedback! Why not fix the issues from my review (trailing whitespaces and worker args) in the PR before merging ? The PR is from the fix/permission branch so everyone with write access to the repo can edit it. This would allows more review :)