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

Feature request: Proper docker support (and more) #314

Closed
TheRealGramdalf opened this issue Jan 4, 2024 · 18 comments
Closed

Feature request: Proper docker support (and more) #314

TheRealGramdalf opened this issue Jan 4, 2024 · 18 comments

Comments

@TheRealGramdalf
Copy link

I recently published a docker image for 1T that allows it to run headlessly on a server, the source for which can be found here.
Though close, it isn't quite ready for release as a self-hostable app/locally running dockerized application. The following are the main issues that would help make it a smoother experience, both to maintain and use.

Configuration

Most programs that are packaged as docker images allow configuration via three sources: Configuration files, environment variables, and command line options. They also typically follow these conventions:

Note: I realize that you probably already know much of this, but for the sake of completeness, potential future documentation, and just to help think it through, I included a large amount of detail.
Extra note: Much of this may well be biased, and based on my personal opinion; but I believe this to be the unwritten standard that most dockerized programs follow - especially in the case of k8s. I am considering writing up a (entirely optional and unofficial, unless it actually gains traction) standard, an RFC of sorts, to help unify configuration across the many different docker apps. I am happy to help further define and create additional documentation surrounding it, should the idea be pursued.

  • Configuration is by order of priority, where a configuration file is the lowest, environment variables are in the middle, and command line options (--opt) are the highest.
    • Some programs forego this in favour of the mutually exclusive model - if an option is specified twice, the program will exit with an error. While this is better UX for new users, it doesn't allow for various things such as defaults and overrides - a better solution might be to always throw an informational message if a option is overridden.
  • Most configurations follow a tree-like structure, comprising of sections, keys, and values.
    • In each of the three formats (file, cli, env), sections, keys, and values are separated by a delimiter.
      • YAML uses whitespace/indentation as the key delimiter, with a colon (:) to delimit a key and a value. Any other configuration format could technically be used, but YAML is typically the easiest to use and the most human readable.
      • CLI depends on the implementation, Traefik uses a period (.) as the key delimiter, and either a whitespace ( ) or equals (=) to delimit keys from a value
      • Environment variables typically use an underscore (_) as the key delimiter, and the key/value delimiter depends on how the variable is set (though typically an equals sign is used). Environment variables are typically all capitalized, but some implementations support camel, snake, pascal, or mixed case.
    • It is easy to visualize this with yaml - see this Traefik configuration for reference.
    • A section is usually a descriptive name that groups several keys together.
    • A key is usually a descriptive name for a configuration option, which can then be mapped to a value
    • A value is linked to a key, and can be of several types:
      • Boolean
        • A simple true or false. Interpretation depends on the programs implementation, but typical values include the boolean strings true or false, 0 and 1, and in some cases null (key not specified) or specified (i.e. key is present, but the value is null). The last case is easiest to visualize as a command line option - if you specify --debug, the key is specified, but has no value; however the mere presence of --debug implies that it should be true.
        • Note: Some programs allow sections to also act as a key - for example, in Traefik, you can enable HTTP/2 support with http2: true, in which http2 acts as a key - but http2 also has sub options, such as http2.MaxConcurrentStreams: 42. Specifying a sub option implicitly sets http2: true.
      • String
        • An arbitrary collection of literal characters, i.e. value123
          • Note: This can quickly get confusing when managing multiple different layers (i.e. when using a variable in docker-compose, and passing the value of said variable to a shell, which may have different rules for escaping special characters)
      • List
        • A list is typically an unordered collection of values corresponding to a single key
      • Array
        • An array is typically an ordered list, with it's own key: value mapping. Each section is technically an array (each section has multiple key: value mappings), but this can also be useful when referring to user-defined sections - such as users.<USERNAME>.passwordhash: sha256, where the value <USERNAME> is specified by the user rather than the program.
  • Environment variables:
    • For a working example, see the Traefik configuration reference
    • Typically prefixed with an application name, i.e. ONETAGGER_ to prevent collisions with other programs
      • The prefix is often an abbreviation rather than the full name - i.e. 1T instead of ONETAGGER
    • Secret values (such as API tokens) can typically be expanded from a file, by setting the environment variable with an additional __FILE suffix, e.g. 1T_MY_SECRET__FILE = /path/to/file. At runtime, the file is parsed, such that the contents of the file are used instead.
      • The implementation varies from program to program - some are implemented directly in the program itself (the program handles the process of grabbing the variable from the file), whereas some are implemented in an entrypoint script that expands all variables with the __FILE suffix, setting the corresponding environment variable with the contents of the file.
        • Note: I would personally recommend implementing it directly in the program/library (see note below), both to make configuration consistent across platforms (since the entrypoint is typically limited to docker images only), and to make it more secure - if the entrypoint expands environment variables, they are still available via /proc to anything that can read it, even if they are unset post startup. Handling expansion directly ensures that the values can be censored properly, and read permissions restricted to the user running 1T only.

