Skip to content

Commit

Permalink
Accept use-candidate unconditionally for ice-lite
Browse files Browse the repository at this point in the history 
There could be a mismatch between the two ends in candidate priority
when using peer reflexive. It happens in the following scenario

1. Client has two srflx candidates.
   a. The first one gets discovered by LiveKit server as prflx.
   b. The second one gets added via ice-trickle first and then
      gets a STUN ping. So, it is srflx remote candidate from
      server's point-of-view.
2. This leads to a priority issue.
   a. Both candidates have same priority from client's point-of-view
      (both are srflx).
   b. But, from server's point-of-view, the first candidate has
      higher priority (prflx).
3. The first candidate establishes connectivity and becomes
   the selected pair (client is ICE controlling and server is
   ICE controlled, server is in ICE lite).
4. libwebrtc does a sort and switch some time later based on RTT.
   As client side has both at same priority, RTT based sorting
   could make the second candidate the preferred one.
   So, the client sends useCandidate=1 for the second candidate.
   pion/ice does not switch because the selected pair is at
   higher priority due to prflx candidate.
5. STUN pings do not happen and the ICE connection eventually fails.

ICE controlled agent should accept use-candidate unconditionally if
it is an ICE lite agentt.
Just in case existing behaviour is needed, it can be configured
using `EnableUseCandidateCheckPriority`.

NOTE: With aggressive nomination, the selected pair could change
a few times, but should eventually settle on what the controlling
side wants.
  • Loading branch information
@boks1971
boks1971 committed Oct 31, 2024
1 parent 166b1b7 commit 15ab10a
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 86 deletions.
8 changes: 8 additions & 0 deletions agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ type Agent struct {
insecureSkipVerify bool

proxyDialer proxy.Dialer

enableUseCandidateCheckPriority bool
}

// NewAgent creates a new Agent
Expand Down Expand Up @@ -219,6 +221,8 @@ func NewAgent(config *AgentConfig) (*Agent, error) { //nolint:gocognit
disableActiveTCP: config.DisableActiveTCP,

userBindingRequestHandler: config.BindingRequestHandler,

enableUseCandidateCheckPriority: config.EnableUseCandidateCheckPriority,
}
a.connectionStateNotifier = &handlerNotifier{connectionStateFunc: a.onConnectionStateChange, done: make(chan struct{})}
a.candidateNotifier = &handlerNotifier{candidateFunc: a.onCandidate, done: make(chan struct{})}
Expand Down Expand Up @@ -1219,3 +1223,7 @@ func (a *Agent) setGatheringState(newState GatheringState) error {
<-done
return nil
}

func (a *Agent) needsToCheckPriorityOnNominated() bool {
return !a.lite || a.enableUseCandidateCheckPriority

Check warning on line 1228 in agent.go

View check run for this annotation

Codecov / codecov/patch

agent.go#L1227-L1228 

Added lines #L1227 - L1228 were not covered by tests
}
7 changes: 7 additions & 0 deletions agent_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ type AgentConfig struct {
// * Implement draft-thatcher-ice-renomination
// * Implement custom CandidatePair switching logic
BindingRequestHandler func(m *stun.Message, local, remote Candidate, pair *CandidatePair) bool

// EnableUseCandidateCheckPriority can be used to enable checking for equal or higher priority to
// switch selected candidate pair if the peer requests USE-CANDIDATE and agent is a lite agent.
// This is disabled by default, i. e. when peer requests USE-CANDIDATE, the selected pair will be
// switched to that irrespective of relative priority between current selected pair
// and priority of the pair being switched to.
EnableUseCandidateCheckPriority bool
}

// initWithDefaults populates an agent and falls back to defaults if fields are unset
Expand Down
217 changes: 133 additions & 84 deletions agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1844,99 +1844,148 @@ func TestAcceptAggressiveNomination(t *testing.T) {

require.NoError(t, wan.Start())

aNotifier, aConnected := onConnected()
bNotifier, bConnected := onConnected()

KeepaliveInterval := time.Hour
cfg0 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net0,

KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
AcceptAggressiveNomination: true,
}

