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

udp-mux: RemoveConnByUfrag removes existing connection when having 1:1 NAT mapping #505

Closed
m1k1o opened this issue Dec 8, 2022 · 5 comments · Fixed by #550
Closed

udp-mux: RemoveConnByUfrag removes existing connection when having 1:1 NAT mapping #505

m1k1o opened this issue Dec 8, 2022 · 5 comments · Fixed by #550
Labels
bug Something isn't working mux UDP and TCP Mux related issues

Comments

@m1k1o
Copy link
Member

m1k1o commented Dec 8, 2022

In recent commit 7f13fd1 closing connection on duplicate candidates has been added.

E.g. we have 2 localAddresses, and extIPMapper as CandidateTypeHost with ipSole. This means, both of our local addresses will be overwritten with a single IP, here:

ice/gather.go

Lines 284 to 291 in ea00dc7

if a.extIPMapper != nil && a.extIPMapper.candidateType == CandidateTypeHost {
if mappedIP, innerErr := a.extIPMapper.findExternalIP(candidateIP.String()); innerErr != nil {
a.log.Warnf("1:1 NAT mapping is enabled but no external IP is found for %s", candidateIP.String())
continue
} else {
candidateIP = mappedIP
}
}

When we continue adding candidate, we check inside for duplicates. It adds first candidate and when processing second candidate, it notices, there is already existing candidate with given addr so attempts to close it.

After closing candidate we do not need, this cleanup task will be executed, that attempts to remove conn by ufrag. But since both connections shared the same ufrag, it removes the first connection instead.

For tcp_mux it is not an issue, because it is identified by local host as well.

I tried to implement the same per-addr separation as there is for tcp_mux, but since udp works differently, i was not able to get reliably correct addr when reading packets. Easier solution would be just to add duplicate check for udp_mux ips.

Your environment.

  • Version: v2.2.12
@r1a2y3
Copy link

r1a2y3 commented Dec 14, 2022

Hi, I haven't try to reproduce your issue. In my opinion, you try to use 2 local address but only one external IP And you want to use udp mux?? Such as local ip (192.168.10.10 / 192.168.10.20, multiplex udp port 3000, they have the same external ip. [128.29.20.30]). Maybe the extIPMapper should specify the relation 192.168.10.10->128.29.20.30, rather than set all ipv4 external ip to 128.29.20.30


By the way, I looked through issues because I found the multiudpmux can't works when the remote use multiudpmux too... Such as two sfu cascaded, but they all use one single udp port.

@m1k1o
Copy link
Member Author

m1k1o commented Dec 14, 2022

Its just about how many local interfaces I have. When inside docker container, those IPs will be 127.0.0.1 and 172.18.0.10 (dynamic, cannot really specify relation) but my real external IP is 192.168.1.10.

This issue is actually generally about wrong handling any UDP duplicate candidates, if the number of them is an even number. This happens to be easily reproducible with NAT 1:1, but its not limited to only this usecase.

Should be as following:

  • 1 canidate: OK
  • 2 duplicate candidates: First added, Second overwrites first one but immediately closes and removes it, so we are left with no canidates.
  • 3 duplicate candidates: After adding and removing one, thrid one is left intact, so OK:

@r1a2y3
Copy link

r1a2y3 commented Dec 15, 2022

Thanks @m1k1o . Got it! Maybe a temporary workaround is to limit your candidates to certain specified range that you want to avoid it. I use the below code to specify candidates now.

        // se
        se := webrtc.SettingEngine{}

        // handle candidates
        nw := vnet.NewNet(nil)
        if len(c.WebRTC.Candidates) > 0 {
                candidates := c.WebRTC.Candidates
                if ifs, err := nw.Interfaces(); err == nil {
                        for _, ifc := range ifs {
                                if addrs, err := ifc.Addrs(); err == nil {
                                        tmpIfc := vnet.NewInterface((net.Interface)(ifc.InterfaceBase))
                                        for _, c := range candidates {
                                                for _, addr := range addrs {
                                                        if strings.HasPrefix(addr.String(), c+"/") {
                                                                tmpIfc.AddAddr(addr)
                                                        }
                                                }
                                        }
                                        *ifc = *tmpIfc
                                }
                        }
                }
        }
        se.SetVNet(nw)

@cnderrauber
Copy link
Member

@m1k1o You can use MultiUDPMux (NewMultiUDPMuxFromPort) to listen on multiple interfaces for udp mux, then each muxed connection is managed by corresponding UDPMux, will not be closed incorrectly

@m1k1o
Copy link
Member Author

m1k1o commented Dec 25, 2022

@cnderrauber yes, that actually fixes my issue. But this makes the change backwards incompatible, although it should still work the same way but only with deprecation warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mux UDP and TCP Mux related issues
Development

Successfully merging a pull request may close this issue.

4 participants