Extra note: it may be worth writing this as a library in rust rather than building it in to 1T directly - there are some other projects (primarily Kanidm) which I could see benefitting from this. Especially if, as I mentioned earlier, I end up writing a standard that apps could follow - having a library that can simply be included into a project (which would make it that much easier for projects that don't want to implement it themselves)
Extra extra note: using a file is typically used by the image builder to specify relevant default options (since the file takes lowest priority) - for example, running as a server (headlessly), and listening on all addresses instead of localhost. Environment variabls are typically where end user configuration occurs, and command line is typically used as enforced configuration (since they can't overwritten by any of the other configurations.)

In the immortal words of larry the cucumber...

... Oofta.

Along with the above system of configuration, the following are a list of options/keys (in no particular order) that aren't available now (to my knowledge), but would greatly simplify packaging 1T as a docker image.

  • Location of all the various files (configuration, logs, and media, which is already implemented)
  • Listen address/ports for the websocket/webserver
  • Allow listening on HTTPS with a self signed certificate
    • Enabling automatic generation of the certificate (or even generating one and using HTTPS by defualt) would be nice, but not necessary - it's a significant upgrade over plain HTTP in terms of security, and would help prevent people deploying this insecurely
  • Better explanation of dependencies
    • This isn't a configuration option, but would be very appreciated so I can (hopefully) use alpine instead of ubuntu for the image base, which would make it much slimmer and overall faster. The dockerfile I currently use ends up with an image roughly ~800 MB in size, of which the 1T binary only takes 30 (ubuntu pulls in a LOT of dependencies with the current apt command).
  • Last but not least, authentication. Even if it's basic, it would be nice to have some form of authentication - ideally direct OpenID connect integration, but since that may be more complicated than it's worth at the moment (for an app intending to be single-user only), HTTP basic auth and HTTP header auth (which could then be plugged into oauth2-proxy for full OpenID integration) would be nice.

These next ones would be nice to have, but aren't absolutely critical

  • Logging target - I'm unsure what the usual is on this one, it might help to take a peek at https://linuxserver.io's images and see if they do something special. Basically where logs end up, i.e. syslog instead of a separate file
  • Remove leading path (i.e. /data/music in the docker image that I linked) from the UI
  • Other options (as relevant) relating to the webserver

Parting thoughts

I'm going to open an issue with Kanidm (a fellow rustacean project) that references this, to see if they would be interested in any of the configuration bit. In that case, collaboration for the library might be a possible option - but of course, it depends entirely on whether or not either project wants to implement this "standard", let alone write a library to implement it. I fully recognize that this project is developed with your own free time, and that if I really want any of this implemented, I should do it myself - you have no obligation to implement any of it.

As I was writing this I also noticed oauth2-proxy's pinned issue... which matches perfectly with my idea of a standard. I'll link them here as well.

Cheers!
~ TheRealGramdalf

@TheRealGramdalf
Copy link
Author

Update in regards to Kanidm: They don't have the development time to write a library, and won't be participating in a standard if such a thing was proposed, for good reason. See the discussion for the full context: kanidm/kanidm#2415

@Marekkon5
Copy link
Owner

Marekkon5 commented Jan 5, 2024

Hello, I appreciate the write up, and understand your points and why to do this. However I will probably disappoint you; OneTagger is intended to be a desktop application. It wasn't meant to be a "self hosted webapp". We just used web technologies to make the development and UI easier to write (like an Electron app).
This is also why the "working directory" and ports are "hardcoded" and not configurable.

Features such as server mode, CLI mode, or exposing the ports were added as an afterthought because someone requested it (or browser compatibility reasons). 1T is (by design) insecure, and shouldn't be exposed to the internet. The HTTP and WS servers were only meant to be used within the app. Therefore we won't add native https support or authentication. You can however use reverse proxy such as nginx to achieve this.

Our primary target is GUI, so the configuration is quite complete mess, and CLI was "hacked in", so in current versions we decided to take the approach of "generate the config in GUI, and use it later in CLI". You can still use CLI overrides for all the configs, however we might deprecate it in future, because there is just too many of them and there is no point in having 50 switches in one command rather than a config file. (Same goes for env variables).

As for Docker size - it is probably caused by the Webkit & GTK dependency. 1T can be modified to run in server mode without depending on those, however as I said - this is a desktop app, not a webserver.

I hope you can understand that, let me know if you have more questions / suggestions. Thanks

@TheRealGramdalf
Copy link
Author

No worries! I totally understand.

The primary reason I want to be able to run this on a server is because it can have direct access to all my music files - sure, I could use an NFS/SMB share, but running it locally (as in with direct access to the filesystem) is just so much faster.

I totally get your point with authentication as well, and I'll see what I can cook up for that externally. This is for sure something that would be targeted at advanced users, and I completely understand why you don't intend on adding this.

As for Docker size - it is probably caused by the Webkit & GTK dependency. 1T can be modified to run in server mode without depending on those, however as I said - this is a desktop app, not a webserver.

I'd be very interested in this process - a brief point in the right direction would be nice.

Our primary target is GUI, so the configuration is quite complete mess, and CLI was "hacked in", so in current versions we decided to take the approach of "generate the config in GUI, and use it later in CLI". You can still use CLI overrides for all the configs, however we might deprecate it in future, because there is just too many of them and there is no point in having 50 switches in one command rather than a config file. (Same goes for env variables).

This is definitely a case of "out of scope for this project", but I would really appreciate keeping environment variables around. Even if it isn't run as a server, there may be some people who can't run it on their native OS - I use NixOS, and there isn't a native package as of yet. The docker image allows me to run it locally, even though there isn't a native package.
Again, I realize that this is very much an edge case, but it is also very handy. The reason I prefer environment variables is that I can use a single docker compose file to manage the base configuration, rather than keeping track of a separate file alongside. I do realize that this strays from the current configuration flow, but it would still be nice to have around for some things such as the path to music etc.

One other thing regarding too many flags etc. - though I could well be wrong, it might actually be easier to maintain given the proper setup. If you were to use the scheme I wrote out above, you could have the possible flags evaluated at runtime rather than hard coding each individual one - have a definition file with all your sections, keys, and values, and 1T will reference this when evaluating configuration (environment variables, cli flags, etc). Rather than adding more logic for each flag, simply adding an option to the definition file will propagate it everywhere else. Unfortunately I don't know enough rust to implement this myself, but I think this would be the ideal way to keep everything in sync.

@Marekkon5
Copy link
Owner

  1. The server mode without GTK - I'll add it to the TODO list, since I realized it would require a bit of restructuring to make it work properly.
  2. Env variables - 1T didn't have any in the first place.
  3. NixOS package - I've noticed that people made AUR and WinGet packages, so I guess anyone can make one for NixOS as well, however we don't wanna maintain all the different repos/packages, since even updating one takes a bunch of effort.
    Also for Linux 1T is shipped as binary, compiled on LTS Ubuntu, which should make it "just work" on most distros if you install the dependencies, so that's prefered.
  4. Flags - they kinda already are parsed at runtime for some of the flags, however the reason why they are like this now is because we use clap for arg parsing, and it does most of the work for us (proper parsing, generating help, simple to use). Making all flags runtime parseable would require kinda rewrite.

@TheRealGramdalf
Copy link
Author

Sounds good!

Env support would be nice to have, but not absolutely necessary.

The NixOS package is definitely going to be a community thing, you don't have to worry about that.

Flags are another nice to have but not necessary. I can work around this if need be.

One final thought - if I were to implement some of these things and contribute, would you be open to merging them? Or are you wanting to keep the scope minimal to reduce maintenance work?

@Marekkon5
Copy link
Owner

We're open to contributors, if the features actually improve the project, and don't create too much needless maintenance. However if you actually plan to contribute, would be great if you split it into smaller chunks rather than "rewriting the entire CLI". Thanks in advance.

@Marekkon5
Copy link
Owner

I'll keep this open as a reminder about the server version, and if you possibly have more questions / suggestions.

@TheRealGramdalf
Copy link
Author

For sure. At the moment I'm just brewing ideas, I'm not trying to start the french revolution ;)

I suppose there is one other thing - if the websocket could listen on the same port that would be nice. It entirely depends on the current webserver setup, but apparently the same process can share a port: https://stackoverflow.com/questions/29497725/can-websockets-and-http-server-both-run-on-the-same-port-number

@Marekkon5
Copy link
Owner

I do know that it's possible, however the situation is complicated. 1T originally only had the WS connection, and HTTP server was added later. On top of that there is another HTTP server on another port just for Spotify authorization.

The entirety of 1T is sync, and because sync is bad for webservers and websocket servers, there aren't many crates that can do both at once (or honestly there isn't a lot of decent sync webservers either (and that's for a good reason)).

I've had some plans to rewrite just the webserver part to use some async webserver, however then I'd have to use one that supports websockets as well and potentially rewrite major parts of the whole system, so I'd say it's work for another day.

@Marekkon5
Copy link
Owner

Marekkon5 commented Jan 7, 2024

Hello, I've restructured the crates and server mode is now possible in the CLI version, which doesn't depend on GTK. According to ldd now the only libaries required are alsa and glibc, which should make the Ubuntu image smaller.

It should technically be possible to also statically link alsa and build for musl target to have fully static binary that works in any container and then use alpine or other smaller images. (However not recommended)

@Marekkon5
Copy link
Owner

In latest commit i've merged the servers together. Now it requires just one port.

@TheRealGramdalf
Copy link
Author

In latest commit i've merged the servers together. Now it requires just one port.

Awesome. Let me get that image built

@TheRealGramdalf
Copy link
Author

TheRealGramdalf commented Jan 9, 2024

Everything should be pretty much good to go, there are just a couple errors:

2024-01-09 19:45:26 [INFO] onetagger_cli: 

Starting OneTagger v1.7.0 Commit: 97469a98 OS: linux


2024-01-09 19:45:26 [WARN] onetagger_ui: Server is exposed to public!
2024-01-09 19:45:26 [INFO] onetagger_ui: Starting web server on: http://0.0.0.0:36913
ALSA lib confmisc.c:855:(parse_card) cannot find card '0'
ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_card_inum returned error: No such file or directory
ALSA lib confmisc.c:422:(snd_func_concat) error evaluating strings
ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_concat returned error: No such file or directory
ALSA lib confmisc.c:1334:(snd_func_refer) error evaluating name
ALSA lib conf.c:5178:(_snd_config_evaluate) function snd_func_refer returned error: No such file or directory
ALSA lib conf.c:5701:(snd_config_expand) Evaluate error: No such file or directory
ALSA lib pcm.c:2664:(snd_pcm_open_noupdate) Unknown PCM default
2024-01-09 19:45:30 [ERROR] onetagger_ui::socket: Failed loading settings, using defaults. No such file or directory (os error 2)
2024-01-09 19:45:30 [ERROR] onetagger_shared: PANIC: panicked at crates/onetagger-player/src/lib.rs:35:79:
called `Result::unwrap()` on an `Err` value: DefaultStreamConfigError(DeviceNotAvailable)
2024-01-09 19:45:30 [ERROR] onetagger_shared: LOCATION: File: crates/onetagger-player/src/lib.rs, Line: 35
2024-01-09 19:45:30 [INFO] onetagger_python: Writing python stdlib for 3.10

I believe everything is fine (the missing config error is because I didn't provide one), except for the sound issue - I assume that's because of the local/remote playback setting, if it's possible to have that configurable through the CLI/environment variables that would be great.

Also here's the full dockerfile - are any of these packages not necessary?

# syntax=docker/dockerfile:1

# This image should be built with: 
# docker build --build-arg=releaseurl='https://url.tld' --tag onetagger:${VER}-{BASEOS} - < Dockerfile
# - $VER = version of onetagger used to build the image
# - $BASEOS = OS of the base image (this file currently uses ubuntu)

FROM ubuntu:22.04
## Server config
EXPOSE 36913/tcp
WORKDIR /data/music
ARG releaseurl='https://github.com/Marekkon5/onetagger/releases/download/1.7.0/OneTagger-linux.tar.gz'

RUN DEBIAN_FRONTEND=noninteractive apt-get update -yq \
    && DEBIAN_FRONTEND=noninteractive apt-get install -yq --no-install-recommends \
    libssl-dev \
    libsndfile1-dev \
    libasound2-dev \
    curl \
    ca-certificates \
    tar \
    && apt-get clean -yq

# Update the CA certificates
RUN update-ca-certificates --fresh


# `chown` is executed in the same RUN directive (layer) to save space
# Stream the zipped output directly to prevent writing intermediate files to disk. zcat (alias for: gzip) is part of coreutils
RUN curl -sL ${releaseurl} | zcat | tar xzvs -C /usr/bin/ \
    && chown 0:0 /usr/bin/onetagger-cli
RUN ln -s ~/.config /config
## Post-download cleanup
# Remove curl
RUN apt-get purge -yq curl

CMD onetagger-cli server --expose --path /data/music

@Marekkon5
Copy link
Owner

The error is from the playback thread (which obviously won't work in docker), and can be safely ignored. However you should enable client side player in settings for playback to work. (NOTE: it transcodes whole file to wav so there is delay when playing).

As for packages: as I said only ALSA is needed now. So libasound2-dev (or probably some non dev version works too, you just need the libasound.so file).

And those apply only to latest dev version: your releaseurl points to latest stable. You can use nightly.link or simillar to get URL to the Actions build.

@TheRealGramdalf
Copy link
Author

Apologies for the late reply, here's an update:

The error is from the playback thread (which obviously won't work in docker), and can be safely ignored. However you should enable client side player in settings for playback to work. (NOTE: it transcodes whole file to wav so there is delay when playing).

Got it. A way to enable this by default via CLI would be nice, but again not necessary

As for packages: as I said only ALSA is needed now. So libasound2-dev (or probably some non dev version works too, you just need the libasound.so file).

Ok. I'll try removing libssl-dev and libsndfile1-dev to see if that breaks anything

And those apply only to latest dev version: your releaseurl points to latest stable. You can use nightly.link or simillar to get URL to the Actions build.

That's my bad - I built it with the correct path passed via --build-arg, so it isn't reflected in the dockerfile. I'll figure out the best way to do that.

Last but not least, the websocket - trying to run it through a reverse proxy (which is therefore exposed to the client on port 443 instead of 36913) fails - to make it configurable I believe it would mean a change to

pub const PORT: usize = 36913;
so that you can override the port from an environment variable/CLI flag, but I'm not familiar enough with rust to do it myself as of yet.
No rush, and many thanks for 1T - it's truly an awesome app.

@Marekkon5
Copy link
Owner

Hello,

  1. Changing the client-side player via CLI: as I said previously: the settings are fully managed by the UI and the backend does not parse them. It would either require a shitty hack which I am against and would require manual updating, or parsing all of the settings which is unnecessary. Also the setting is only relevant in the webui, so imo it's sufficient.
  2. According to ldd both should be unneccesary. I am unsure about libssl-dev however.
  3. Changing port: well as I said previously this was meant to be desktop app / not webserver, so it is hardcoded. In latest commit I've added a fix into webui, so it should detect the port, so if you use docker/reverse proxy, the webui should use the correct port. Maybe in future I'll make this configurable, but I kinda don't have the time for this at the moment.

Thanks

@TheRealGramdalf
Copy link
Author

  1. Got it, no worries. I didn't realize they were that decoupled for some reason, must have misinterpreted something
  2. Seems like libssl isn't needed either, not entirely sure what it's for - it might be required for spotify/musicbrainz etc though, I'm not quite sure
  3. The fix works perfectly, so nothing more should be needed there.

Thanks for your input/fixes

@Marekkon5
Copy link
Owner

  1. I am pretty sure the http client in past needed openssl for TLS/HTTPS, but iirc i switched to rustls, which does not depend on it, and the openssl dep remained in readme from past.

I'll close this for now, feel free to open new issues if you have more suggestions / questions.

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

No branches or pull requests

2 participants