From 45f989f52a769bb51e37bf91d3c4ca64593f52e6 Mon Sep 17 00:00:00 2001 From: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> Date: Thu, 26 Mar 2026 12:36:31 -0400 Subject: [PATCH] ipn/ipnlocal: warn incompatibility between no-snat-routes and exitnode (#19023) * ipn/ipnlocal: warn incompatibility between no-snat-routes and exitnode This commit adds a warning to health check when the --snat-subnet-routes=false flag for subnet router is set alone side --advertise-exit-node=true. These two would conflict with each other and result internet-bound traffic from peers using this exit node no masqueraded to the node's source IP and fail to route return packets back. The described combination is not valid until we figure out a way to separate exitnode masquerade rule and skip it for subnet routes. Updates #18725 Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> * use date instead of for now to clarify effectivness Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> --------- Signed-off-by: KevinLiang10 <37811973+KevinLiang10@users.noreply.github.com> --- cmd/tailscale/cli/set.go | 5 +++ cmd/tailscale/cli/up.go | 5 +++ ipn/ipnlocal/local.go | 25 +++++++++++++ ipn/ipnlocal/local_test.go | 72 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/cmd/tailscale/cli/set.go b/cmd/tailscale/cli/set.go index b12b7de49..6fd4b09ad 100644 --- a/cmd/tailscale/cli/set.go +++ b/cmd/tailscale/cli/set.go @@ -265,6 +265,11 @@ func runSet(ctx context.Context, args []string) (retErr error) { checkPrefs := curPrefs.Clone() checkPrefs.ApplyEdits(maskedPrefs) + // We want to make sure user is aware setting --snat-subnet-routes=false with --advertise-exit-node would break exitnode, + // but we won't prevent them from doing it since there are current dependencies on that combination. (as of 2026-03-25) + if checkPrefs.NoSNAT && checkPrefs.AdvertisesExitNode() { + warnf("--snat-subnet-routes=false is set with --advertise-exit-node; internet traffic through this exit node may not work as expected") + } if err := localClient.CheckPrefs(ctx, checkPrefs); err != nil { return err } diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 81c67d662..ba87739fc 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -357,6 +357,11 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo if goos == "linux" { prefs.NoSNAT = !upArgs.snat + // We want to make sure user is aware setting --snat-subnet-routes=false with --advertise-exit-node would break exitnode, + // but we won't prevent them from doing it since there are current dependencies on that combination. (as of 2026-03-25) + if prefs.NoSNAT && prefs.AdvertisesExitNode() { + warnf("--snat-subnet-routes=false is set with --advertise-exit-node; internet traffic through this exit node may not work as expected") + } // Backfills for NoStatefulFiltering occur when loading a profile; just set it explicitly here. prefs.NoStatefulFiltering.Set(!upArgs.statefulFiltering) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7cd4e2afb..845317c4a 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2576,6 +2576,7 @@ func (b *LocalBackend) startLocked(opts ipn.Options) error { // Reset the always-on override whenever Start is called. b.resetAlwaysOnOverrideLocked() b.setAtomicValuesFromPrefsLocked(prefs) + b.updateNoSNATExitNodeWarning(prefs) wantRunning := prefs.WantRunning() if wantRunning { @@ -4745,6 +4746,7 @@ func (b *LocalBackend) setPrefsLocked(newp *ipn.Prefs) ipn.PrefsView { b.pauseOrResumeControlClientLocked() // for prefs.Sync changes b.updateWarnSync(prefs) + b.updateNoSNATExitNodeWarning(prefs) if oldp.ShieldsUp() != newp.ShieldsUp || hostInfoChanged { b.doSetHostinfoFilterServicesLocked() @@ -6984,6 +6986,18 @@ func (b *LocalBackend) sshServerOrInit() (_ SSHServer, err error) { Text: health.StaticMessage("SELinux is enabled; Tailscale SSH may not work. See https://tailscale.com/s/ssh-selinux"), }) +// warnNoSNATWithExitNode is a warnable for when a node is advertising as an +// exit node but has SNAT disabled. In this configuration internet-bound traffic +// from peers using this exit node will not be masqueraded to the node's own +// source IP, so return packets cannot be routed back, causing the exit node to +// not work as expected. +var warnNoSNATWithExitNode = health.Register(&health.Warnable{ + Code: "nosnat-with-advertised-exit-node", + Title: "Exit node advertising may not work correctly", + Severity: health.SeverityMedium, + Text: health.StaticMessage("snat-subnet-routes is disabled while advertising as an exit node; internet traffic through this exit node may not work as expected"), +}) + func (b *LocalBackend) updateSELinuxHealthWarning() { if hostinfo.IsSELinuxEnforcing() { b.health.SetUnhealthy(warnSSHSELinuxWarnable, nil) @@ -7000,6 +7014,17 @@ func (b *LocalBackend) updateWarnSync(prefs ipn.PrefsView) { } } +func (b *LocalBackend) updateNoSNATExitNodeWarning(prefs ipn.PrefsView) { + if !buildfeatures.HasAdvertiseExitNode { + return + } + if prefs.NoSNAT() && prefs.AdvertisesExitNode() { + b.health.SetUnhealthy(warnNoSNATWithExitNode, nil) + } else { + b.health.SetHealthy(warnNoSNATWithExitNode) + } +} + func (b *LocalBackend) handleSSHConn(c net.Conn) (err error) { s, err := b.sshServerOrInit() if err != nil { diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 317275567..bac9e0418 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -7878,3 +7878,75 @@ func TestEditPrefs_InvalidAdvertiseRoutes(t *testing.T) { }) } } + +func TestNoSNATWithAdvertisedExitNodeWarning(t *testing.T) { + exitRoutes := []netip.Prefix{ + netip.MustParsePrefix("0.0.0.0/0"), + netip.MustParsePrefix("::/0"), + } + warnCode := health.WarnableCode("nosnat-with-advertised-exit-node") + + tests := []struct { + name string + prefs *ipn.Prefs + wantWarning bool + }{ + { + name: "no-snat-without-exit-node", + prefs: &ipn.Prefs{ + NoSNAT: true, + AdvertiseRoutes: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}, + }, + wantWarning: false, + }, + { + name: "snat-enabled-with-exit-node", + prefs: &ipn.Prefs{ + NoSNAT: false, + AdvertiseRoutes: exitRoutes, + }, + wantWarning: false, + }, + { + name: "no-snat-with-exit-node", + prefs: &ipn.Prefs{ + NoSNAT: true, + AdvertiseRoutes: exitRoutes, + }, + wantWarning: true, + }, + { + name: "no-snat-with-exit-node-and-subnet", + prefs: &ipn.Prefs{ + NoSNAT: true, + AdvertiseRoutes: append([]netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/24"), + }, exitRoutes...), + }, + wantWarning: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := newTestLocalBackend(t) + b.SetPrefsForTest(tt.prefs) + _, hasWarning := b.HealthTracker().CurrentState().Warnings[warnCode] + if hasWarning != tt.wantWarning { + t.Errorf("warning present = %v, want %v", hasWarning, tt.wantWarning) + } + }) + } + + // Verify that the warning clears when the conflicting combination is resolved. + t.Run("warning-clears-on-fix", func(t *testing.T) { + b := newTestLocalBackend(t) + b.SetPrefsForTest(&ipn.Prefs{NoSNAT: true, AdvertiseRoutes: exitRoutes}) + if _, ok := b.HealthTracker().CurrentState().Warnings[warnCode]; !ok { + t.Fatal("expected warning to be set") + } + b.SetPrefsForTest(&ipn.Prefs{NoSNAT: false, AdvertiseRoutes: exitRoutes}) + if _, ok := b.HealthTracker().CurrentState().Warnings[warnCode]; ok { + t.Fatal("expected warning to be cleared after enabling SNAT") + } + }) +}