Skip to content

Commit

Permalink
Add STUN support in Janus config (#1579)
Browse files Browse the repository at this point in the history
Part of #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
#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](tiny-pilot/tinypilotkvm.com#939 (comment)).
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](#1578) where Janus
would fail to come up right after booting, if a STUN server is
specified. We’ll fix this separately.


<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1579"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
  • Loading branch information
jotaen4tinypilot authored Aug 23, 2023
1 parent e305440 commit edb4441
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 3 deletions.
4 changes: 1 addition & 3 deletions debian-pkg/debian/tinypilot.postinst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions debian-pkg/etc/sudoers.d/tinypilot
Original file line number Diff line number Diff line change
@@ -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
Expand Down
43 changes: 43 additions & 0 deletions debian-pkg/opt/tinypilot-privileged/scripts/configure-janus
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 -%}

0 comments on commit edb4441

Please sign in to comment.