feat(sip): Add from_host field to SIP outbound trunk#1419
feat(sip): Add from_host field to SIP outbound trunk#1419civilcoder55 wants to merge 4 commits intolivekit:mainfrom
Conversation
* Add from_host field to SIPOutboundTrunkInfo and SIPOutboundConfig
* Add validation for from_host to ensure it's a domain/IP, not a SIP URI
* Update SIPOutboundTrunkUpdate to support from_host changes
* Use from_host as hostname in CreateSIPParticipantRequest, fallback to own hostname if not set
🦋 Changeset detectedLatest commit: 780a486 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
dennwc
left a comment
dennwc
left a comment
There was a problem hiding this comment.
Thank you for implementing it! The change looks good, but I'm blocking it for now. Please see the reason below.
rpc/sip.go
Outdated
| DestinationCountry: destinationCountry, | ||
| Address: hostname, | ||
| Hostname: ownHostname, | ||
| Hostname: fromHost, |
There was a problem hiding this comment.
Note that this change will affect LiveKit Cloud. For now, we do not want users to be able to override the domain in the From header.
It totally makes sense for OSS, though. So I think we'll need to pass an extra parameter like allowCustomHostname to this function. If it is false, fromHost must have no effect.
There was a problem hiding this comment.
updated
now, the allowCustomHostname option has been added to the SIP server configuration
livekit/sip#594
| DestinationCountry: destinationCountry, | ||
| Address: hostname, | ||
| Hostname: ownHostname, | ||
| FromHostname: fromHost, |
There was a problem hiding this comment.
I think we should not introduce a new field here. Lets just keep the Hostname here and pass allowCustomFrom (or whatever we want to call it) to the NewCreateSIPParticipantRequest function. This will intentionally break compatibility and we'll have to pass something to it in Cloud. This will ensure it cannot silently change the behavior.
There was a problem hiding this comment.
@dennwc
The challenge is that I can’t read the SIP server configuration when executing NewCreateSIPParticipantRequest because it is being called from LiveKit, not from SIP.
What do you suggest?
There was a problem hiding this comment.
That's okay, OSS can always pass true here.
| // Hostname for the 'From' SIP address in INVITE | ||
| string hostname = 20; | ||
| // custom Hostname for the 'From' SIP address in INVITE | ||
| string from_hostname = 34; |
There was a problem hiding this comment.
I might be missing something here, but how it's different from the hostname above?
There was a problem hiding this comment.
@dennwc
The hostname value is always set to ownHostname, which is passed as an empty string from livekit-server and defaults later to the SIP server Host value
There was a problem hiding this comment.
Lets change it in one of those places then. We shouldn't introduce another field in the protocol for this to work. We already have a field, just need to use it differently.
2 participants
This option lets users define a custom From-header hostname when setting up an outbound trunk, allowing SIP messages to have a hostname in the From header that differs from the server’s default.
This resolves livekit/sip#578
Before
{ "trunk": { "name": "Normal outbound", "address": "normal.pstn.twilio.com", "numbers": ["+15105550100"], "authUsername": "normal", "authPassword": "normal" } }After
{ "trunk": { "name": "Overridden outbound", "address": "normal.pstn.twilio.com", "numbers": ["+15105550101"], "authUsername": "normal", "authPassword": "normal", "fromHost": "overridden.com" } }