aetaric / checkrr

Checkrr Scans your library files for corrupt media and replace the files via sonarr and radarr
MIT License
202 stars 10 forks source link

[Feature] Setting User:Group and autocreate files as common with docker images #79

Closed quorn23 closed 10 months ago

quorn23 commented 10 months ago

Is your feature request related to a problem? Please describe. With checkrr i need to create the checkrr.db and checkrr.log before the first run and need to mount each file. It's out of ~40 running container the only one required to do so. The ability to either set uid:gid via environment variable, or via user: ${PUID}:${PGID} so the created files have the right owner.

Describe the solution you'd like Change to common practice, so the files (if not found) are being created on start up Being able to just mount the config directory ie

- ${DOCKERCONFDIR}/checkrr:/config and have all 3 files (yaml, db and log) in there.

Being able to set the UID/GID ie

      - PUID=${PUID}
      - PGID=${PGID}
      - TZ=${TZ}

See https://docs.linuxserver.io/general/understanding-puid-and-pgid/

Describe alternatives you've considered Being at least able to use user: ${PUID}:${PGID} as if done so the container currently stops with: time="2023-11-13T20:40:55Z" level=info msg="err: While parsing config: yaml: control characters are not allowed" time="2023-11-13T20:40:55Z" level=fatal msg="Database file path missing or unset, please check your config file." startup=true

Additional context Hotio and Lsio dockerfile examples, with (hopefully) helpful parts highlighted. https://github.com/hotio/base/blob/alpine/linux-amd64.Dockerfile#L42-L46 https://github.com/linuxserver/docker-baseimage-alpine/blob/master/Dockerfile#L84-L95

aetaric commented 10 months ago

Setting defaults and writing the config file is an amazingly bad idea. checkrr's logic looks at if sections of the config file exist to turn features on and off. You could actually just mount a volume at /etc/checkrr with the checkrr.yaml file already there and it will make the db file on it's own if it doesn't exist. That logic already works today.

example without db already existing:

checkrr:
  checkpath: 
    - test/tv
    - test/movies
    - test/music
  database: checkrr.db
  debug: true
  ignorehidden: true
  ignorepaths:
    - '@eadir'
  ignoreexts:
    - 'nfo'
    - '.(null)'
    - 'DS_Store'
    - '_DS_Store'
    - '.srt'
    - '.sfv'
    - '.ass'
  cron: "0 * * * *"
webserver:
  port: 8585
  baseurl: "/"
  ➜  checkrr git:(main) ls checkrr.db
ls: checkrr.db: No such file or directory
  ➜  checkrr git:(main) ./checkrr -c test-noarr.yaml

            _|                               _|
   _|_|_|   _|_|_|       _|_|       _|_|_|   _|  _|     _|  _|_|   _|  _|_|
 _|         _|    _|   _|_|_|_|   _|         _|_|       _|_|       _|_|
 _|         _|    _|   _|         _|         _|  _|     _|         _|
   _|_|_|   _|    _|     _|_|_|     _|_|_|   _|    _|   _|         _|
Checkrr version development
 Commit: 
 Built On: 
 Built By: 
