Handle syspolicy ExitNodeID "auto" value #12114
Conversation
ipn/ipnlocal/local.go
Outdated
| func (b *LocalBackend) SuggestExitNode() (response apitype.ExitNodeSuggestionResponse, err error) { | ||
| b.mu.Lock() | ||
| // b.mu must be held. | ||
| func (b *LocalBackend) suggestExitNodeLocked(isAuto bool) (response apitype.ExitNodeSuggestionResponse, err error) { |
There was a problem hiding this comment.
@bradfitz I started using the suggestExitNode function in places where the LocalBackend mutex is already held. Is this refactor okay for how I want to use the mutex w/ respect to reading and writing the lastSuggestedExitNode value?
clairew
force-pushed
the
clairew/handle-auto-exit-node-value
branch
2 times, most recently
from
c32b5dc
to
01a5fb6
Compare
clairew
force-pushed
the
clairew/handle-auto-exit-node-value
branch
5 times, most recently
from
ca5cf43
to
83517ab
Compare
clairew
force-pushed
the
clairew/handle-auto-exit-node-value
branch
2 times, most recently
from
4ad52fe
to
392858e
Compare
clairew
force-pushed
the
clairew/handle-auto-exit-node-value
branch
from
392858e
to
1879b6e
Compare
| @@ -1379,6 +1381,15 @@ func (b *LocalBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo | |||
| nm.Peers = make([]tailcfg.NodeView, 0, len(b.peers)) | |||
| for _, p := range b.peers { | |||
| nm.Peers = append(nm.Peers, p) | |||
| if !*p.Online() && p.StableID() == b.pm.CurrentPrefs().ExitNodeID() { | |||
There was a problem hiding this comment.
If p.Online() == nil maybe skip this check.
... although, looking at this closer, it would be even better to look at muts because that'll show which nodes changed, rather than needing to loop through all of them.
| if exitNodeIDStr, _ := syspolicy.GetString(syspolicy.ExitNodeID, ""); exitNodeIDStr == "auto" { | ||
| prefs := b.pm.CurrentPrefs().AsStruct() | ||
| b.setExitNodeIDLocked(prefs, nm) | ||
| if err := b.pm.SetPrefs(prefs.View(), ipn.NetworkProfile{}); err != nil { |
There was a problem hiding this comment.
Looks like the ipn.NetworkProfile should be filled in, there's a bunch of examples all over, e.g. look at SetControlClientStatus here.
Also looking at b.setPrefsLockedOnEntry, I don't think this is actually sufficient to get everything updated -- there needs to be a notify with the updated prefs, and it needs to take effect etc. I think it needs to trigger something like SetControlClientStatus in the background somehow.
After a lot of digging, I think the solution is to do the expensive full netmap update path if the current auto exit node goes offline. What that means is return false. I think that should be done before partially applying the updates though.
My best idea right now: right after b.mu.Lock(), check for auto exit node being enabled. If so, look in muts for the current exit node getting a lastSeen or online status update. If so, return false. Otherwise, keep going. lastSuggestedExitNode may need to be cleared too so it won't get reused.
| prefs.ExitNodeID = exitNodeID | ||
| var changed bool | ||
| if exitNodeIDStr == "auto" { | ||
| if b.lastSuggestedExitNode.id != "" { |
There was a problem hiding this comment.
I think reusing the last suggested exit node should only happen after checking that the last suggestion is still a good one. Like at the end of suggestExitNodeLocked when it uses a random selection to pick one of the candidates, always pick the last suggestion if it's in the candidate list, but if it's not in the candidate list, then do the random selection.
| b.mu.Unlock() | ||
|
|
||
| if cc == nil { | ||
| return | ||
| } | ||
| cc.SetNetInfo(ni) | ||
| if refresh { | ||
| if exitNodeIDStr, _ := syspolicy.GetString(syspolicy.ExitNodeID, ""); exitNodeIDStr == "auto" { |
There was a problem hiding this comment.
This is done so often, there should be a shouldAutoExitNode() or something.
ipn/ipnlocal/local.go
Outdated
| if netInfo != nil { | ||
| latency, err = processNetInfo(netInfo) | ||
| if err != nil { | ||
| return response, ErrCannotSuggestExitNode |
There was a problem hiding this comment.
| return response, ErrCannotSuggestExitNode | |
| return response, err |
| b.mu.Unlock() | ||
|
|
||
| if cc == nil { | ||
| return | ||
| } | ||
| cc.SetNetInfo(ni) | ||
| if refresh { | ||
| if exitNodeIDStr, _ := syspolicy.GetString(syspolicy.ExitNodeID, ""); exitNodeIDStr == "auto" { | ||
| b.suggestExitNodeLocked(true, ni) |
There was a problem hiding this comment.
This computes a new answer, but does it actually change the active exit node?
Updates tailscale/corp#19681 Signed-off-by: Claire Wang <claire@tailscale.com>
clairew
force-pushed
the
clairew/handle-auto-exit-node-value
branch
from
1879b6e
to
d541079
Compare
If ExitNodeID syspolicy value is "auto", keeps choosing suggested exit node.
If the selected exit node goes offline, automatically chooses another suggestion.
If a rebind happens, automatically chooses another suggestion.