Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Add coturn integration #20

Merged
merged 48 commits into from
Jun 13, 2019
Merged

Add coturn integration #20

merged 48 commits into from
Jun 13, 2019

Conversation

Kirguir
Copy link
Contributor

@Kirguir Kirguir commented Apr 11, 2019

Part of #10.

  1. Create docker-compose with coturn and coturn-redis.
  2. Extend config with coturn_ip, coturn_port, coturn_user, coturn_pass.
  3. Implement authorization.
  4. Pass ice_servers in PeerCreated event.

@alexlapa alexlapa mentioned this pull request Apr 11, 2019
16 tasks
@alexlapa
Copy link
Collaborator

alexlapa commented Apr 11, 2019

@Kirguir ,

Нам нужно иметь два варианта запуска всех связанных приложений:

  1. Локальная разработка: все в хосте, кроме coturn'а с базой.
  2. Удаленный деплой, где все в докере.

Для первого milestone достаточно первого варианта. Roadmap задачи обновил.

@alexlapa alexlapa added feature New feature or request k::deploy Related to deployment capabilities labels Apr 11, 2019
@alexlapa alexlapa added this to the 0.1.0 milestone Apr 11, 2019
@alexlapa alexlapa changed the title WIP: Dockerize application WIP: Add coturn integration Apr 11, 2019
@Kirguir
Copy link
Contributor Author

Kirguir commented Apr 12, 2019

@alexlapa

Pass ice_servers in PeerCreated event.

т.е. надо в конфиг добавить секцию (или в server) с параметрами TURN_IP & TURN_PORT, что бы прокинув их в Room. мы могли передать их клиенту:

"ice_servers": [{
    "urls": [
      "turn:turnserver.com:3478",
      "turn:turnserver.com:3478?transport=tcp"
    ],
    "username": "turn_user",
    "credential": "turn_credential"
  }]

username & credential - это уже к authorization. Их откуда брать?

@alexlapa
Copy link
Collaborator

@Kirguir ,

Итого, по результатам обсуждения:

  1. Авторизиацию через redis пока не делаем.
  2. Статического пользователя coturn'а оставляем.

Значит в конфиг прокидывыаем: coturn_ip, coturn_port, coturn_user, coturn_pass.
В PeerCreated отдаем:

"ice_servers": [
   {
      "urls":[
         "stun:${coturn_ip}:${coturn_port}"
      ]
   },
   {
      "urls":[
         "turn:${coturn_ip}:${coturn_port}",
         "turn:${coturn_ip}:${coturn_port}?transport=tcp"
      ],
      "username":"${coturn_user}",
      "credential":"${coturn_pass}"
   }
]

@alexlapa
Copy link
Collaborator

@Kirguir ,

По результатам обсуждения решили перенести расширение PeerCreated до момента стабилизации #16 .

Kirguir added 6 commits April 15, 2019 10:35
# Conflicts:
#	src/api/client/server.rs
- add docker-compose with COTURN for offline dev
# Conflicts:
#	src/api/client/rpc_connection.rs
#	src/api/protocol.rs
@Kirguir Kirguir requested a review from alexlapa May 14, 2019 15:52
alexlapa added 10 commits May 15, 2019 14:30
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	proto/client-api/src/lib.rs
#	src/api/client/rpc_connection.rs
#	src/signalling/participants.rs
#	src/signalling/room.rs
# Conflicts:
#	config.toml
#	src/conf/mod.rs
@tyranron tyranron force-pushed the master branch 2 times, most recently from bc5cb78 to a29e0a1 Compare May 23, 2019 11:03
@nWacky
Copy link
Contributor

nWacky commented Jun 5, 2019

@alexlapa ,

Подтянул master

Copy link
Collaborator

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Также, хочу отметить, что не компилируется jason.

