extensions to lost and http_client modules #2675
Conversation
- new api call including a content-type argument implemented as new function - required to resolve geolocation url using POST (#2641) - Note: not sure if this is the most elegant solution, but it helps to avoid backward compatibility issues
…tion - features: LoST redirect, dynamic HELD url resolving (#2574), LoST NAPTR, POST request to dereference loation - attributes: reponse_time (-1:emergencyDispatch, 0:emergencyRouting, >0[ms]); post_request (POST method to dereference location #2641); recursion (yes/no); location_profile (PIDF/LO profile selection: first/last/geo/civic); verbose (detailed LoST response as log INFO); geoheader_type (filter schema: any/cid/http/https); geoheader_order (first/last) - function: lost_held_dereference (specific function to dereference location using POST method); attributes are url (r), resp.-time (r), resp.-type (r), pidf (r/w) and error (r/w) - general: The extension of the module allows dynamic querying of LIS/HELD and LOST services via NAPTR lookup. In the case of LOST, a redirect response is evaluated. In case a lost_held_request (used to connect to a LIS via POST or GET) is passed with an empty string ("") for the connection parameter, then P-A-I or From header value hostnames are used for NAPTR lookup to get a corresponding service.
|
Please don't create that much duplicated code. Make the old API function call the new as a wrapper with a null argument for content type header. That way we keep backwards compatibility and can add new functions. Please also add new tests to the API test module. |
Hi Olle, i think you are referring to the http_client part of this pull request? Which one in particular, not sure if you know that you can also add comments to individual functions in the review mode. |
Hi Olle, Henning, I think I understood Olle's comment - I am about to implement the suggestion ... |
|
Code duplication was removed for http client module. I guess this can be merged, if no other opinions against. |
|
All good. Thanks for fixing! |
src/modules/lost/functions.c
Outdated
| /* responseTime: emergencyRouting|emergencyDispatch */ | ||
| if((ltime == 0) && (strlen(ptr) > 0)) { | ||
| if(strncasecmp(ptr, HELD_ED, strlen(HELD_ED)) == 0) { | ||
| heldreq = lost_held_post_request(&len, -1, rtype); |
| @@ -0,0 +1,255 @@ | |||
| /* | |||
| * lost module naptr functions | |||
| * thankfully taken over from the enum module | |||
There was a problem hiding this comment.
There are several functions (parse_naptr_regexp, naptr_greater, naptr_sort..) copied from the enum module. We could consider moving them to the core. If we want to keep them, the copyright from the author should be present here as well.
| #define RED_PROP_MSG (const char *)"message" | ||
|
|
||
| #define ERRORS_NODE (const char *)"errors" | ||
|
|
There was a problem hiding this comment.
The section below defines several data structures. In order to prevent overlapping with existing data structures in modules and other libraries you probably want to add a prefix (e.g. lost_list_t, lost_data_t etc..).
|
Moving code to the core from existing module must ensure it can be relocated under BSD license. Regarding the cloning of some str values, they seem to be required due to used functions that require zero-terminated strings, like ipstr that is passed to lost_get_nameinfo(), which is using libc inet_pton(). |
|
Good point @miconda regarding the zero termination, then of course one needs to do a copy. The license topic is of course valid as well, the original author of the enum code is Juha. |
|
@wkampich: lost_copy_string() is not a significant amount of code to replace with pkg_str_dup(), the signature is different as well -- overall, it could be good, but not a must. If you fix the leak, the PR should be ready to merge. |
| ... | ||
| # HELD location dereference | ||
| if ($hdr(Geolocation)=~"^<http.*$") { | ||
| $var(url) = $(hdr(Geolocation){s.select,0,;}); |
There was a problem hiding this comment.
I can suggest use
$var(url) = @hf_value.geolocation[1].uri;
There was a problem hiding this comment.
Note that @... were SER specific variables, still available, but not with an active maintainer. I would rather recommend using $... instead of @..., so better examples with $hdr(...) when gives what's needed.
There was a problem hiding this comment.
@sergey-safarov: you may use the following to dereference multiple header fields:
## response time in ms: 20000, or 'emergencyRouting' or 'emergencyDispatch'
#!substdef "!MY_LOC_TIME!emergencyDispatch!g"
## location type: 'any', 'civic', 'geodetic', or 'civic geodetic'
#!substdef "!MY_LOC_TYPE!any!g"
...
if(is_present_hf("Geolocation")) {
$var(count) = 0;
while($var(count) < $hdrc(Geolocation)) {
$var(geo) = $(hdr(Geolocation)[$var(count)]);
if ($var(geo)=~"^<http.*$") {
$var(url) = $(var(geo){s.select,0,;});
xlog("L_INFO", "### HELD dereferencing location at: $(var(url){s.unbracket})\n");
$var(res) = lost_held_dereference("$(var(url){s.unbracket})", "MY_LOC_TIME", "MY_LOC_TYPE", "$var(pidf)", "$var(err)");
if ($var(res) == 200) {
xlog("L_INFO", "### HELD dereference request returned loc:\n$var(pidf)\n");
} else {
xlog("L_INFO", "### held request failed with $var(err)\n");
}
}
}
}
|
tested this config file During URL dereferencing send this request According to config file |
| <section id="lost.p.post_request"> | ||
| <title><varname>post_request</varname> (int)</title> | ||
| <para> | ||
| Dereferencing the location can be done using either the HTTP GET or POST method. This parameter |
There was a problem hiding this comment.
"This parameter" duplicated here.
|
Tested config works as expected. |
|
tested config Test with one Here may be updated Also in Kamailio console, I can see this error message when Looks as here here "ERROR" level misslead. The script may be started using the command |
|
Use case when the call contains two Example Technically such a call contains Geolocacation by_value and does not require deference first Geolocation header by_reference. From my point of view. Technically on Kamailio maybe used So as idea, in lost request check the number of present Geolocation headers, if present more than one header:
|
@sergey-safarov: basically that is what |
@sergey-safarov: the derefencing function only supports "false" (does not consider the module parameter ... I'll add this info to doc) |
@sergey-safarov: that looks like another http_client API fix as the API call does not support to set a specific header.
@sergey-safarov: I see the same ERROR, but I suspect it is more of a WARNING |
|
Regarding: Maybe there are white spaces at the end of body that fit inside the Content-Length in the sipp scenario (try to print the $mb enclosed in some delimiters like [ ] to see it completely. If proves to be a problem, then an issue can be created for this case with a minimal config to reproduce and the sipp scenario for it. Then related this PR, while not familiar with all its aspects, it seems some of the questions are actually about further improvements (e.g., handling two Geolocation headers). They do not have to be implemented as part of this PR, can be done separately. If no problems need to be solved on this PR, then it should be merged in order to get more chances to be tested before branching 5.5. |
I did check parse_body.c and it seems that part_multipart_headers_cmp logs an ERROR in case it does not find a matching header, which is the case when comparing the first part (application/sdp) of the multipart body (see the example xml). So the error happens any time when comparing sup-parts that do not match. In the example provided the second part contains the |
|
If it is a valid multipart body, with a header that may not exist, then the log message should be DEBUG. You can make the patch if that's the case. |
…om ERR to DBG in case a header does not exist
|
If there are no further comments, I will merge the pull request today (late afternoon). Just a note about "handling more than one |
|
Thank you for your contributions, Wolfgang! |
|
@wkampich Thanks for the update. I think the comments regarding the missing original copyright notice in the natptr.[c,h] files and also the response.h naming are not resolved yet. The copyright notice should be added, I can also do it later if you prefer this.
Several of this typedefs seems to be also not used in your code. But if you think its no big deal for your module, fine. |
|
Hello, @wkampich |
@sergey-safarov: yes, ok for me - just a hint: the problem is not in the multipart header parsing but in the function that searches for specific header values in several body parts - parsing (successfully) happens beforehand. Just add the following to your sipp example (xml) and you get the error (or debug message) twice: |
@henningw: Thanks for the reminder - I'll include your naming and copyright suggestions in a new commit. |
|
@wkampich, @sergey-safarov: the error message from part_multipart_headers_cmp() does not seem related to the code added in this PR, so, even if it is an issue, it is not required to be fixed by this PR. If proves to be a bug, an issue can be opened, or even better, a new PR can be made to fix it. |
|
yes, have checked "ERROR" generation. Then Looks as current |
Ok, thanks for testing. @miconda How shall I proceed? Merge the PR with or without the patch (commit 106b8a6)? |
|
Merge it with the patch, it is only changing the printing of the log message in that case and should be legit that the header is missing, not being mandatory. |
Pre-Submission Checklist
in
doc/subfolder, the README file is autogenerated)Type Of Change
Checklist:
Description
lost: new features, attributes and a new function to dereference location
POST request to dereference loation
post_request (POST method to dereference location lost: resolve geolocation url using POST #2641); recursion (1:yes/0:no);
location_profile (PIDF/LO profile selection: 0:first/1:last/2:geo/3:civic);
verbose 0/1 (detailed LoST response as log INFO);
geoheader_type (filter schema: 0:any/1:cid/2:http/3:https);
geoheader_order (0:first/1:last)
using POST method); attributes are url (r;string), resp.-time (r;string),
resp.-type (r;string), pidf (r/w;string) and error (r/w;string)
and LOST services via NAPTR lookup. In the case of LOST, a redirect
response is evaluated. In case a lost_held_request (used to connect
to a LIS via POST or GET) is passed with an empty string ("") for the
connection parameter, then P-A-I or From header value hostnames are used
for NAPTR lookup to get a corresponding service.
http_client: http_client_request (api) content-type header support
function - required to resolve geolocation url using POST (lost: resolve geolocation url using POST #2641)
to avoid backward compatibility issues