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

JVB cont-init OCTO check is broken #1787

Closed
spijet opened this issue Apr 22, 2024 · 12 comments
Closed

JVB cont-init OCTO check is broken #1787

spijet opened this issue Apr 22, 2024 · 12 comments

Comments

@spijet
Copy link
Contributor

spijet commented Apr 22, 2024

Hello everyone!

While poking around my k8s Jitsi Meet testbed with OCTO enabled, I stumbled upon a piece of information mentioned here and there that OCTO doesn't work without Colibri WebSockets and so JVB should fail to start in an environment where OCTO is enabled and Colibri WebSocket support is disabled. This snippet from JVB's etc/cont-init.d/10-config seems to imply just that:

if [[ (-z $ENABLE_COLIBRI_WEBSOCKET || $ENABLE_COLIBRI_WEBSOCKET == "0") && $ENABLE_OCTO == "1" ]]; then
echo "ERROR: In order to enable Octo relays (with ENABLE_OCTO=1), you MUST enable Colibri websockets (with ENABLE_COLIBRI_WEBSOCKET=1)";
exit 1;
fi

However, this check didn't fire in my test installation (with SCTP and no WebSockets), because the environment variables are a mix of true/false and 1/0:

root@jitsi-test-jitsi-meet-jvb-8998996fb-2prtw:/# env | sort | grep -P 'OCTO|SCTP|WEBS'
ENABLE_COLIBRI_WEBSOCKET=false
ENABLE_OCTO=1
ENABLE_SCTP=true
ENABLE_XMPP_WEBSOCKET=false
JVB_OCTO_BIND_ADDRESS=X.X.X.X
JVB_OCTO_BIND_PORT=4096
JVB_OCTO_PUBLIC_ADDRESS=10.42.Y.Y
JVB_OCTO_REGION=all
TESTING_OCTO_PROBABILITY=1

So initially I wanted to file an issue about this mishap. But then I stumbled upon this JVB documentation page that says:

It uses and requires either SCTP or Colibri WebSockets for the bridge-bridge connections.

And sure enough, jitsi/jitsi-videobridge#2001 seems to add explicit support for OCTO over SCTP transport. So the issue boils down to two points:

  1. The cont-init.d/10-config check should probably check that any of (ENABLE_COLIBRI_WEBSOCKET, ENABLE_SCTP) is true;
  2. Maybe it should honor true/false as well, not just 1/0.

As a maintainer of Jitsi Meet Helm Chart, I'm especially curious about the second point, because depending on your answer I might have to make sure that all relevant environment variables that this chart uses stick to either 1 or 0, with no true/false/True/False/etc. :)

@spijet
Copy link
Contributor Author

spijet commented Apr 23, 2024

Here's how they check for true/false in Bitnami images' entrypoint scripts:

#!/usr/bin/env bash

# <...>

########################
# Check if the provided argument is a boolean or is the string 'yes/true'
# Arguments:
#   $1 - Value to check
# Returns:
#   Boolean
#########################
is_boolean_yes() {
    local -r bool="${1:-}"
    # comparison is performed without regard to the case of alphabetic characters
    shopt -s nocasematch
    if [[ "$bool" = 1 || "$bool" =~ ^(yes|true)$ ]]; then
        true
    else
        false
    fi
}

And then it's as simple as:

#!/usr/bin/env bash

# Check if variable is true:
if is_boolean_yes "${ENABLE_SOMETHING}"; then
  echo "Yay, something's enabled!"
else
  echo "Something's disabled, boo! :("
fi

# Check if variable is false:
if ! is_boolean_yes "${ENABLE_SOMETHING}"; then
  echo "Haha, it's disabled!"
else
  echo "Noo, disable it quick!!!"
fi

@saghul
Copy link
Member

saghul commented Apr 23, 2024

We should just remove the check altogether. In the template we should set the default values for SCTP based on the colibri WS ones. If one is not set use the other one.

Are you up for sending a PR?

@spijet
Copy link
Contributor Author

spijet commented Apr 23, 2024

What if the user somehow disables both SCTP and WebSockets? This had actually been the case for the Chart for a while, and I would bang my head against the wall wondering why it doesn't work. :D

I'm certainly up for a PR, but I'm much more confident about fixing the check in the script than about fixing the configuration template. :)

@saghul
Copy link
Member

saghul commented Apr 23, 2024

If both are disabled via env vars the template should use SCTP, since it will work out of the box unlike WS which requires web-server config.

@spijet
Copy link
Contributor Author

spijet commented Apr 24, 2024

Hmm. Ok then, I'll see what I can do and will send you a PR. :)

@saghul
Copy link
Member

saghul commented Apr 24, 2024

Thank you!

@spijet
Copy link
Contributor Author

spijet commented Apr 24, 2024

By the way, is it safe/reasonable to try to enable both SCTP and WebSockets at the same time? Does it have any benefits over using just one option (e.g. client app falling back to SCTP if WS session breaks)?

@damencho
Copy link
Member

Nope, there is no such fallback. And if sctp fails, this means that and media to the bridge will fail.

@spijet
Copy link
Contributor Author

spijet commented Apr 24, 2024

So basically there's no point in enabling both, right?

ADD: I see that Jicofo has an option to additionally disable OCTO over SCTP when SCTP transport is enabled, so it looks like having both WS and SCTP enabled is a valid scenario as well? Also, I see no references to SCTP in jvb.conf, only in jicofo.conf. 🤔

@damencho
Copy link
Member

You can have both enabled as there is and this client config: https://github.com/jitsi/jitsi-meet/blob/aa04692e9b31ec0711b4226ba2c4dc620ea2edb0/config.js#L77

@spijet
Copy link
Contributor Author

spijet commented Apr 24, 2024

Ok then. For now it looks like there are no config template modifications needed, except for maybe changing the default value of ENABLE_SCTP to 1 in jicofo.conf. :)

@spijet
Copy link
Contributor Author

spijet commented Apr 24, 2024

See the changes I came up with in #1791. :)

@saghul saghul closed this as completed in 76ffaa7 May 7, 2024
le-firehawk pushed a commit to le-firehawk/docker-jitsi-meet that referenced this issue Jun 6, 2024
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

3 participants