/// Representation of [`iceServers`] field of [`RTCConfiguration`] dictionary.
#[derive(Clone, Debug, Deserialize, Serialize)]
#[cfg_attr(test, derive(PartialEq))]
pub struct IceServer {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Требуется использовать conditional компиляцию по аналогии с другими структурами.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa,

Я так понял что при feature = "medea", derive(Serialize);
При "jason" - Deserialize


pub mod test {
use super::*;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Тоже удаляйте, зачем пустому модулю висеть?

self.room_id,
self.members.iter().map(|(id, _)| *id).collect(),
)
.map_err(|_| ()),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

То есть из базы Вы credentials'ы удаляете, но у нас (в Member.ice_user) их оставляете? Не надо так.

pub enum ParticipantServiceErr {
TurnServiceErr(TurnServiceErr),
MailBoxErr(MailboxError),
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Прикрутите Fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa,

Я добавил, но я не совсем понимаю где его возвращать
remove_all_users_fut возвращает (), чтобы потом запустить ctx.wait() в Handler в Room

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

В смысле derive Fail. Как тут, например.


#[allow(clippy::module_name_repetitions)]
/// Manages Turn server credentials.
pub trait TurnAuthService: fmt::Debug + Send {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Давайте лучше: create(), delete(), delete_batch().

src/turn/repo.rs Outdated
let client = redis::Client::open(connection_info.clone().into())?;
let connection_manager = RedisConnectionManager::new(client)?;
let mut runtime =
tokio::runtime::Runtime::new().expect("Unable to create a runtime");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

А в каком случае тут ошибка вылетит?

Copy link
Contributor

@nWacky nWacky Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa,

Да, тут будет паника
Добавлю IOError в TurnDatabaseErr и верну его если не получилось запустить runtime

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Так а ошибка тут в каком случае вылетает? Просто, это же у нас инициализация тред-пула, грубо говоря. Достаточно неожиданно, что эта операция может заошибить.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa ,

Я думаю что если кончилась память для хранения этого пула или не удалось зарегистрировать пул в памяти

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

В этом случае можно не предполагать, а просто пройтись по исходникам и узнать наверняка :)

Copy link
Contributor

@nWacky nWacky Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexlapa ,

В исходниках после вызова new я вижу возможные ошибки в tokio-reactor в let io = mio::Poll::new() и io.register => ошибка может произойти если невозможно получить handle памяти

@nWacky nWacky requested a review from alexlapa June 7, 2019 11:10
Copy link
Collaborator

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nWacky ,

Обсудили, добавил тудушки.

@nWacky
Copy link
Contributor

nWacky commented Jun 7, 2019

@alexlapa,

У меня появилась пара вопросов:

  • Мы храним credentials в формате 1:1:medea:pass, coturn может не понять что у нас 3 credentials

  • Какой room_id у static IceUser?

  • В IceUser есть поле name: String, clippy предлагает заменить его на &str, так как это поле не используется внутри IceUser. Я думаю, что нужно так оставить, т.к. мы используем IceUser в результатах Actor => мы не сможем стабильно использовать generic associated types E0658

@alexlapa
Copy link
Collaborator

alexlapa commented Jun 9, 2019

@nWacky ,

Мы храним credentials в формате 1:1:medea:pass, coturn может не понять что у нас 3 credentials

Вы, вероятно, имеете в виду, что мы, при генерации IceUser.name используем такой же разделитель что и при генерации value в redis? Согласен, поменяем на _.

Какой room_id у static IceUser?

У него нет room_id. В текущей реализации, static IceUser прокидывается через конфиг Coturn'а. Соответственно, нам его не нужно его сохранять в redis'е.

В IceUser есть поле name: String, clippy предлагает заменить его на &str, так как это поле не используется внутри IceUser. Я думаю, что нужно так оставить, т.к. мы используем IceUser в результатах Actor => мы не сможем стабильно использовать generic associated types E0658

Clippy говорит не о поле структуры IceUser, а о параметре функции new(). И, в общем-то, правильно говорит.

@alexlapa alexlapa requested a review from tyranron June 9, 2019 19:30
@@ -86,6 +86,7 @@ impl EventHandler for InnerRoom {
_peer_id: u64,
_sdp_offer: Option<String>,
_tracks: Vec<Track>,
_ice_servers: Vec<IceServer>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ is preferred to user over _name as allows compiler to opt-out those at all, while _name just silences warnings.

config.toml Outdated Show resolved Hide resolved
src/conf/mod.rs Show resolved Hide resolved
@tyranron
Copy link
Member

FCM

Integrate Coturn as ICE server (#20, #10)

- provide ICE servers to use for WebRTC connections on client side
- impl static Coturn user authorization scheme
- impl dynamic authorization scheme backed by Redis database of Coturn users

@nWacky nWacky merged commit beda425 into master Jun 13, 2019
@nWacky nWacky deleted the dockerize branch June 13, 2019 10:47
@tyranron tyranron changed the title WIP: Add coturn integration Add coturn integration Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature or request k::deploy Related to deployment capabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants