Optionally add Record-Route for inbound, better errors #315
Conversation
| @@ -548,19 +548,19 @@ func (c *inboundCall) runMediaConn(offerData []byte, conf *config.Config, featur | |||
| EnableJitterBuffer: c.s.conf.EnableJitterBuffer, | |||
| }, RoomSampleRate) | |||
| if err != nil { | |||
| return nil, err | |||
| return nil, fmt.Errorf("cannot create media port: %w", err) | |||
There was a problem hiding this comment.
Should we use some error wrapping utility or psrpc.NewError here (and below) instead, to allow extracting the original error?
There was a problem hiding this comment.
That's mostly for debugging for now. I'll need to annotate all errors this way if we want to propagate them.
pkg/sip/inbound.go
Outdated
| @@ -1150,6 +1150,16 @@ func (c *sipInbound) Accept(ctx context.Context, sdpData []byte, headers map[str | |||
| // This will effectively redirect future SIP requests to this server instance (if host address is not LB). | |||
| r.AppendHeader(c.contact) | |||
|
|
|||
| // Other in-dialog requests should be sent to this instance as well. | |||
There was a problem hiding this comment.
I'm sure this isn't the right way to do it. I reviewed the inbound SIP logic, and it looks like it's just imitating the SIP handling used in the existing setup. It needs a major overhaul—something I'd like to do when I find the time.
There was a problem hiding this comment.
Why do you think it's incorrect? Also, this is not for OSS version.
There was a problem hiding this comment.
In-dialog requests are sent to the Contact header if there's no Record-Route (RR) header. So, copying the Contact to the RR doesn't make sense to me. Also, RR handling should be managed by the proxy—doing it on the UA side feels a bit hacky.
There was a problem hiding this comment.
And if the INVITE contains Record-Route headers, they must also be present in the 200 OK response—which I believe is currently handled by the sipgo library automatically. Interfering with this process could break routing.
There was a problem hiding this comment.
Added config flag for this behavior.
@zusrut As I understand it, ideally SIP server should drop Contact header in case it wants all signaling to go through proxy and set Record-Route then.
But for now we want both paths to work - if server is hit through the proxy and if it's hit directly (through LB). Proxy will use Route and non-proxy path will use either Record or Contact. I guess we could drop Contact and always add Record-Route instead to achieve the same result. Do you think that would be better?
There was a problem hiding this comment.
@dennwc The SIP Proxy does not drop the Contact header. Instead, it simply adds a Record-Route header and forwards the request to the UAS. When the UAS needs to send a request within the dialog, it sets the Request-URI based on the received Contact header and Route headers according received RR headers. If no Record-Route headers were present, the request will be sent using the Request-URI derived from the Contact header. If there are Route headers, the request will be sent to the first Route, and the proxy will remove any Route headers that refer to itself before forwarding the request to the next hop or to the Request-URI if no more Route headers are present.
So Contact header is used in every ineraction at the last step and cannot be dropped.
There was a problem hiding this comment.
The SIP Proxy does not drop the Contact header.
I don't mean that the server should drop it. That must not happen.
What I mean is that if we want the signaling to always go through the proxy, LiveKit SIP should not set Contact, but could set Record-Route instead.
dennwc
force-pushed
the
update
branch
2 times, most recently
from
ad100dd to
77b7ccc
Compare
dennwc
changed the title
No description provided.