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

Destination Port Inferred Incorrectly When Assigning $du #700

Closed
colinmorelli opened this issue Jul 7, 2016 · 7 comments
Closed

Destination Port Inferred Incorrectly When Assigning $du #700

colinmorelli opened this issue Jul 7, 2016 · 7 comments

Comments

@colinmorelli
Copy link

When manually assigning $du, the destination port pseudo-var $dp is set to 5060 unless explicitly overridden in the URI.

This should probably be inferred from the transport param, as it yields an incorrect value when transport=tls (which, unless explicitly overridden, should default to port 5061).

@oej
Copy link
Member

oej commented Jul 8, 2016

If the $du is referring to a SRV record, chances are that 5060 is wrong too as the SRV may have any port.

@miconda
Copy link
Member

miconda commented Jul 8, 2016

It is only about the returned value of the variable $dp. The port is not set in the outbound proxy uri.

The logic is, if the $du has no port value, then return 5060 -- probably some old code relying on default value for port -- relevant piece of code inside the pv module:

    } else if(param->pvn.u.isname.name.n==2) /* port */ {
        if(uri.port.s==NULL)
            return pv_get_5060(msg, param, res);
        return pv_get_strintval(msg, param, res, &uri.port, (int)uri.port_no);

miconda added a commit that referenced this issue Jul 8, 2016
@miconda
Copy link
Member

miconda commented Jul 8, 2016

So I pushed a patch to return 5061 for tls, thinking that the target was to have a valid value if one tries to use $dp to build a new URI.

On the other hand, I think it may be better to make it return $null if port is not set in the uri.

@oej
Copy link
Member

oej commented Jul 8, 2016

I agree, returning $NULL if the port is not set is an improvement.

@miconda
Copy link
Member

miconda commented Jul 8, 2016

Or maybe adding a new variable like $dup (and $rup given it is the same for $rp), which will return based on uri value and $dp/$rp stay as expected destination port... just ideas...

@colinmorelli
Copy link
Author

Great point @oej - I hadn't considered the case of SRV records which may change that.

@miconda I could go for something like that. We could also just be very explicit about it, and rather than create a pseudo-var for something that may be wrong for reasons @oej mentioned, maybe just provide a function in core, like default_port_for_transport("tls"). This way it's very clear that it is not "based" off of the URI, but rather it just provides an opportunity to access the default port that a transport will use. Then, my case would be satisfied by default_port_for_transport("$dP"); I have no idea if this is a better/worse solution, but I do like that it avoids any implication that it's actually the destination port for the request.

If going the $dp (or a similar pseudo var) route, it'd probably help to also toss some help text on the pseudovariables page. Something like:

In cases where the destination URI does not contain a port number, $dp is only a guess at the destination port that will be used given the transport selected. Other factors (SRV record lookups, - maybe others?) could result in the request being sent to a different port which won't be known until Kamailio attempts to send the request.

@miconda
Copy link
Member

miconda commented Jul 12, 2016

It may be that {uri.port} transformation return empty string or 0 when the port is not in an uri, like:

$(du{uri.port})

For documentation, feel free to add the content you think it's helpful in the wiki portal where the variables are documented.

miconda added a commit that referenced this issue Sep 8, 2016
- reported by Colin Morelli, GH #700

(cherry picked from commit 0421bf5)
@miconda miconda closed this as completed Sep 12, 2016
miconda added a commit that referenced this issue Jun 13, 2017
- reported by Colin Morelli, GH #700

(cherry picked from commit 0421bf5)
(cherry picked from commit f7e0b25)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants