search

pion / webrtc

Pure Go implementation of the WebRTC API
https://pion.ly
MIT License
13.47k stars 1.63k forks source link

Enhance the test using vnet #712

Closed enobufs closed 3 years ago

enobufs commented 5 years ago

This is an umbrella ticket for test enhancement using vnet. @Sean-Der @hugoArregui @ernado - please throw any of your thoughts here.

Overview

Plan

I believe we have two major steps:

Progress

Sean-Der commented 5 years ago

@ernado What should we do about turn? It is going to have dependency on pion/transport to make this all work. IMO we should move it to the Pion repo (we are going to create spaghetti dependencies otherwise)

@enobufs Amazing work on all the docs, especially the Usage Policy I really appreciate that we are thinking about users on this one :) this is going to be a huge step up.

I am ready to start testing everything with vnet in ICE! Another nice thing is we probably get rid of our testing code that simulates packet loss (like E2E) in pion/dtls and use vnet directly instead. The less APIs the better :)

ernado commented 5 years ago

The gortc/turn package does no networking, so we only need to modify turnc.

enobufs commented 5 years ago

I can try to add vnet support to pion/stun, pion/turn and pion/turnc if you do not mind.. Looks like pion/turn borrows pion/stun to handle STUN messages, so I guess we may not need to use vnet in pion/stun..

enobufs commented 5 years ago

@Sean-Der @ernado I kinda realized that I wasn't fully sure what your expectation to pion/turn was when we have @ernado's turn server. Please let me know your thoughts.

  1. merge gortc/turn at some point with pion/turn
  2. we'd keep pion/turn as-is (for test purpose?)
ernado commented 5 years ago

@enobufs We have following repositories:

  1. pion/turn, TURN Server implementation
  2. gortc/gortcd, "complicated" TURN server implementation
  3. gortc/turn, protocol implementation (mostly data structures, constants)

The 3 can be moved under pion, but I'm not sure about naming. We can't just merge both "turns", because the one is server and the other is just protocol.

The 1 can evolve further with simplicity in mind and pion stack (e.g. the logger) The 2 can be developed separately, and I really don't want to drop the uber/zap and other dependencies. I even think about creating the zap turnc version, it is really useful logger.

So everything is messed up.

enobufs commented 5 years ago

I see! Thanks @ernado - now I have better understanding. Looks like pion/turnc already uses gortc/turn. I guess pion/turn could also use it..(?) - because I guess the common ground between pion and gortc seems to be gortc/turn - the protocol portion, and we (as pion developers) would recommend gortc/gortcd for more complicated (real world) usage in return.

For the test enhancement using vnet, keeping and evolving pion/turn seems to be justifiable (hopefully using gortc/turn to minimize our effort and to keep it simple!) @Sean-Der any other thoughts you may have?

Sean-Der commented 5 years ago

@ernado I don't think everything is messed up! We have done more then anyone thought we could get done so far, but we still have a lot more to do :)

I could have made the wrong decision here, but here is my reasoning. All of these things are two-way doors so we can go back if wrong.

Why does pion/turn still exist? Is it useful to have two TURN servers.

It is a library, not an application. I wanted something that I could use my own logger and have custom callbacks for authentication. For example with coturn you have redis/mongo... integration. With pion/turn you can have whatever custom integration you want.

It is embeddable. People are putting it into existing applications. This way for small applications you don't need to run a dedicated TURN server. Also since it is a simple API we can let people use it in other languages

It is useful for our testing. We use it in pion/ice for testing, and it is all fired via go test doesn't require using docker or anything else.

Why not use zap

I have nothing against zap, but why should we choose that over

I want to give the user the ability to choose what ever they want. We should treat all pion code with the same scrutiny that the stdlib has. It would be unexpected if using Go's http brought in a logging library.

Renaming things/getting everything under Pion

Having a single name is really important to the projects success. Most users don't care they just want to use one library and go. If we split things it is going to slow down adoption.

If people feel strongly lets rename pion/turn I just want to move forward. The amount of work that we still have to get done QUIC, mDNS and DTLS just for me is pretty crushing still. I just want everyone to be happy/productive.

thanks @ernado @enobufs!

enobufs commented 5 years ago

@hugoArregui @ernado @Sean-Der Looking at the pion/ice, pion/turnc, pion/stun, in order for us to get started with vnet for ICE testing, what's left are:

Almost there!

I thought I'd neet to convert pion/turnc and pion/stun to vnet, but it looks like all the conns we use are created within pion/ice (correct me if I am wrong)

I have just realized pion/ice uses net.ResolveUDPAddr() with a host name (look-up involved). I will need to add ResolveUDPAddr() to vnet, unless pion/ice switch to use Dial() which has already been supported by vnet including name resolution (internal virtual resolver). Any thoughts? > @hugoArregui

enobufs commented 5 years ago

@Sean-Der - quick question. This line: https://github.com/pion/ice/blob/v0.4.2/agent.go#L237

Wouldn't mDNS mode be disabled by default? (I am going to add a warning, or raise an error if both mDNS and vnet were enabled)

enobufs commented 5 years ago

@Sean-Der Question for you. As I update pion/ice to v0.5.1 (with the new TURN client) for pion/webrtc, I thought I would add config param for vnet also, but I realized NewICEGatherer() method does not take Config struct and adding an argument to it would be a breaking change... :( Should I wait for supporting vnet at webrtc level? (the majority of net ops are done in ice and turn, so things we wanted are mostly covered too...)

Sean-Der commented 3 years ago

@enobufs I wrote my first test using vnet in WebRTC! TestICERestart_Error_Handling

Currently there is nothing in pion/webrtc we are unable to test, so I am going to resolve :) (re-open if you disagree!)

Adding TCP support would be nice, but there are no tests I would write if I had it!