From edb44415699c1a35925933026e9d86a26f032104 Mon Sep 17 00:00:00 2001 From: jotaen4tinypilot <83721279+jotaen4tinypilot@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:24:18 +0200 Subject: [PATCH] Add STUN support in Janus config (#1579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of https://github.com/tiny-pilot/tinypilot/issues/1460. This PR templatizes the Janus main config file, and adds a privileged script for (re-)generating the config from the template plus from the properties in `settings.yml` (i.e., `ustreamer_janus_stun_server` and `ustreamer_janus_stun_port`). ## Notes - In the Debian `postinst` routine, we deliberately delegate to a privileged script instead of just inlining the logic ([like we do for writing the app settings](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/debian-pkg/debian/tinypilot.postinst#L73-L80), for example), since we want to re-use the `configure-janus` logic in the backend in a later step. - Later, when calling the `configure-janus` privileged script from the backend, we also need to restart the uStreamer and Janus systemd services manually. To me, it felt safer and more explicit to let the caller take care of that, instead of making the service restart part of `configure-janus`. I also wasn’t sure we should be issuing direct calls to `systemctl` or `/usr/sbin/service` anyways during the Debian install procedure, instead of using `deb-systemd-invoke`. - I debated whether we should add a comment above the `nat {}` block in the Janus config, however, I wasn’t sure whether a generic explanation of what STUN is would add enough value. - Since neither `ustreamer_janus_stun_server` nor `ustreamer_janus_stun_port` do have default values, it doesn’t make sense to list them [in `settings.py`](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/app/update/settings.py#L24-L31). We currently don’t have an authoritative place anymore that lists and documents all supported properties in `settings.yml`, so the two newly added STUN properties are implicit and undocumented now. I have opened https://github.com/tiny-pilot/tinypilot/issues/1573 for tracking that, since I think this is a broader issue. A few more things on testing: - I have tested this PR quite a bit on device with the [latest bundle build off `af5ec0d`](https://output.circle-artifacts.com/output/job/29dc16fa-0b35-4b41-b920-85e61fa8883f/artifacts/0/bundler/dist/tinypilot-community-20230822T1618Z-1.9.0-66+af5ec0d.tgz), so if you like, I’d suggest that a brief spot-check QA on your end would suffice. - Please note that testing this PR is only about verifying that we generate a valid Janus config [according to the documentation](https://github.com/meetecho/janus-gateway/blob/7c7093e1885834ac4154244bd99f8dd93ae71170/conf/janus.jcfg.sample.in#L259-L288), not about actually verifying the STUN mechanism itself, as the setup for that [is quite complex and laborious](https://github.com/tiny-pilot/tinypilotkvm.com/issues/939#issuecomment-1592775400). We already gathered enough evidence that Janus with STUN in itself does the job, and solves the use-case scenarios we are after. - We [have an issue](https://github.com/tiny-pilot/tinypilot/issues/1578) where Janus would fail to come up right after booting, if a STUN server is specified. We’ll fix this separately. Review on CodeApprove --- debian-pkg/debian/tinypilot.postinst | 4 +- debian-pkg/etc/sudoers.d/tinypilot | 1 + .../scripts/configure-janus | 43 +++++++++++++++++++ .../{janus.jcfg => templates/janus.jcfg.j2} | 7 +++ 4 files changed, 52 insertions(+), 3 deletions(-) create mode 100755 debian-pkg/opt/tinypilot-privileged/scripts/configure-janus rename debian-pkg/usr/share/tinypilot/{janus.jcfg => templates/janus.jcfg.j2} (87%) diff --git a/debian-pkg/debian/tinypilot.postinst b/debian-pkg/debian/tinypilot.postinst index ec7cad77c..d5157eca5 100755 --- a/debian-pkg/debian/tinypilot.postinst +++ b/debian-pkg/debian/tinypilot.postinst @@ -131,9 +131,7 @@ else fi # Restore the Janus config files. -cp \ - /usr/share/tinypilot/janus.jcfg \ - /etc/janus/janus.jcfg +/opt/tinypilot-privileged/scripts/configure-janus cp \ /usr/share/tinypilot/janus.transport.websockets.jcfg \ /etc/janus/janus.transport.websockets.jcfg diff --git a/debian-pkg/etc/sudoers.d/tinypilot b/debian-pkg/etc/sudoers.d/tinypilot index 4a6102ae0..1afd13d3c 100644 --- a/debian-pkg/etc/sudoers.d/tinypilot +++ b/debian-pkg/etc/sudoers.d/tinypilot @@ -1,5 +1,6 @@ tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/change-hostname tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/collect-debug-logs +tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/configure-janus tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/read-update-log tinypilot ALL=(ALL) NOPASSWD: /opt/tinypilot-privileged/scripts/update tinypilot ALL=(ALL) NOPASSWD: /sbin/shutdown diff --git a/debian-pkg/opt/tinypilot-privileged/scripts/configure-janus b/debian-pkg/opt/tinypilot-privileged/scripts/configure-janus new file mode 100755 index 000000000..5773f5cb4 --- /dev/null +++ b/debian-pkg/opt/tinypilot-privileged/scripts/configure-janus @@ -0,0 +1,43 @@ +#!/bin/bash +# +# Writes the main Janus config file. The config file is based on the config +# template, and it depends on the values found in /home/tinypilot/settings.yml. +# Note that you have to restart both the Janus and uStreamer systemd services +# after running this script, in order to effectuate the config changes. + +# Exit on unset variable. +set -u + +# Exit on first error. +set -e + +print_help() { + cat << EOF +Usage: ${0##*/} [--help] +(Re-)writes the Janus configuration, based on the settings.yml file. + --help Display this help and exit. +EOF +} + +# Parse command-line arguments. +while [[ "$#" -gt 0 ]]; do + case "$1" in + --help) + print_help + exit + ;; + *) + >&2 print_help + exit 1 + esac +done + +# Generate and write config file from template. +pushd /opt/tinypilot +. venv/bin/activate && \ + PYTHONPATH=/opt/tinypilot/app \ + ./scripts/render-template \ + < /usr/share/tinypilot/templates/janus.jcfg.j2 \ + > /etc/janus/janus.jcfg && \ + deactivate +popd diff --git a/debian-pkg/usr/share/tinypilot/janus.jcfg b/debian-pkg/usr/share/tinypilot/templates/janus.jcfg.j2 similarity index 87% rename from debian-pkg/usr/share/tinypilot/janus.jcfg rename to debian-pkg/usr/share/tinypilot/templates/janus.jcfg.j2 index 53c12236d..fdd0d66e8 100644 --- a/debian-pkg/usr/share/tinypilot/janus.jcfg +++ b/debian-pkg/usr/share/tinypilot/templates/janus.jcfg.j2 @@ -18,3 +18,10 @@ transports: { loggers: { disable = "libjanus_jsonlog.so" } + +{% if janus_stun_server and janus_stun_port -%} +nat: { + stun_server = "{{ janus_stun_server }}" + stun_port = {{ janus_stun_port }} +} +{% endif -%}