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

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'

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. Future-us can either break this, or deprecate it and replace it with something more interesting.

Signed-off-by: Jonathan Nobels <jonathan@tailscale.com>

ipn/ipn_clone.go

@@ -42,7 +42,7 @@ var _PrefsCloneNeedsRegeneration = Prefs(struct {
 	AllowSingleHosts       bool
 	ExitNodeID             tailcfg.StableNodeID
 	ExitNodeIP             netip.Addr
-	InternalExitNodePrior  string
+	InternalExitNodePrior  tailcfg.StableNodeID
 	ExitNodeAllowLANAccess bool
 	CorpDNS                bool
 	RunSSH                 bool

ipn/ipn_view.go

@@ -69,7 +69,7 @@ func (v PrefsView) RouteAll() bool                     { return v.ж.RouteAll }
 func (v PrefsView) AllowSingleHosts() bool                      { return v.ж.AllowSingleHosts }
 func (v PrefsView) ExitNodeID() tailcfg.StableNodeID            { return v.ж.ExitNodeID }
 func (v PrefsView) ExitNodeIP() netip.Addr                      { return v.ж.ExitNodeIP }
-func (v PrefsView) InternalExitNodePrior() string      { return v.ж.InternalExitNodePrior }
+func (v PrefsView) InternalExitNodePrior() tailcfg.StableNodeID { return v.ж.InternalExitNodePrior }
 func (v PrefsView) ExitNodeAllowLANAccess() bool                { return v.ж.ExitNodeAllowLANAccess }
 func (v PrefsView) CorpDNS() bool                               { return v.ж.CorpDNS }
 func (v PrefsView) RunSSH() bool                                { return v.ж.RunSSH }
@@ -105,7 +105,7 @@ var _PrefsViewNeedsRegeneration = Prefs(struct {
 	AllowSingleHosts       bool
 	ExitNodeID             tailcfg.StableNodeID
 	ExitNodeIP             netip.Addr
-	InternalExitNodePrior  string
+	InternalExitNodePrior  tailcfg.StableNodeID
 	ExitNodeAllowLANAccess bool
 	CorpDNS                bool
 	RunSSH                 bool

ipn/ipnlocal/local.go

@@ -3096,7 +3096,7 @@ func (b *LocalBackend) SetUseExitNodeEnabled(v bool) (ipn.PrefsView, error) {
 		mp.ExitNodeIDSet = true
 		mp.ExitNodeID = ""
 		mp.InternalExitNodePriorSet = true
-		mp.InternalExitNodePrior = string(p0.ExitNodeID())
+		mp.InternalExitNodePrior = p0.ExitNodeID()
 	}
 	return b.editPrefsLockedOnEntry(mp, unlock)
 }
@@ -3105,6 +3105,13 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (ipn.PrefsView, error) {
 	if mp.SetsInternal() {
 		return ipn.PrefsView{}, errors.New("can't set Internal fields")
 	}
+
+	// Zeroing the ExitNodeId via localAPI must also zero the prior exit node.
+	if mp.ExitNodeIDSet && mp.ExitNodeID == "" {
+		mp.InternalExitNodePrior = ""
+		mp.InternalExitNodePriorSet = true
+	}
+
 	unlock := b.lockAndGetUnlock()
 	defer unlock()
 	return b.editPrefsLockedOnEntry(mp, unlock)

ipn/ipnlocal/local_test.go

@@ -458,6 +458,44 @@ func TestLazyMachineKeyGeneration(t *testing.T) {
 	time.Sleep(500 * time.Millisecond)
 }
 
+func TestZeroExitNodeViaLocalAPI(t *testing.T) {
+	lb := newTestLocalBackend(t)
+
+	// Give it an initial exit node in use.
+	if _, err := lb.EditPrefs(&ipn.MaskedPrefs{
+		ExitNodeIDSet: true,
+		Prefs: ipn.Prefs{
+			ExitNodeID: "foo",
+		},
+	}); err != nil {
+		t.Fatalf("enabling first exit node: %v", err)
+	}
+
+	// SetUseExitNodeEnabled(false) "remembers" the prior exit node.
+	if _, err := lb.SetUseExitNodeEnabled(false); err != nil {
+		t.Fatal("expected failure")
+	}
+
+	// Zero the exit node
+	pv, err := lb.EditPrefs(&ipn.MaskedPrefs{
+		ExitNodeIDSet: true,
+		Prefs: ipn.Prefs{
+			ExitNodeID: "",
+		},
+	})
+
+	if err != nil {
+		t.Fatalf("enabling first exit node: %v", err)
+	}
+
+	// We just set the internal exit node to the empty string, so InternalExitNodePrior should
+	// also be zero'd
+	if got, want := pv.InternalExitNodePrior(), tailcfg.StableNodeID(""); got != want {
+		t.Fatalf("unexpected InternalExitNodePrior %q, want: %q", got, want)
+	}
+
+}
+
 func TestSetUseExitNodeEnabled(t *testing.T) {
 	lb := newTestLocalBackend(t)
 
@@ -488,7 +526,7 @@ func TestSetUseExitNodeEnabled(t *testing.T) {
 		if g, w := prefs.ExitNodeID(), tailcfg.StableNodeID(""); g != w {
 			t.Fatalf("unexpected exit node ID %q; want %q", g, w)
 		}
-		if g, w := prefs.InternalExitNodePrior(), "foo"; g != w {
+		if g, w := prefs.InternalExitNodePrior(), tailcfg.StableNodeID("foo"); g != w {
 			t.Fatalf("unexpected exit node prior %q; want %q", g, w)
 		}
 	}
@@ -500,7 +538,7 @@ func TestSetUseExitNodeEnabled(t *testing.T) {
 		if g, w := prefs.ExitNodeID(), tailcfg.StableNodeID("foo"); g != w {
 			t.Fatalf("unexpected exit node ID %q; want %q", g, w)
 		}
-		if g, w := prefs.InternalExitNodePrior(), "foo"; g != w {
+		if g, w := prefs.InternalExitNodePrior(), tailcfg.StableNodeID("foo"); g != w {
 			t.Fatalf("unexpected exit node prior %q; want %q", g, w)
 		}
 	}

ipn/prefs.go

@@ -107,11 +107,11 @@ type Prefs struct {
 
 	// InternalExitNodePrior is the most recently used ExitNodeID in string form. It is set by
 	// the backend on transition from exit node on to off and used by the
-	// backend. It's not of type tailcfg.StableNodeID because in the future we plan
-	// to overload this field to mean things like "Anything in country $FOO" too.
+	// backend.
 	//
-	// As an Internal field, it can't be set by LocalAPI clients.
-	InternalExitNodePrior string
+	// As an Internal field, it can't be set by LocalAPI clients, rather it is set indirectly
+	// when the ExitNodeID value is zero'd and via the set-use-exit-node-enabled endpoint.
+	InternalExitNodePrior tailcfg.StableNodeID
 
 	// ExitNodeAllowLANAccess indicates whether locally accessible subnets should be
 	// routed directly or via the exit node.