The setting "keep_proxy_rr" will add the Record-Route headers added by the proxy to the route_set stored in the dialog. When in use, sending locally generated in-dialog requests will loop back to the proxy with a proper Record-Route header, including any parameters.
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally on 5.1, compile tested on master - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1832
-- Commit Summary --
* dialog: Add setting to loop BYE through proxy
-- File Changes --
M src/modules/dialog/dialog.c (9) M src/modules/dialog/dlg_handlers.c (29) M src/modules/dialog/dlg_handlers.h (2) M src/modules/dialog/dlg_hash.c (82) M src/modules/dialog/dlg_hash.h (9) M src/modules/dialog/doc/dialog_admin.xml (36)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1832.patch https://github.com/kamailio/kamailio/pull/1832.diff
@gaaf Thank you, can you add a bit here about the targeted use case for this functionality?
@gaaf - I see that dlg_update_rr_set() is used only for DLG_CALLER_LEG, no need to do it for CALLEE as well?
Having an option to update Record-Route sets within dialog can be useful, I think at this moment they are stored only during the call setup.
On the other hand, for what you try to achieve, I wonder if it would not be simpler to use somehow the caller/callee sockets fields, they are already stored inside the dialog record and they should overlap in address, port and transport with local record-route. So based on modparam, the local requests are sent to the caller/callee socket, with top record route built at storage time or at send time (store vs cpu question here, like none that relevant to decide for one or the other option) -- this just as an idea, I haven't really analyzed if it is a simpler solution, I just thought that those addresses are already stored.
@henningw: In the current situation, the local Record-Route headers are stripped before they are stored in the dialog module. If a request is sent by the dialog module (forced BYE for example) it completely bypasses the routing script as it will be sent to the remote parties directly. With this PR the local Record-Route headers will not be stripped so the request will be looped back to the proxy as the first Route header is now the proxy itself. This causes the request to follow the routing script as if it was sent by one of the parties in the call and all processing that would normally be done for an "external" request would also be done for this request.
@miconda: - The callee is handled only when the dialog is confirmed. As there is no previous RR set, no update is needed. The caller's leg is preliminary set when the request is handled, but the final RR set is only known when the dialog gets confirmed (dependent on the winning branch), hence the update is needed. - Changing the RR in-dialog is not the intention of this PR. - The sockets don't store (record-)route parameters so this would not be enough. As alternative, maybe using local_route is possible, but it still needs access to RR parameters to be complete. As this patchset predates the local_route functionality I have not investigated much into that direction.
In short, this PR makes the dialog module to not strip the local record-route headers, something I have always considered to be a bug.
Thank you for the explanation, understand it now. I will have a brief look to the patch and comment there if I spot something.
henningw commented on this pull request.
From the code change all looks fine to me. I've added some comments inline about a few things I noticed.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s) shm_free(dlg->tag[leg].s); - dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
Why you decrease here the shm malloc'ed area, do you change something in the tag as well?
@@ -269,6 +271,25 @@ int populate_leg_info( struct dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
This free is in current git master at the end of this function and seems redundant now. Maybe it make sense to combine it with the other free in line 290 and move it to the end of the function?
/* skip_recs contains the number of RR's for the callee */
+ skip_recs -= own_rr; + /* Add local RR's to caller's routeset */ + if( print_rr_body(msg->record_route, &rr_set, DLG_CALLER_LEG, + &skip_recs) != 0) { + LM_ERR("failed to print route records \n"); + goto error0; + } + LM_DBG("updating caller route_set %.*s\n", + rr_set.len, rr_set.s); + if (dlg_update_rr_set( dlg, DLG_CALLER_LEG, &rr_set)!=0) { + LM_ERR("dlg_update_rr_set failed\n"); + if (rr_set.s) pkg_free(rr_set.s); + goto error0; + } + if (rr_set.s) pkg_free(rr_set.s);
see above
@gaaf This is apparently still open, I would like to resolve this for the upcoming 5.3 to actually merge it. I've had a minor few remarks/questions to the code, would be great if you can have a look to that.
gaaf commented on this pull request.
@@ -269,6 +271,25 @@ int populate_leg_info( struct dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
print_rr_body() allocates the memory for rrset.s, it is up to the caller to free it. As it may be allocated twice, it must be freed twice and that must be done before the next call to print_rr_body(). Each free is in the same scope as the call to print_rr_body(). I fail to see how these can be combined into one free.
gaaf commented on this pull request.
/* skip_recs contains the number of RR's for the callee */
+ skip_recs -= own_rr; + /* Add local RR's to caller's routeset */ + if( print_rr_body(msg->record_route, &rr_set, DLG_CALLER_LEG, + &skip_recs) != 0) { + LM_ERR("failed to print route records \n"); + goto error0; + } + LM_DBG("updating caller route_set %.*s\n", + rr_set.len, rr_set.s); + if (dlg_update_rr_set( dlg, DLG_CALLER_LEG, &rr_set)!=0) { + LM_ERR("dlg_update_rr_set failed\n"); + if (rr_set.s) pkg_free(rr_set.s); + goto error0; + } + if (rr_set.s) pkg_free(rr_set.s);
see above
gaaf commented on this pull request.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s) shm_free(dlg->tag[leg].s); - dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
It is not necessarily decreased, it is just realloc'ed to the size of the new tag, which may be bigger or smaller.
The tag is not "changed" it is stored untouched from the incoming response.
There may be multiple to-tag's in a single dialog as the call may have been forked. The dialog module does not track individual branches separately, it just stores 1 of them, we just hope it is the correct one. When storing the tag of the 200 OK, it may be overwriting a previous tag (from a 1XX response).
henningw commented on this pull request.
@@ -528,7 +533,7 @@ int dlg_set_leg_info(struct dlg_cell *dlg, str* tag, str *rr, str *contact,
if(dlg->tag[leg].s) shm_free(dlg->tag[leg].s); - dlg->tag[leg].s = (char*)shm_malloc( tag->len + rr->len );
Thank you, understood.
henningw commented on this pull request.
@@ -269,6 +271,25 @@ int populate_leg_info( struct dlg_cell *dlg, struct sip_msg *msg,
if (rr_set.s) pkg_free(rr_set.s);
Thanks for the clarification
Merged #1832 into master.
Thank you!
-
Alex Hermann -
Daniel-Constantin Mierla
-
Henning Westerholt