var aAgent, bAgent *Agent
aAgent, err = NewAgent(cfg0)
require.NoError(t, err)
defer func() {
require.NoError(t, aAgent.Close())
}()
require.NoError(t, aAgent.OnConnectionStateChange(aNotifier))

cfg1 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net1,
KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
}

bAgent, err = NewAgent(cfg1)
require.NoError(t, err)
defer func() {
require.NoError(t, bAgent.Close())
}()
require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))

connect(aAgent, bAgent)

// Ensure pair selected
// Note: this assumes ConnectionStateConnected is thrown after selecting the final pair
<-aConnected
<-bConnected
testCases := []struct {
name string
isLite bool
enableUseCandidateCheckPriority bool
useHigherPriority bool
isExpectedToSwitch bool
}{
{"should accept higher priority - full agent", false, false, true, true},
{"should not accept lower priority - full agent", false, false, false, false},
{"should accept higher priority - no use-candidate priority check - lite agent", true, false, true, true},
{"should accept lower priority - no use-candidate priority check - lite agent", true, false, false, true},
{"should accept higher priority - use-candidate priority check - lite agent", true, true, true, true},
{"should not accept lower priority - use-candidate priority check - lite agent", true, true, false, false},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
aNotifier, aConnected := onConnected()
bNotifier, bConnected := onConnected()

KeepaliveInterval := time.Hour
cfg0 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net0,
KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
Lite: tc.isLite,
EnableUseCandidateCheckPriority: tc.enableUseCandidateCheckPriority,
}
if tc.isLite {
cfg0.CandidateTypes = []CandidateType{CandidateTypeHost}
}

// Send new USE-CANDIDATE message with higher priority to update the selected pair
buildMsg := func(class stun.MessageClass, username, key string, priority uint32) *stun.Message {
msg, err1 := stun.Build(stun.NewType(stun.MethodBinding, class), stun.TransactionID,
stun.NewUsername(username),
stun.NewShortTermIntegrity(key),
UseCandidate(),
PriorityAttr(priority),
stun.Fingerprint,
)
require.NoError(t, err1)
var aAgent, bAgent *Agent
aAgent, err = NewAgent(cfg0)
require.NoError(t, err)
defer func() {
require.NoError(t, aAgent.Close())
}()
require.NoError(t, aAgent.OnConnectionStateChange(aNotifier))

cfg1 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net1,
KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
}

return msg
}
bAgent, err = NewAgent(cfg1)
require.NoError(t, err)
defer func() {
require.NoError(t, bAgent.Close())
}()
require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))

connect(aAgent, bAgent)

// Ensure pair selected
// Note: this assumes ConnectionStateConnected is thrown after selecting the final pair
<-aConnected
<-bConnected

// Send new USE-CANDIDATE message with priority to update the selected pair
buildMsg := func(class stun.MessageClass, username, key string, priority uint32) *stun.Message {
msg, err1 := stun.Build(stun.NewType(stun.MethodBinding, class), stun.TransactionID,
stun.NewUsername(username),
stun.NewShortTermIntegrity(key),
UseCandidate(),
PriorityAttr(priority),
stun.Fingerprint,
)
require.NoError(t, err1)

return msg
}

selectedCh := make(chan Candidate, 1)
var expectNewSelectedCandidate Candidate
err = aAgent.OnSelectedCandidatePairChange(func(_, remote Candidate) {
selectedCh <- remote
})
require.NoError(t, err)
var bcandidates []Candidate
bcandidates, err = bAgent.GetLocalCandidates()
require.NoError(t, err)
selectedCh := make(chan Candidate, 1)
var expectNewSelectedCandidate Candidate
err = aAgent.OnSelectedCandidatePairChange(func(_, remote Candidate) {
selectedCh <- remote
})
require.NoError(t, err)
var bcandidates []Candidate
bcandidates, err = bAgent.GetLocalCandidates()
require.NoError(t, err)

