tailcfg, control/controlclient: start moving MapResponse.DefaultAutoUpdate to a nodeattr

And fix up the TestAutoUpdateDefaults integration tests as they
weren't testing reality: the DefaultAutoUpdate is supposed to only be
relevant on the first MapResponse in the stream, but the tests weren't
testing that. They were instead injecting a 2nd+ MapResponse.

This changes the test control server to add a hook to modify the first
map response, and then makes the test control when the node goes up
and down to make new map responses.

Also, the test now runs on macOS where the auto-update feature being
disabled would've previously t.Skipped the whole test.

Updates #11502

Change-Id: If2319bd1f71e108b57d79fe500b2acedbc76e1a6
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This commit is contained in:
Brad Fitzpatrick 2025-09-29 12:17:52 -07:00 committed by Brad Fitzpatrick
parent 848978e664
commit ac0b15356d
9 changed files with 147 additions and 43 deletions

View File

@ -107,7 +107,7 @@ OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.ClientVersion
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.CollectServices OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.CollectServices
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.ControlDialPlan OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.ControlDialPlan
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.Debug OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.Debug
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DefaultAutoUpdate OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DeprecatedDefaultAutoUpdate
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DERPMap OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DERPMap
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DNSConfig OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.DNSConfig
OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.Node OmitEmptyShouldBeOmitZero tailscale.com/tailcfg.MapResponse.Node

View File

@ -1184,7 +1184,19 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap
metricMapResponseKeepAlives.Add(1) metricMapResponseKeepAlives.Add(1)
continue continue
} }
if au, ok := resp.DefaultAutoUpdate.Get(); ok {
// DefaultAutoUpdate in its CapMap and deprecated top-level field forms.
if self := resp.Node; self != nil {
for _, v := range self.CapMap[tailcfg.NodeAttrDefaultAutoUpdate] {
switch v {
case "true", "false":
c.autoUpdatePub.Publish(AutoUpdate{c.controlClientID, v == "true"})
default:
c.logf("netmap: [unexpected] unknown %s in CapMap: %q", tailcfg.NodeAttrDefaultAutoUpdate, v)
}
}
}
if au, ok := resp.DeprecatedDefaultAutoUpdate.Get(); ok {
c.autoUpdatePub.Publish(AutoUpdate{c.controlClientID, au}) c.autoUpdatePub.Publish(AutoUpdate{c.controlClientID, au})
} }

View File

@ -7,6 +7,8 @@ package feature
import ( import (
"errors" "errors"
"reflect" "reflect"
"tailscale.com/util/testenv"
) )
var ErrUnavailable = errors.New("feature not included in this build") var ErrUnavailable = errors.New("feature not included in this build")
@ -55,6 +57,19 @@ func (h *Hook[Func]) Set(f Func) {
h.ok = true h.ok = true
} }
// SetForTest sets the hook function for tests, blowing
// away any previous value. It will panic if called from
// non-test code.
//
// It returns a restore function that resets the hook
// to its previous value.
func (h *Hook[Func]) SetForTest(f Func) (restore func()) {
testenv.AssertInTest()
old := *h
h.f, h.ok = f, true
return func() { *h = old }
}
// Get returns the hook function, or panics if it hasn't been set. // Get returns the hook function, or panics if it hasn't been set.
// Use IsSet to check if it's been set, or use GetOrNil if you're // Use IsSet to check if it's been set, or use GetOrNil if you're
// okay with a nil return value. // okay with a nil return value.

View File

