netbird/ice
0
0
Fork 0
mirror of https://github.com/netbirdio/ice.git synced 2026-05-22 17:10:58 -07:00
Code Issues Packages Projects Releases Wiki Activity

Accept use-candidate unconditionally for ice-lite (#739)

Browse Source 
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.
This commit is contained in:
Raja Subramanian
2024-10-31 11:05:14 +05:30
committed by GitHub
parent 166b1b7f9e
commit 9407bb0d2a
4 changed files with 144 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
agent.go
+8
View File
@@ -145,6 +145,8 @@ type Agent struct {
insecureSkipVerify bool
proxyDialer proxy.Dialer
enableUseCandidateCheckPriority bool
}
// NewAgent creates a new Agent
@@ -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{})}
@@ -1219,3 +1223,7 @@ func (a *Agent) setGatheringState(newState GatheringState) error {
<-done
return nil
}
func (a *Agent) needsToCheckPriorityOnNominated() bool {
return !a.lite || a.enableUseCandidateCheckPriority
}
agent_config.go
+7
View File
@@ -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
agent_test.go
+127 -78
View File
@@ -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,
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},
}
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))
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
aNotifier, aConnected := onConnected()
bNotifier, bConnected := onConnected()
cfg1 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net1,
KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
}
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}
}
bAgent, err = NewAgent(cfg1)
require.NoError(t, err)
defer func() {
require.NoError(t, bAgent.Close())
}()
require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))
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))
connect(aAgent, bAgent)
cfg1 := &AgentConfig{
NetworkTypes: []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6},
MulticastDNSMode: MulticastDNSModeDisabled,
Net: net1,
KeepaliveInterval: &KeepaliveInterval,
CheckInterval: &KeepaliveInterval,
}
// Ensure pair selected
// Note: this assumes ConnectionStateConnected is thrown after selecting the final pair
<-aConnected
<-bConnected
bAgent, err = NewAgent(cfg1)
require.NoError(t, err)
defer func() {
require.NoError(t, bAgent.Close())
}()
require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))
// 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)
connect(aAgent, bAgent)
return msg
}
// Ensure pair selected
// Note: this assumes ConnectionStateConnected is thrown after selecting the final pair
<-aConnected
<-bConnected
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)
// 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)
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
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)
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())
selection.go
+2 -2
View File
@@ -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())) {
s.agent.setSelectedPair(p)
} else if selectedPair != p {
s.log.Tracef("Ignore nominate new pair %s, already nominated pair %s", p, selectedPair)
@@ -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())) {
s.agent.setSelectedPair(p)
} else if selectedPair != p {
s.log.Tracef("Ignore nominate new pair %s, already nominated pair %s", p, selectedPair)