WebRTC support with Coturn #16
Conversation
| name: mozilla-hubs | ||
| ipam: |
There was a problem hiding this comment.
Today I learned that ipam stands for IP address management.
keianhzo
force-pushed
the
coturn-support
branch
from
61631bc
to
9829c0b
Compare
There was a problem hiding this comment.
I left some questions and suggestions. Could you also please remove the .DS_Store file? (You might consider adding it to your global gitignore.)
| networks: | ||
| default: | ||
| hubs_network: |
There was a problem hiding this comment.
The default applies the custom network (mozilla-hubs) to all the services. When you remove default you then have to specify the network for each service. That makes sense when you are divvying up the services between multiple networks, but we aren’t doing that. What’s the benefit in ditching the default here?
There was a problem hiding this comment.
IIRC it was required to assigning a static ip address.
| @@ -9,6 +31,10 @@ services: | |||
| user: postgres | |||
| volumes: | |||
| - pgdata:/var/lib/postgresql/data | |||
| ports: | |||
| - "5432:5432" | |||
There was a problem hiding this comment.
This is a smell. Why do we need to expose the database to the host OS?
There was a problem hiding this comment.
Coturn needs to access the database. Is there another better way of inter container communication?
There was a problem hiding this comment.
Containers on the same network can reach one another. The ports configuration maps the container port to the port on the host OS. So in this case, that’s HOST:CONTAINER which maps port 5432 from the container to the same port on the host OS. Will software on your local machine need to access the database directly?
| # This should be the same as MEDIASOUP_MIN_PORT and MEDIASOUP_MAX_PORT | ||
| # You might neeed to increase this number depending on how many client you are connecting. | ||
| # (Keep the ports range small to improve container statup time) | ||
| - "40000-40050:40000-40050" |
There was a problem hiding this comment.
Do these ports need to be exposed to the host OS? If so, does the user need to accept proxy certificates in the browser for them?
There was a problem hiding this comment.
This are used for Coturn <-> Dialog communication but again this is the only way I found to communicate two containers. Open to hear alternatives.
There was a problem hiding this comment.
The ports config shouldn’t affect communication between two services on the same network. They are able to communicate directly without the intervention of the host OS.
| volumes: | ||
| - dialog:/code | ||
| working_dir: /code | ||
| networks: | ||
| hubs_network: | ||
| ipv4_address: 10.20.30.12 |
There was a problem hiding this comment.
Why is it necessary to fix the address to a static IP?
There was a problem hiding this comment.
You need to listen in a specific IP address, you can't listen in all interfaces. Is there a better way to assign the current container IP without making it static?
There was a problem hiding this comment.
I don’t follow. Does it need to be the same between orchestrations? Who is listening?
There was a problem hiding this comment.
If we just need to know the current container IP, then # hostname -i should do the trick.
dockerfiles/coturn.Dockerfile
Outdated
| COPY files/coturn/certs/key.pem /certs/key.pem | ||
| COPY files/coturn/certs/cert.pem /certs/cert.pem | ||
| COPY files/coturn/entrypoint.sh /entrypoint.sh | ||
| RUN chmod +x /entrypoint.sh |
There was a problem hiding this comment.
If you chmod the file in the repo, then you don’t have to in the dockerfile.
There was a problem hiding this comment.
Can you please add a newline to the end of the file?
There was a problem hiding this comment.
Are permissions kepts accross different OSs?
There was a problem hiding this comment.
Affirmative. The file permission comes from the build context.
docker-compose.yml
Outdated
| build: | ||
| context: . | ||
| dockerfile: dockerfiles/coturn.Dockerfile | ||
| entrypoint: /bin/sh /entrypoint.sh |
There was a problem hiding this comment.
What would you think about moving this line directly into the dockerfile?
There was a problem hiding this comment.
Oh yep, I see that you have been moving those. What the benefit of doing that? image reusability?
There was a problem hiding this comment.
It makes the compose file lighter and more focused on orchestration (rather than containerization). If that makes sense. 😄
| coturn: | ||
| environment: | ||
| REALM: ret | ||
| PSQL: host=db dbname=ret_dev user=postgres password=postgres options='-c search_path=coturn' connect_timeout=30 |
There was a problem hiding this comment.
Does this need access to Reticulum’s database? If not, it should get its own and probably its own host too since the version of Postgres Reticulum needs may differ from that of coturn.
|
What feature(s) am I able to exercise with this addition that I was unable to without it? |
|
When I run this branch locally I get errors about missing tables. How are the necessary tables meant to be created? |
Why
We need to test WebRTC comms in the containerized version of the Hubs infra but the current config doesn't support it.
What
This PR assigns a public IP and opens ports for RTC comms in the dialog config for hhost to host communication. It also adds a new container for the TURN server for tcp/udp relayed communication.