for _, c := range bcandidates {
if c != bAgent.getSelectedPair().Local {
if expectNewSelectedCandidate == nil {
incr_priority:
for _, candidates := range aAgent.remoteCandidates {
for _, candidate := range candidates {
if candidate.Equal(c) {
candidate.(*CandidateHost).priorityOverride += 1000 //nolint:forcetypeassert
break incr_priority
for _, c := range bcandidates {
if c != bAgent.getSelectedPair().Local {
if expectNewSelectedCandidate == nil {
expected_change_priority:
for _, candidates := range aAgent.remoteCandidates {
for _, candidate := range candidates {
if candidate.Equal(c) {
if tc.useHigherPriority {
candidate.(*CandidateHost).priorityOverride += 1000 //nolint:forcetypeassert
} else {
candidate.(*CandidateHost).priorityOverride -= 1000 //nolint:forcetypeassert
}
break expected_change_priority
}
}
}
if tc.isExpectedToSwitch {
expectNewSelectedCandidate = c
} else {
expectNewSelectedCandidate = aAgent.getSelectedPair().Remote
}
} else {
// a smaller change for other candidates other the new expected one
change_priority:
for _, candidates := range aAgent.remoteCandidates {
for _, candidate := range candidates {
if candidate.Equal(c) {
if tc.useHigherPriority {
candidate.(*CandidateHost).priorityOverride += 500 //nolint:forcetypeassert
} else {
candidate.(*CandidateHost).priorityOverride -= 500 //nolint:forcetypeassert
}
break change_priority
}
}
}
}
_, err = c.writeTo(buildMsg(stun.ClassRequest, aAgent.localUfrag+":"+aAgent.remoteUfrag, aAgent.localPwd, c.Priority()).Raw, bAgent.getSelectedPair().Remote)
require.NoError(t, err)
}
expectNewSelectedCandidate = c
}
_, err = c.writeTo(buildMsg(stun.ClassRequest, aAgent.localUfrag+":"+aAgent.remoteUfrag, aAgent.localPwd, c.Priority()).Raw, bAgent.getSelectedPair().Remote)
require.NoError(t, err)
}
}

time.Sleep(1 * time.Second)
select {
case selected := <-selectedCh:
require.True(t, selected.Equal(expectNewSelectedCandidate))
default:
t.Fatal("No selected candidate pair")
time.Sleep(1 * time.Second)
select {
case selected := <-selectedCh:
require.True(t, selected.Equal(expectNewSelectedCandidate))
default:
if !tc.isExpectedToSwitch {
require.True(t, aAgent.getSelectedPair().Remote.Equal(expectNewSelectedCandidate))
} else {
t.Fatal("No selected candidate pair")
}
}
})
}

require.NoError(t, wan.Stop())
Expand Down
4 changes: 2 additions & 2 deletions selection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (s *controlledSelector) HandleSuccessResponse(m *stun.Message, local, remot
s.log.Tracef("Found valid candidate pair: %s", p)
if p.nominateOnBindingSuccess {
if selectedPair := s.agent.getSelectedPair(); selectedPair == nil ||
(selectedPair != p && selectedPair.priority() <= p.priority()) {
(selectedPair != p && (!s.agent.needsToCheckPriorityOnNominated() || selectedPair.priority() <= p.priority())) {

Check warning on line 244 in selection.go

View check run for this annotation

Codecov / codecov/patch

selection.go#L244 

Added line #L244 was not covered by tests
s.agent.setSelectedPair(p)
} else if selectedPair != p {
s.log.Tracef("Ignore nominate new pair %s, already nominated pair %s", p, selectedPair)
Expand All @@ -266,7 +266,7 @@ func (s *controlledSelector) HandleBindingRequest(m *stun.Message, local, remote
// generated a valid pair (Section 7.2.5.3.2). The agent sets the
// nominated flag value of the valid pair to true.
selectedPair := s.agent.getSelectedPair()
if selectedPair == nil || (selectedPair != p && selectedPair.priority() <= p.priority()) {
if selectedPair == nil || (selectedPair != p && (!s.agent.needsToCheckPriorityOnNominated() || selectedPair.priority() <= p.priority())) {

Check warning on line 269 in selection.go

View check run for this annotation

Codecov / codecov/patch

selection.go#L269 

Added line #L269 was not covered by tests
s.agent.setSelectedPair(p)
} else if selectedPair != p {
s.log.Tracef("Ignore nominate new pair %s, already nominated pair %s", p, selectedPair)
Expand Down

0 comments on commit 15ab10a

Please sign in to comment.