@ -6,6 +6,8 @@ package feature
import ( import (
"net/http" "net/http"
"net/url" "net/url"
"os"
"sync"
"tailscale.com/types/logger" "tailscale.com/types/logger"
"tailscale.com/types/persist" "tailscale.com/types/persist"
@ -15,9 +17,16 @@ import (
// to conditionally initialize. // to conditionally initialize.
var HookCanAutoUpdate Hook[func() bool] var HookCanAutoUpdate Hook[func() bool]
var testAllowAutoUpdate = sync.OnceValue(func() bool {
return os.Getenv("TS_TEST_ALLOW_AUTO_UPDATE") == "1"
})
// CanAutoUpdate reports whether the current binary is built with auto-update // CanAutoUpdate reports whether the current binary is built with auto-update
// support and, if so, whether the current platform supports it. // support and, if so, whether the current platform supports it.
func CanAutoUpdate() bool { func CanAutoUpdate() bool {
if testAllowAutoUpdate() {
return true
}
if f, ok := HookCanAutoUpdate.GetOk(); ok { if f, ok := HookCanAutoUpdate.GetOk(); ok {
return f() return f()
} }

View File

@ -177,7 +177,8 @@ type CapabilityVersion int
// - 128: 2025-10-02: can handle C2N /debug/health. // - 128: 2025-10-02: can handle C2N /debug/health.
// - 129: 2025-10-04: Fixed sleep/wake deadlock in magicsock when using peer relay (PR #17449) // - 129: 2025-10-04: Fixed sleep/wake deadlock in magicsock when using peer relay (PR #17449)
// - 130: 2025-10-06: client can send key.HardwareAttestationPublic and key.HardwareAttestationKeySignature in MapRequest // - 130: 2025-10-06: client can send key.HardwareAttestationPublic and key.HardwareAttestationKeySignature in MapRequest
const CurrentCapabilityVersion CapabilityVersion = 130 // - 131: 2025-11-25: client respects [NodeAttrDefaultAutoUpdate]
const CurrentCapabilityVersion CapabilityVersion = 131
// ID is an integer ID for a user, node, or login allocated by the // ID is an integer ID for a user, node, or login allocated by the
// control plane. // control plane.
@ -2149,12 +2150,14 @@ type MapResponse struct {
// or nothing to report. // or nothing to report.
ClientVersion *ClientVersion `json:",omitempty"` ClientVersion *ClientVersion `json:",omitempty"`
// DefaultAutoUpdate is the default node auto-update setting for this // DeprecatedDefaultAutoUpdate is the default node auto-update setting for this
// tailnet. The node is free to opt-in or out locally regardless of this // tailnet. The node is free to opt-in or out locally regardless of this
// value. This value is only used on first MapResponse from control, the // value. Once this value has been set and stored in the client, future
// auto-update setting doesn't change if the tailnet admin flips the // changes from the control plane are ignored.
// default after the node registered. //
DefaultAutoUpdate opt.Bool `json:",omitempty"` // Deprecated: use NodeAttrDefaultAutoUpdate instead. See
// https://github.com/tailscale/tailscale/issues/11502.
DeprecatedDefaultAutoUpdate opt.Bool `json:"DefaultAutoUpdate,omitempty"`
} }
// DisplayMessage represents a health state of the node from the control plane's // DisplayMessage represents a health state of the node from the control plane's
@ -2721,6 +2724,14 @@ const (
// default behavior is to trust the control plane when it claims that a // default behavior is to trust the control plane when it claims that a
// node is no longer online, but that is not a reliable signal. // node is no longer online, but that is not a reliable signal.
NodeAttrClientSideReachability = "client-side-reachability" NodeAttrClientSideReachability = "client-side-reachability"
// NodeAttrDefaultAutoUpdate advertises the default node auto-update setting
// for this tailnet. The node is free to opt-in or out locally regardless of
// this value. Once this has been set and stored in the client, future
// changes from the control plane are ignored.
//
// The value of the key in [NodeCapMap] is a JSON boolean.
NodeAttrDefaultAutoUpdate NodeCapability = "default-auto-update"
) )
// SetDNSRequest is a request to add a DNS record. // SetDNSRequest is a request to add a DNS record.

View File

@ -576,6 +576,7 @@ type TestNode struct {
stateFile string stateFile string
upFlagGOOS string // if non-empty, sets TS_DEBUG_UP_FLAG_GOOS for cmd/tailscale CLI upFlagGOOS string // if non-empty, sets TS_DEBUG_UP_FLAG_GOOS for cmd/tailscale CLI
encryptState bool encryptState bool
allowUpdates bool
mu sync.Mutex mu sync.Mutex
onLogLine []func([]byte) onLogLine []func([]byte)
@ -840,6 +841,9 @@ func (n *TestNode) StartDaemonAsIPNGOOS(ipnGOOS string) *Daemon {
"TS_DISABLE_PORTMAPPER=1", // shouldn't be needed; test is all localhost "TS_DISABLE_PORTMAPPER=1", // shouldn't be needed; test is all localhost
"TS_DEBUG_LOG_RATE=all", "TS_DEBUG_LOG_RATE=all",
) )
if n.allowUpdates {
cmd.Env = append(cmd.Env, "TS_TEST_ALLOW_AUTO_UPDATE=1")
}
if n.env.loopbackPort != nil { if n.env.loopbackPort != nil {
cmd.Env = append(cmd.Env, "TS_DEBUG_NETSTACK_LOOPBACK_PORT="+strconv.Itoa(*n.env.loopbackPort)) cmd.Env = append(cmd.Env, "TS_DEBUG_NETSTACK_LOOPBACK_PORT="+strconv.Itoa(*n.env.loopbackPort))
} }

View File

@ -25,6 +25,7 @@ import (
"slices" "slices"
"strconv" "strconv"
"strings" "strings"
"sync"
"sync/atomic" "sync/atomic"
"testing" "testing"
"time" "time"
@ -1412,14 +1413,27 @@ func TestLogoutRemovesAllPeers(t *testing.T) {
wantNode0PeerCount(expectedPeers) // all existing peers and the new node wantNode0PeerCount(expectedPeers) // all existing peers and the new node
} }
func TestAutoUpdateDefaults(t *testing.T) { func TestAutoUpdateDefaults(t *testing.T) { testAutoUpdateDefaults(t, false) }
if !feature.CanAutoUpdate() { func TestAutoUpdateDefaults_cap(t *testing.T) { testAutoUpdateDefaults(t, true) }
t.Skip("auto-updates not supported on this platform")
} // useCap is whether to use NodeAttrDefaultAutoUpdate (as opposed to the old
// DeprecatedDefaultAutoUpdate top-level MapResponse field).
func testAutoUpdateDefaults(t *testing.T, useCap bool) {
t.Cleanup(feature.HookCanAutoUpdate.SetForTest(func() bool { return true }))
tstest.Shard(t) tstest.Shard(t)
tstest.Parallel(t)
env := NewTestEnv(t) env := NewTestEnv(t)
var (
modifyMu sync.Mutex
modifyFirstMapResponse = func(*tailcfg.MapResponse, *tailcfg.MapRequest) {}
)
env.Control.ModifyFirstMapResponse = func(mr *tailcfg.MapResponse, req *tailcfg.MapRequest) {
modifyMu.Lock()
defer modifyMu.Unlock()
modifyFirstMapResponse(mr, req)
}
checkDefault := func(n *TestNode, want bool) error { checkDefault := func(n *TestNode, want bool) error {
enabled, ok := n.diskPrefs().AutoUpdate.Apply.Get() enabled, ok := n.diskPrefs().AutoUpdate.Apply.Get()
if !ok { if !ok {
@ -1431,17 +1445,23 @@ func TestAutoUpdateDefaults(t *testing.T) {
return nil return nil
} }
sendAndCheckDefault := func(t *testing.T, n *TestNode, send, want bool) { setDefaultAutoUpdate := func(send bool) {
t.Helper() modifyMu.Lock()
if !env.Control.AddRawMapResponse(n.MustStatus().Self.PublicKey, &tailcfg.MapResponse{ defer modifyMu.Unlock()
DefaultAutoUpdate: opt.NewBool(send), modifyFirstMapResponse = func(mr *tailcfg.MapResponse, req *tailcfg.MapRequest) {
}) { if mr.Node == nil {
t.Fatal("failed to send MapResponse to node") mr.Node = &tailcfg.Node{}
} }
if err := tstest.WaitFor(2*time.Second, func() error { if useCap {
return checkDefault(n, want) if mr.Node.CapMap == nil {
}); err != nil { mr.Node.CapMap = make(tailcfg.NodeCapMap)
t.Fatal(err) }
mr.Node.CapMap[tailcfg.NodeAttrDefaultAutoUpdate] = []tailcfg.RawMessage{
tailcfg.RawMessage(fmt.Sprintf("%t", send)),
}
} else {
mr.DeprecatedDefaultAutoUpdate = opt.NewBool(send)
}
} }
} }
@ -1452,29 +1472,54 @@ func TestAutoUpdateDefaults(t *testing.T) {
{ {
desc: "tailnet-default-false", desc: "tailnet-default-false",
run: func(t *testing.T, n *TestNode) { run: func(t *testing.T, n *TestNode) {
// First received default "false".
sendAndCheckDefault(t, n, false, false) // First the server sends "false", and client should remember that.
// Should not be changed even if sent "true" later. setDefaultAutoUpdate(false)
sendAndCheckDefault(t, n, true, false) n.MustUp()
n.AwaitRunning()
checkDefault(n, false)
// Now we disconnect and change the server to send "true", which
// the client should ignore, having previously remembered
// "false".
n.MustDown()
setDefaultAutoUpdate(true) // control sends default "true"
n.MustUp()
n.AwaitRunning()
checkDefault(n, false) // still false
// But can be changed explicitly by the user. // But can be changed explicitly by the user.
if out, err := n.TailscaleForOutput("set", "--auto-update").CombinedOutput(); err != nil { if out, err := n.TailscaleForOutput("set", "--auto-update").CombinedOutput(); err != nil {
t.Fatalf("failed to enable auto-update on node: %v\noutput: %s", err, out) t.Fatalf("failed to enable auto-update on node: %v\noutput: %s", err, out)
} }
sendAndCheckDefault(t, n, false, true) checkDefault(n, true)
}, },
}, },
{ {
desc: "tailnet-default-true", desc: "tailnet-default-true",
run: func(t *testing.T, n *TestNode) { run: func(t *testing.T, n *TestNode) {
// First received default "true". // Same as above but starting with default "true".
sendAndCheckDefault(t, n, true, true)
// Should not be changed even if sent "false" later. // First the server sends "true", and client should remember that.
sendAndCheckDefault(t, n, false, true) setDefaultAutoUpdate(true)
n.MustUp()
n.AwaitRunning()
checkDefault(n, true)
// Now we disconnect and change the server to send "false", which
// the client should ignore, having previously remembered
// "true".
n.MustDown()
setDefaultAutoUpdate(false) // control sends default "false"
n.MustUp()
n.AwaitRunning()
checkDefault(n, true) // still true
// But can be changed explicitly by the user. // But can be changed explicitly by the user.
if out, err := n.TailscaleForOutput("set", "--auto-update=false").CombinedOutput(); err != nil { if out, err := n.TailscaleForOutput("set", "--auto-update=false").CombinedOutput(); err != nil {
t.Fatalf("failed to disable auto-update on node: %v\noutput: %s", err, out) t.Fatalf("failed to enable auto-update on node: %v\noutput: %s", err, out)
} }
sendAndCheckDefault(t, n, true, false) checkDefault(n, false)
}, },
}, },
{ {
@ -1484,22 +1529,21 @@ func TestAutoUpdateDefaults(t *testing.T) {
if out, err := n.TailscaleForOutput("set", "--auto-update=false").CombinedOutput(); err != nil { if out, err := n.TailscaleForOutput("set", "--auto-update=false").CombinedOutput(); err != nil {
t.Fatalf("failed to disable auto-update on node: %v\noutput: %s", err, out) t.Fatalf("failed to disable auto-update on node: %v\noutput: %s", err, out)
} }
// Defaults sent from control should be ignored.
sendAndCheckDefault(t, n, true, false) setDefaultAutoUpdate(true)
sendAndCheckDefault(t, n, false, false) n.MustUp()
n.AwaitRunning()
checkDefault(n, false)
}, },
}, },
} }
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) { t.Run(tt.desc, func(t *testing.T) {
n := NewTestNode(t, env) n := NewTestNode(t, env)
n.allowUpdates = true
d := n.StartDaemon() d := n.StartDaemon()
defer d.MustCleanShutdown(t) defer d.MustCleanShutdown(t)
n.AwaitResponding() n.AwaitResponding()
n.MustUp()
n.AwaitRunning()
tt.run(t, n) tt.run(t, n)
}) })
} }

View File

@ -79,6 +79,10 @@ type Server struct {
ExplicitBaseURL string // e.g. "http://127.0.0.1:1234" with no trailing URL ExplicitBaseURL string // e.g. "http://127.0.0.1:1234" with no trailing URL
HTTPTestServer *httptest.Server // if non-nil, used to get BaseURL HTTPTestServer *httptest.Server // if non-nil, used to get BaseURL
// ModifyFirstMapResponse, if non-nil, is called exactly once per
// MapResponse stream to modify the first MapResponse sent in response to it.
ModifyFirstMapResponse func(*tailcfg.MapResponse, *tailcfg.MapRequest)
initMuxOnce sync.Once initMuxOnce sync.Once
mux *http.ServeMux mux *http.ServeMux
@ -993,6 +997,7 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi
// register an updatesCh to get updates. // register an updatesCh to get updates.
streaming := req.Stream && !req.ReadOnly streaming := req.Stream && !req.ReadOnly
compress := req.Compress != "" compress := req.Compress != ""
first := true
w.WriteHeader(200) w.WriteHeader(200)
for { for {
@ -1025,6 +1030,10 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi
if allExpired { if allExpired {
res.Node.KeyExpiry = time.Now().Add(-1 * time.Minute) res.Node.KeyExpiry = time.Now().Add(-1 * time.Minute)
} }
if f := s.ModifyFirstMapResponse; first && f != nil {
first = false
f(res, req)
}
// TODO: add minner if/when needed // TODO: add minner if/when needed
resBytes, err := json.Marshal(res) resBytes, err := json.Marshal(res)
if err != nil { if err != nil {

View File

@ -177,5 +177,5 @@ func mapResponseContainsNonPatchFields(res *tailcfg.MapResponse) bool {
// function is called, so it should never be set anyway. But for // function is called, so it should never be set anyway. But for
// completedness, and for tests, check it too: // completedness, and for tests, check it too:
res.PeersChanged != nil || res.PeersChanged != nil ||
res.DefaultAutoUpdate != "" res.DeprecatedDefaultAutoUpdate != ""
} }