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

Add Docker support and update README #13

Merged
merged 29 commits into from
Aug 14, 2024
Merged

Conversation

perennialtech
Copy link
Contributor

@perennialtech perennialtech commented Aug 13, 2024

This PR introduces containerisation support for inv_sig_helper and improves documentation:

  • Add Dockerfile for building and running the service in a container
  • Create compose.yaml for deployment via Docker Compose
  • Expand README.md with information about features and licensing, as well as instructions for building and running the service

Fixes #3.

@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

It's great, but I have wanted to have inv_sig_helper inside scratch image so that there is nothing that google may exploit from if they really want to.

Something like this:


FROM scratch
COPY --from=builder /app/app .
COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
CMD ["./app"]

@perennialtech
Copy link
Contributor Author

perennialtech commented Aug 13, 2024

It's great, but I have wanted to have inv_sig_helper inside scratch image so that there is nothing that google may exploit from if they really want to.

I'd tried to use a scratch image for the second stage but had issues with OpenSSL dependencies initially.

I've pushed a commit that implements a scratch image, but the trade-off is that the Dockerfile is now more complex, as we need to compile OpenSSL from source for musl.
Just realised that we can skip building OpenSSL for musl if we use an Alpine-based Rust image for the build stage; see 6e195d4

I'd appreciate your thoughts.

@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

It's great but the final goal is to have a multi architecture docker image.

Because invidious does not support only x86.

Dockerfile now detects the architecture being used during the build process and templates in the correct Rust target architecture
@perennialtech
Copy link
Contributor Author

@unixfox Multi-arch builds in 1547376 seem to be working now, with the resulting image loading the correct binary (at least on AMD64).

README.md Show resolved Hide resolved
compose.yaml Outdated Show resolved Hide resolved
@perennialtech perennialtech requested a review from unixfox August 13, 2024 10:28
@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

Great, awesome! We might even be able to add build to quay.io?

You can take example on https://github.com/iv-org/invidious/blob/master/.github/workflows/build-stable-container.yml and I'll add the correct tokens for quay.io login

@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

Would you mind checking if you could lock down the container by removing some capabilities like searxng is doing: https://github.com/searxng/searxng-docker/blob/master/docker-compose.yaml?

I think we will only need NET_BIND_SERVICE but I'm not sure.

And could you set read_only: true in docker-compose? https://superuser.com/a/1635761

Credentials were added for quay.io: https://quay.io/repository/invidious/inv-sig-helper

@perennialtech
Copy link
Contributor Author

@unixfox Added a workflow to build and push a container image to quay.io in 95b602e

Triggers on pushes to master and version tags, with semver used for tagging (e.g., 0.1.2). Also includes a quay.io expiration label like the Invidious workflow

Let me know if any changes are needed

@perennialtech
Copy link
Contributor Author

perennialtech commented Aug 13, 2024

I think we will only need NET_BIND_SERVICE but I'm not sure.

NET_BIND_SERVICE shouldn't be necessary since we're binding to 12999 (so not a port number less than 1024), but can't test at the moment though

Dockerfile Outdated Show resolved Hide resolved
More flexible as the build process will now automatically adapt to whatever architecture the container is being built on, without needing to explicitly list out each supported architecture
@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

@DmitrySandalov
Copy link

Is there a way to include a healthcheck: in a docker-compose.yml?

E.g. some endpoint

    healthcheck:
      test: wget -nv --tries=1 --spider http://127.0.0.1:12999/REPLACEME || exit 1
      interval: 30s
      timeout: 5s
      retries: 2

@techmetx11
Copy link
Collaborator

techmetx11 commented Aug 13, 2024

Is there a way to include a healthcheck: in a docker-compose.yml?

E.g. some endpoint

    healthcheck:
      test: wget -nv --tries=1 --spider http://127.0.0.1:12999/REPLACEME || exit 1
      interval: 30s
      timeout: 5s
      retries: 2

inv_sig_helper doesn't use the HTTP protocol, so just check if the port is still open? (or use one of the opcodes that do nothing but return info like PLAYER_STATUS, and see if the server responds with something)

@unixfox
Copy link
Member

unixfox commented Aug 13, 2024

Is there a way to include a healthcheck: in a docker-compose.yml?

E.g. some endpoint

    healthcheck:
      test: wget -nv --tries=1 --spider http://127.0.0.1:12999/REPLACEME || exit 1
      interval: 30s
      timeout: 5s
      retries: 2

No. It's going to be the development docker compose not the production one. A production docker compose will be published under https://docs.invidious.io

- Run as a non-privileged user within the scratch container
- Add security_opt: - no-new-privileges:true to docker-compose.yml
@perennialtech
Copy link
Contributor Author

Since we're using a scratch image, I don't think healthchecks are possible unless we copy a utility like netcat from an earlier stage

- rust:1.80-alpine replaced with rust:1.80 for the builder stage
- alpine:3.20 replaced with debian:12.6-slim for the user-stage
- Build command simplified to use default target architecture
docker-compose.yaml Outdated Show resolved Hide resolved
…command"

This reverts commit ff9a378.

Reasons for reverting:
1. Compiling via musl is necessary to statically link dependencies and create a truly standalone Rust binary. [1]
2. Alpine-based Rust images are required for the build stage because such systems support dynamic linking, which is also needed for statically-linked binaries. [2]
3. Determining the target architecture and templating the correct value for the --target flag is necessary for the statically-linked binary to be built correctly. [2]

[1]: https://doc.rust-lang.org/1.13.0/book/advanced-linking.html#linux
[2]: rust-lang/rust#40174 (comment)
@perennialtech
Copy link
Contributor Author

perennialtech commented Aug 14, 2024

Not sure how to meaningfully improve the Dockerfile as of 6b94718

The target arch detection isn't overly complex and doesn't hard-code architectures in the build process, and image tests via docker run --platform linux/amd64 and docker run --platform linux/arm64 show the binary being loaded and run correctly

@unixfox unixfox merged commit 4d3cff6 into iv-org:master Aug 14, 2024
@unixfox
Copy link
Member

unixfox commented Aug 14, 2024

Thank you for all the effort @perennialtech!

It's already a very good start. We will see another time if we need to improve the Docker image, for now it works great.

@techmetx11
Copy link
Collaborator

Thanks for your contribution @perennialtech

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.

Create Docker images
4 participants