Add option to enable/disable heuristic early-return logic#2133
Conversation
…rning when there is a match with address advertised in the SDP
|
I don't think making this an option is a good solution. You said this is affecting scenarios with ICE - I'm thinking endpoint learning in general should be defused or even disabled altogether when ICE is in use, as ICE should be in charge of determining endpoint address. I was under the impression that this is already done. Let me check. |
|
Have you tried what happens if you just disable endpoint learning? |
Yes. And, using this eventual parameter introduces some help, for people that wants to keep using |
That's odd. ICE should still adjust the endpoint addresses based on the outcome of the negotiations, basically obsoleting the other learning mechanism. Perhaps there's some other underlying issue lurking. Sadly I have no way to reproduce this myself.
I can see it as a feasible solution for a backport, but for future versions going forward I'd rather avoid having it. Determining the endpoint should be delegated to ICE - the regular learning mechanism should be just a fallback. |
@rfuchs, i completely see your point. And its indeed an interesting case. But, that commit is the root cause that switch the behavior between the |
|
Hi @rfuchs, Oh yes, it solves the problem. EDIT: But, introduces a side effect. Now, i have |
|
That's good information, thanks. |
Ok, this is actually unrelated and happens due to f2d57de Will have a fix-up for this as well. |
With ICE in use, we don't want to use the regular endpoint mechanism, and instead let ICE determine the appropriate peer address. This is more secure and also avoids incorrect peer addresses learned from the SDP shortcut logic. Closes #2133 Change-Id: I67e11c06cdda5db26413529c948fcdb8deadc11b
|
Alright, took the opportunity to rework a few other related things that popped up, to hopefully improve the overall situation for good. Everything is pushed to https://github.com/sipwise/rtpengine/tree/rfuchs/2133 for now. Let me know if you can test this and how it goes. |
Thank you @rfuchs, for all the effort here. Tomorrow, i will re-test, and i will give feedback. |
|
Hi @rfuchs, Tested today again, with your most recent commits. The kernel ERR logs have disappeared, but now i got the same behavior as before. |
Sure you're running the code from that branch? Because there is a test for that scenario... If confirmed then post a full debug log, because that means something else is going on. |
Yes, i'm running from your branch. Debug log --> Debug Log 104.x (VPN IP) and 2.x (real IP) |
This seems to be on purpose though? ICE eventually converged on: Candidate So if the priority of that candidate was higher than the two candidates from the offer, this one would be the correct one to use. Sadly the priority isn't being logged. You could see it in the STUN attributes in Wireshark. I'll add the priority to the logs. |
|
Pushed 3c6811f to the branch to log the priority |
|
Where do you see the priority matching the one from the SDP? SDP has priorities 1685987071 and 1685855999. Learned candidate has priority 1853628159, which is higher. This is in line with what the RFC says: the type preference for peer-reflexive (learned) candidates must be higher than the type preference for server-reflexive candidates. Both candidates from the SDP are server-reflexive, so the learned peer-reflexive candidate wins. So based on that I would say that using the candidate 104.28.181.215:59670 is correct. What makes you think that it's the wrong one? |
I see your point. But, this behavior starts only after 13.4 version (using heuristic without early-return, works). I completely see your point, but this could impact in a lot of environments. Maybe my suggestion, could make sense to be backported, for people that relays in the same issue. |
Well, couple points. ICE is very explicit with priorities. Your client offers only two server-reflexive candidates. What your client probably didn't expect is that the effective port from the lower-priority VPN candidate came out different from the one in the SDP, and so that results in a new, higher-priority candidate. But your client will be aware of this, from the STUN response, and will know that there is now a new higher-priority peer-reflexive candidate. Your client could just not nominate this new candidate if it doesn't want to use it, but it does so anyway. Your client is the controlling agent here, and rtpengine isn't even in a position to make any other choice. ICE under RFC 5245 mandates that the nominated candidate pair with the highest priority must be used. Which interface does your client eventually end up using for outgoing RTP? How does it end up sending RTP? ICE under RFC 8445 relaxes this a little bit and does away with aggressive nomination. Under that RFC, only one candidate pair must ever be nominated, but any working candidate pair may be used for media until then. This isn't what your client is doing though. The only thing I can imagine is to allow for slightly misbehaving clients which nominate one pair but end up using another one, but I'm not sure if that's a good solution. |
|
What do you get when, with the current unpatched code base, you set |
|
Hi @rfuchs, Sorry for the delay. Regarding the client part, sure its the controller. But, the Client is the same one between the tests from 13.4 and >=13.5, the only changing piece is RTPEngine, that produces a different behavior using the same environment and flags. Doing captures on the client side, DTLS and STUN packets are the same in both cases. Interface used was I don't want to switch to I'm ok, using the old approach (<= This is a hard topic, and yes hard to get answers. But, my concern is, massive tests and feedback from people were done, when that commit (afce798) was introduced? |
It doesn't matter, because both versions are not behaving correctly. Endpoint learning is largely incompatible with ICE. One version just coincidentally produces a result which you think is what is expected, and the other does not. Neither is actually correct.
That doesn't really tell me anything. You mean it was not sending it via the VPN?
That is not helpful. From the code it looks like it might just do what you need (and tests are confirming this), but now I can't be sure.
You're the first one who has ever reported a problem, and your problem is really unrelated to this commit. I've been trying to get to an actual and proper solution, and your insistence on this particular commit being the problem is also not helpful. |
|
Ok @rfuchs. Yes, it does not send it via Ok, i will try to do the test with Ok, understood. I really appreciate your effort. But, i also can't do major changes on my side, as we have a massive number of clients, and this is a scenario hard to predict the full picture. |
My idea is that for ICE, the media-handover behaviour could implicitly be enabled (with some modifications), as we already guarantee that only verified ICE pairs are able to see media, and so this would allow the remote client to switch to a different pair simply by redirecting its RTP, even if ICE nominated some other pair. |
|
Branch |
2 participants
This PR is related to the following RTPEngine Forum question: (https://groups.google.com/g/rtpengine/c/YsTU99HGd2o).
After 13.5 (including) the following commit was introduced: (afce798).
The idea of this commit is quiet good, but could affect some scenarios, like: when clients are behind some VPN's.
So, the proposal here, is to include a new parameter:
endpoint-learning-heuristic-disable-early-return. This parameter by default assumesfalseas value (so, the current behavior is preserved). But, passingtrueas value, reverts the current behavior and preserves the old logic (<= 13.4).The following PR was already tested locally, and allows the correct turn on/off.
If possible, a backport should be made for versions >= 13.5 (14.x also).