sip: redirect RTP destination on re-INVITE port change#728
Open
VictorUvarov wants to merge 11 commits into
Open
sip: redirect RTP destination on re-INVITE port change#728VictorUvarov wants to merge 11 commits into
VictorUvarov wants to merge 11 commits into
Conversation
When a carrier sends a re-INVITE with a new SDP offer containing a different RTP destination address/port, update the media port to send packets to the new destination. This fixes mid-call carrier port changes which were previously ignored. The fix parses the re-INVITE SDP offer body and calls UpdateRemote() to change the RTP destination before accepting the re-INVITE. Parsing errors are logged but don't prevent accepting the re-INVITE.
…INVITE test - In MediaPort.UpdateRemote(), add !addr.Addr().IsUnspecified() guard to reject RFC 3264 §8.4 call-hold form (c=0.0.0.0 with non-zero port). netip.AddrPort with unspecified address is IsValid() == true, so prior guard missed it. - Add TestMediaPortUpdateRemote case verifying 0.0.0.0:N is a no-op. - Add TestReinvite/inbound/no_body subtest: verify a body-less re-INVITE leaves RemoteAddr unchanged (guards against offer-in-ACK / no-SDP-body cases).
- Add sdpBodyFromRequest helper to guard updateRemoteFromSDP against non-SDP content types; avoids spurious warn log on e.g. application/json - Derive expected RemoteAddr in tests from newOffer.Addr instead of a hardcoded string literal - Add outbound/no_body test symmetric to inbound/no_body; use call.NewRequest(INVITE) in both no_body cases for a truly body-less re-INVITE (Invite(nil) silently generates an SDP offer)
|
|
udpConn.SetDst logged a nil *netip.AddrPort as "prev" on the first call
(before any destination was stored). The logger called .String() on the
nil pointer, causing a panic. Replace with the zero value netip.AddrPort{}.
Same class of bug as SetDst: udpConn.Read logged a nil *netip.AddrPort
as "prev" on the first packet received. Replace with netip.AddrPort{}.
req.ContentType() returns *ContentTypeHeader (a Stringer). When there is no body the header is nil; fmt calls String() on the typed nil pointer, causing a recovered panic that tparse reports as PANIC. Drop content-type from the reinvite log calls — content-length already indicates whether a body is present.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #728 +/- ##
==========================================
+ Coverage 65.25% 66.41% +1.15%
==========================================
Files 51 41 -10
Lines 6588 7625 +1037
==========================================
+ Hits 4299 5064 +765
- Misses 1915 2099 +184
- Partials 374 462 +88 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The "setting media destination/source" branch fires when prev is nil (no previous value). Logging prev as an empty AddrPort was misleading.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a SIP carrier sends a mid-call re-INVITE with a new RTP port (e.g. after a NAT rebind or media relay failover), LiveKit SIP was responding 200 OK but continuing to send RTP to the stale address — violating RFC 3264 §8, which requires the answerer to direct media to the address/port in the new offer.
Closes #661
What changed
pkg/sip/media_port.go: two new methods onMediaPort:UpdateRemote: atomically redirects the RTP destination.RemoteAddr: read accessor, used in tests.pkg/sip/inbound.go: both re-INVITE paths now parse the SDP body and callUpdateRemotebefore responding 200 OK.Tests
pkg/sip/media_port_test.go—TestMediaPortUpdateRemotecovers thehappy path, the zero-value no-op, and the unspecified-addr no-op.pkg/sip/signaling_test.go—TestReinviteextended:inbound/normal: assertsRemoteAddr()changes after a re-INVITE.outbound/normal: same for the outbound path.inbound/no_body: asserts a body-less re-INVITE leavesRemoteAddr()unchanged.Unit tests
Integration tests
go test ./pkg/sip/... -count=1Thought process summary
The root cause was a one-liner gap: the re-INVITE handler called AcceptAsKeepAlive (200 OK) but never forwarded the new c=/m= address to the RTP layer. The SDP body already contained the new addr+port as desc.Addr after ParseWith — nothing needed to be renegotiated, just the destination pointer swapped.
The tricky edge case was RFC 3264 §8.4 call-hold, where c=0.0.0.0 signals hold and
netip.MustParseAddrPort("0.0.0.0:N").IsValid()returnstrue. A plainIsValid()guard would have let hold re-INVITEs overwrite the real destination with the zero address. The!addr.Addr().IsUnspecified()check inside UpdateRemote blocks that.