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

rr: add enable_double_rr_always option #257

Merged
merged 1 commit into from Jul 30, 2015
Merged

rr: add enable_double_rr_always option #257

merged 1 commit into from Jul 30, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jul 20, 2015

This patch helps us to handle inadequatness of Linphone mobile apps (both iOS and Android flavours are affected) without reinventing a lot of logics to handle each transport (we use TCP, TLS, WS, WSS).
E.g. if we have two TLS-based Linphone clients, even with enable_double_rr for gentle cross-forwarding we would get single RR header without transport= attribute, resulting in Linphone sending ACK and BYE via UDP.
This new option, being enabled, results in two RR headers being always inserted, with transport attribute being always added. I believe this cannot harm the installation with enable_double_rr, just makes instructions more explicit.
The Linphone's behaviour looks like a set of different bugs, but it would get us much more efforts to fix.
I don't know if any other useragent needs such workaround. So I would perfectly understand if this commit wouldn't be accepted upstream.

Also this commit currently lacks README doc rebuild, because I don't know how to do that. Only XML doc is updated.

@miconda
Copy link
Member

miconda commented Jul 20, 2015

For clarification enable_double_rr_always still requires that enable_double_rr is set, right?

I wonder that instead of adding a new parameter won't be better to consider a new value for enable_double_rr, like:

  • 0 - double rr disabled
  • 1 - conditional double based on different sockets for incoming and outgoing (what is now)
  • 2 - always double rr (what is added by this patch)

@ghost
Copy link
Author

ghost commented Jul 20, 2015

For clarification enable_double_rr_always still requires that enable_double_rr is set, right?

Yes. This decreases number of extended codelines (no need to change lines with "if (enable_double_rr)").

I agree with your proposition to use new value of enable_double_rr. I will rework the patch soon.

@ghost
Copy link
Author

ghost commented Jul 22, 2015

Updated code as agreed, also removed accidentally included unrelated debug code.
Please not that there's still no update to plain-text README, just doc xml is updated. Feel free to amend this commit for this.

@ghost
Copy link
Author

ghost commented Jul 24, 2015

Please wait before merging this... Retesting...

@ghost
Copy link
Author

ghost commented Jul 24, 2015

It turned out that simple record_route() with enable_double_rr=1 (which is default) solves likely all the issues. We have met our problems because we used record_route_preset() initially, then moved to record_route_advertised_address(), but record_route_advertised_address() (with enable_double_rr=1) doesn't append transport attribute when the call is from TLS to WS (record_route() does). So for now, we need either to use this patch, or to investigate why this happens, or to investigate why record_route doesn't pick advertise_address (it really doesn't for tcp and tls connections; for websocket-based it does). Or to use record_route() and just live with Record-Route headers containing internal IP of Kamailio server (it seems to work anyway).

@miconda
Copy link
Member

miconda commented Jul 29, 2015

I think worth merging the patch as it is an option anyhow -- if not enabled, it doesn't change anything -- who knows when it can come handy with some UAs.

@ghost
Copy link
Author

ghost commented Jul 29, 2015

Ok. Thank you.

miconda added a commit that referenced this pull request Jul 30, 2015
rr: add enable double rr always option
@miconda miconda merged commit 9fd57a9 into kamailio:master Jul 30, 2015
@ghost
Copy link
Author

ghost commented Aug 3, 2015

Damn, sorry for unrelated change in tcp_read.c in my commit. Thank you for fixing that.

@africallshop
Copy link

Hello,
I have a similar problem (ack problem)
Here is the call flow :

linphone (tls) --> (tls) kamailio (udp) --> (udp) asterisk

I set up kamailio like as :

modparam("rr", "enable_full_lr", 1)
modparam("rr", "append_fromtag", 1)
modparam("rr", "enable_double_rr", 2)

kamailio -v
version: kamailio 4.4.0-pre0 (x86_64/linux) f39d14
flags: STATS: Off, USE_TCP, USE_TLS, USE_SCTP, TLS_HOOKS, USE_RAW_SOCKS, DISABLE_NAGLE, USE_MCAST, DNS_IP_HACK, SHM_MEM, SHM_MMAP, PKG_MALLOC, Q_MALLOC, F_MALLOC, TLSF_MALLOC, DBG_SR_MEMORY, USE_FUTEX, FAST_LOCK-ADAPTIVE_WAIT, USE_DNS_CACHE, USE_DNS_FAILOVER, USE_NAPTR, USE_DST_BLACKLIST, HAVE_RESOLV_RES
ADAPTIVE_WAIT_LOOPS=1024, MAX_RECV_BUFFER_SIZE 262144, MAX_LISTEN 16, MAX_URI_SIZE 1024, BUF_SIZE 65535, DEFAULT PKG_SIZE 8MB
poll method support: poll, epoll_lt, epoll_et, sigio_rt, select.
id: f39d14
compiled on 13:25:30 Feb 12 2016 with gcc 4.9.1

But I have still the problem.
Any help ? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants