ipn/{localapi, ipnlocal}: forget the prior exit node when localAPI is used to zero the ExitNodeID

[barnstar]

Updates tailscale/corp#18724

When localAPI clients directly set ExitNodeID to "", the expected behaviour is that the prior exit node also gets zero'd - effectively setting the UI state back to 'no exit node was ever selected'

If a client wants to zero the exit node, but remember it, they use SetUseExitNodeEnabled.

The IntenalExitNodePrior has been changed to be a non-opaque type, as it is read by the UI to render the users last selected exit node, and must be concrete type. Future-us can either break this, or deprecate it and replace it with something more interesting.

[bradfitz]

Code's fine but I feel like we've gone down the wrong path if we have to add this explosion of so-related APIs around all this.

I also don't see why this is needed--- if they set it to None and we set that with EditPrefs, do we care if they toggle it back on with the toggle API and it goes back to the previous pre-None one?

And if we really care, can we make EditPrefs do the clearing of the old one instead?

[barnstar]

@bradfitz It's about visually representing the state where you have no exit node set, and you have no prior preference. This is just a UX thing. Without this, selecting none is the equivalent of selecting "off". There's no combination of prefs or api calls that gives ExitNodeID="" and Prior=""... Prior is always set to something, and therefore, it's always shown in the UI.

If you're OK with making InternalPrior not internal (ie: let the UI just set it), that seems like the simplest approach.

[bradfitz]

Oh, so there's the root of the problem: the UI's assuming some distinction between Off and None that doesn't actually exist.

I assume you're also reading the Internal value and trying to make sense of it? It's supposed to be an opaque string with future meanings like "something in country $X" in the future. I don't think the UI should be reading it.

[barnstar]

This is way simpler than a new endpoint. It's really just localAPI zeroing the ExitNodeID we care about. Should be all backwards and future compatible. The backend is still free to set that InternalExitNodePrior to "future thing" when an ExitNode is enabled, and the UI will still zero it if the user decides Exit Nodes aren't their thing.

[barnstar]

Hmm @bradfitz just saw your comment there about the opacity of that internal value... The right answer seems to be "add a value the UI can read"... And leave future considerations for the future.

[oxtoacart]

Pardon my confusion, but does this already incorporate the OSS change?

[barnstar]

Pardon my confusion, but does this already incorporate the OSS change?

It does. This is a slight modification to that. I ran into the noted UX issue of not being able to do the "None" thing when trying to get things matchy-matchy with iOS.