INFO[0000] Using config file: test-noarr.yaml           
[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.

[GIN-debug] [WARNING] Running in "debug" mode. Switch to "release" mode in production.
 - using env:   export GIN_MODE=release
 - using code:  gin.SetMode(gin.ReleaseMode)

INFO[0000] Next Run: 2023-11-20 22:00:00 -0700 MST      
[GIN-debug] GET    /api/files/bad            --> github.com/aetaric/checkrr/webserver.getBadFiles (4 handlers)
[GIN-debug] POST   /api/files/bad            --> github.com/aetaric/checkrr/webserver.deleteBadFiles (4 handlers)
[GIN-debug] GET    /api/stats/current        --> github.com/aetaric/checkrr/webserver.getCurrentStats (4 handlers)
[GIN-debug] GET    /api/stats/historical     --> github.com/aetaric/checkrr/webserver.getHistoricalStats (4 handlers)
[GIN-debug] GET    /api/schedule             --> github.com/aetaric/checkrr/webserver.getSchedule (4 handlers)
[GIN-debug] POST   /api/run                  --> github.com/aetaric/checkrr/webserver.runCheckrr (4 handlers)
[GIN-debug] Listening and serving HTTP on :8585
^C
➜  checkrr git:(main) ls -la checkrr.db
-rw-------  1 aetaric  staff  131072 Nov 20 21:21 checkrr.db

The instructions are somewhat outdated, but 90% of issues come from docker environments, people not reading the config file's comments, or file encoding issues that report things like control character problems.

The issue with UID/GID is, this docker image isn't based on linuxserver.io's work. It's based on the official alpine container, which runs tasks as root.

➜  checkrr git:(main) docker run -it --entrypoint /bin/sh aetaric/checkrr
/ # whoami
root
/ # exit
➜  checkrr git:(main) 

Recent versions of docker now have the --user flag which will set the user and group to a specified user id and group id:

➜  checkrr git:(main) docker run -it --entrypoint /bin/sh --user 501:20 aetaric/checkrr        
/ $ whoami
whoami: unknown uid 501
/ $ id
uid=501 gid=20(dialout) groups=20(dialout)
/ $ exit
quorn23 commented 10 months ago

Setting defaults and writing the config file is an amazingly bad idea. checkrr's logic looks at if sections of the config file exist to turn features on and off. You could actually just mount a volume at /etc/checkrr with the checkrr.yaml file already there and it will make the db file on it's own if it doesn't exist. That logic already works today.

The instructions are somewhat outdated, but 90% of issues come from docker environments, people not reading the config file's comments, or file encoding issues that report things like control character problems.

Fair enough @ mounting /etc/checkrr if this would work with with setting the user and pw. Freshly created container, even with your example config (where i don't understand what's so "amazingly bad" about copying the example yaml you have in the git on start to the directory so the user can edit and rename it to get the app going, but that's another topic) results still, as mentioned in my initial post in:

level=info msg="err: While parsing config: yaml: control characters are not allowed"
level=fatal msg="Database file path missing or unset, please check your config file." startup=true

Removing user: ${PUID}:${PGID} from the compose and the container indeed boots up fine Edit: i just noticed it doesn't create the db file with database: "checkrr.db" in the config yaml and - ${DOCKERCONFDIR}/checkrr:/etc/checkrr in the compose

23/11/2023
22:30:09
            _|                               _|
23/11/2023
22:30:09
   _|_|_|   _|_|_|       _|_|       _|_|_|   _|  _|     _|  _|_|   _|  _|_|
23/11/2023
22:30:09
 _|         _|    _|   _|_|_|_|   _|         _|_|       _|_|       _|_|
23/11/2023
22:30:09
 _|         _|    _|   _|         _|         _|  _|     _|         _|
23/11/2023
22:30:09
   _|_|_|   _|    _|     _|_|_|     _|_|_|   _|    _|   _|         _|
23/11/2023
22:30:09
Checkrr version 3.2.1
23/11/2023
22:30:09
 Commit: a449c42348f4217e8c0241fc8a3f611e6aa3ae73
23/11/2023
22:30:09
 Built On: 2023-11-06T22:46:47Z
23/11/2023
22:30:09
 Built By: goreleaser
23/11/2023
22:30:09
time="2023-11-23T21:30:09Z" level=info msg="Using config file: /etc/checkrr/checkrr.yaml"
23/11/2023
22:30:09
time="2023-11-23T21:30:09Z" level=info msg="Next Run: 2023-11-23 22:00:00 +0000 UTC"
23/11/2023
22:30:09
[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.
23/11/2023
22:30:09
23/11/2023
22:30:09
[GIN-debug] [WARNING] Running in "debug" mode. Switch to "release" mode in production.
23/11/2023
22:30:09
 - using env:   export GIN_MODE=release
23/11/2023
22:30:09
 - using code:  gin.SetMode(gin.ReleaseMode)
23/11/2023
22:30:09
23/11/2023
22:30:09
[GIN-debug] GET    /api/files/bad            --> github.com/aetaric/checkrr/webserver.getBadFiles (4 handlers)
23/11/2023
22:30:09
[GIN-debug] POST   /api/files/bad            --> github.com/aetaric/checkrr/webserver.deleteBadFiles (4 handlers)
23/11/2023
22:30:09
[GIN-debug] GET    /api/stats/current        --> github.com/aetaric/checkrr/webserver.getCurrentStats (4 handlers)
23/11/2023
22:30:09
[GIN-debug] GET    /api/stats/historical     --> github.com/aetaric/checkrr/webserver.getHistoricalStats (4 handlers)
23/11/2023
22:30:09
[GIN-debug] GET    /api/schedule             --> github.com/aetaric/checkrr/webserver.getSchedule (4 handlers)
23/11/2023
22:30:09
[GIN-debug] POST   /api/run                  --> github.com/aetaric/checkrr/webserver.runCheckrr (4 handlers)
23/11/2023
22:30:09
[GIN-debug] Listening and serving HTTP on :8585

The issue with UID/GID is, this docker image isn't based on linuxserver.io's work. It's based on the official alpine container, which runs tasks as root.

Yes, and? The two dockerfiles i posted are also based on vanilla alpine, as it would be an addition to your dockerfile, hence a FR. The build of the imagine is run per root, it then adds the user etc. The app itself has no need to run as root, correct me if i'm wrong.

Anyways, i just wanted to quickly replying to the irretating parts, as the whole answer comes across a little bit passive aggressive and i wanted to clarify parts of it, as this might be just me misjudging the vibe it gives off and in the spirit of open source i prefer a dialog, when possible.

Of course i will respect your not planned fully, but maybe this clears up parts where i didn't express the FR properly. Take care.

aetaric commented 10 months ago

Sorry, I tend to be a little terse in github comments. It was not my intention to give off any passive aggressive vibe.

Regarding a default config file, checkrr isn't a typical app and almost all of the design of the app in centered around the config file enabling and disabling features. A good example is leaving out the webserver or notification blocks. The code inspects the config looking for the section before ever running a bit of code related to that section (e.g. https://github.com/aetaric/checkrr/blob/main/check/checkrr.go#L222-L231, with special attention to line 223). checkrr looks at if the block even exists in the file before continuing with anything further.

This design allows for a very extensive feature set that grows with almost every minor release. Historically with issues on github, discord, and on forums people have had issues with just dropping the full config in place, editing the parts they think they need, and then asking why it crashes when the config sections are defined but empty. The majority of the time I'm the sole person providing support for these situations, by providing the minimal config to the user and instructing them to look at the full possible config example, I've reduced the number of support questions that result in the config file being the issue to near zero. The exceptions being confusion around the mapping for arr services, which I completely understand. That code did my head in to write and I get confused everytime I look at it too. (perhaps that needs a rewrite, but it would be breaking and need a major version)

Regarding Dockerfile changes, It's fairly rare for me to use docker personally. I have 5 containers remaining on my personal docker host. None of them leverage PUID and PGID, but none of them are related to media server type tasks. I personally run checkrr as a binary file, but a good friend of mine runs latest in docker and has been the guy to make sure that docker works on release as he keeps his container updated automatically to whatever is tagged latest and shares the discord notification channel he configured with me so I see if there are issues. I do have a series of test configs I run to make sure the latest build isn't broken thankfully (after a few failed container versions, I've learned to test before pushing a change to the world). I'm not unfamiliar with the PUID and PGID concepts. When I mentioned that a change of the image would be required, that was because those container images are what make PUID and PGID possible. https://github.com/linuxserver/docker-baseimage-alpine/blob/3a67103736985bc871f9ddc196807e7d44478e32/root/etc/s6-overlay/s6-rc.d/init-adduser/run#L4-L8 The images are based on alpine, yes, but they are heavily modified. Both hotio and linuxserver.io make good docker base images, but generally getting the image directly from alpine means checkrr will not be held up by them in the case of a security related issue that requires updating the distro version only.

I hope this provides more insight into why I don't intend to make these changes. I do however, intend to update the readme file in the near future to recommend doing the volume mapping over mapping files, and to update the config examples to point to /etc/checkrr as the location of the database to make it easier on docker users. I will probably throw something in there as well about how to use the --user flag so CLI docker users can make use of user/group permissions if their situation requires it.