From dfc2667f8f35cd0496c7dd732b7b042ea5c9a1e6 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 20 Apr 2026 00:40:02 +0000 Subject: [PATCH] tstest/integration/testcontrol: make Stream w/ capver >= 68 match docs, prod testcontrol wasn't following the document specs (and prod behavior) breaking a WIP integration test elsewhere. Updates tailscale/corp#40088 Change-Id: I02cf70894346bad7c85940b617d99c21c5310664 Signed-off-by: Brad Fitzpatrick --- tstest/integration/testcontrol/testcontrol.go | 9 +- .../testcontrol/testcontrol_test.go | 132 ++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tstest/integration/testcontrol/testcontrol_test.go diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index 53d9137c4..f16fc89b5 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/tstest/integration/testcontrol/testcontrol.go @@ -1157,8 +1157,15 @@ func (s *Server) serveMap(w http.ResponseWriter, r *http.Request, mkey key.Machi return } + // Per tailcfg.MapRequest.Stream docs: if Stream is true and Version >= 68, + // the server must treat this as read-only and ignore Hostinfo, Endpoints, + // DiscoKey, etc. — modern clients send those via a separate non-streaming + // POST /machine/map from a dedicated updateRoutine, not piggybacked on the + // streaming poll. Without this, the streaming MapRequest's zero-valued + // DiscoKey/Endpoints clobber whatever was just pushed out-of-band. + streamingNonUpdate := req.Stream && req.Version >= 68 var peersToUpdate []tailcfg.NodeID - if !req.ReadOnly { + if !req.ReadOnly && !streamingNonUpdate { endpoints := filterInvalidIPv6Endpoints(req.Endpoints) node.Endpoints = endpoints node.DiscoKey = req.DiscoKey diff --git a/tstest/integration/testcontrol/testcontrol_test.go b/tstest/integration/testcontrol/testcontrol_test.go new file mode 100644 index 000000000..d3008cdb7 --- /dev/null +++ b/tstest/integration/testcontrol/testcontrol_test.go @@ -0,0 +1,132 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +package testcontrol_test + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "tailscale.com/control/ts2021" + "tailscale.com/control/tsp" + "tailscale.com/net/tsdial" + "tailscale.com/tailcfg" + "tailscale.com/tstest/integration/testcontrol" + "tailscale.com/types/key" + "tailscale.com/util/must" +) + +// TestStreamingMapReqReadOnlyByVersion verifies that testcontrol matches +// production control's streaming-is-read-only semantics for clients at +// capability version >= 68. Per tailcfg.MapRequest.Stream docs, a streaming +// MapRequest from a cap>=68 client must be treated as read-only by the +// server (Endpoints/Hostinfo/DiscoKey are sent separately via a non-streaming +// /machine/map call), so the streaming MapRequest's zero-valued DiscoKey +// must not clobber the node's currently stored DiscoKey. +// +// For older (cap<68) clients, the streaming MapRequest is still a write and +// writes do happen, so DiscoKey=zero in the request does clobber. +func TestStreamingMapReqReadOnlyByVersion(t *testing.T) { + tests := []struct { + version tailcfg.CapabilityVersion + wantClobber bool + }{ + {67, true}, // pre-cap-68: streaming is a write, DiscoKey=zero clobbers. + {68, false}, // cap>=68: streaming is read-only, DiscoKey unchanged. + } + + for _, tt := range tests { + t.Run(fmt.Sprintf("v%d", tt.version), func(t *testing.T) { + ctrl := &testcontrol.Server{} + ctrl.HTTPTestServer = httptest.NewUnstartedServer(ctrl) + ctrl.HTTPTestServer.Start() + t.Cleanup(ctrl.HTTPTestServer.Close) + baseURL := ctrl.HTTPTestServer.URL + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + serverKey := must.Get(tsp.DiscoverServerKey(ctx, baseURL)) + + // Register a node and push a known DiscoKey via SendMapUpdate + // (a non-streaming, unambiguously-a-write request). + nodeKey := key.NewNode() + machineKey := key.NewMachine() + wantDisco := key.NewDisco().Public() + + tc := must.Get(tsp.NewClient(tsp.ClientOpts{ + ServerURL: baseURL, + MachineKey: machineKey, + })) + defer tc.Close() + tc.SetControlPublicKey(serverKey) + must.Get(tc.Register(ctx, tsp.RegisterOpts{ + NodeKey: nodeKey, + Hostinfo: &tailcfg.Hostinfo{Hostname: "target"}, + })) + if err := tc.SendMapUpdate(ctx, tsp.SendMapUpdateOpts{ + NodeKey: nodeKey, + DiscoKey: wantDisco, + Hostinfo: &tailcfg.Hostinfo{Hostname: "target"}, + }); err != nil { + t.Fatalf("SendMapUpdate: %v", err) + } + if n := ctrl.Node(nodeKey.Public()); n == nil || n.DiscoKey != wantDisco { + t.Fatalf("pre: DiscoKey not set; node=%+v", n) + } + + // Fire a streaming MapRequest with the chosen Version and a + // zero DiscoKey. Use ts2021 directly because tsp.Map hardcodes + // Version to tailcfg.CurrentCapabilityVersion. + nc := must.Get(ts2021.NewClient(ts2021.ClientOpts{ + ServerURL: baseURL, + PrivKey: machineKey, + ServerPubKey: serverKey, + Dialer: tsdial.NewFromFuncForDebug(t.Logf, (&net.Dialer{}).DialContext), + })) + defer nc.Close() + + body := must.Get(json.Marshal(&tailcfg.MapRequest{ + Version: tt.version, + NodeKey: nodeKey.Public(), + Stream: true, + // DiscoKey intentionally zero. + })) + reqURL := strings.Replace(baseURL+"/machine/map", "http:", "https:", 1) + reqCtx, reqCancel := context.WithCancel(ctx) + defer reqCancel() + req := must.Get(http.NewRequestWithContext(reqCtx, "POST", reqURL, bytes.NewReader(body))) + ts2021.AddLBHeader(req, nodeKey.Public()) + + // nc.Do returns once response headers arrive, which in + // testcontrol's serveMap is AFTER the write branch has run + // (or been skipped). So by the time this returns, any write + // this request is going to do has already happened. + res, err := nc.Do(req) + if err != nil { + t.Fatalf("nc.Do: %v", err) + } + res.Body.Close() // tears down the streaming session server-side + + got := ctrl.Node(nodeKey.Public()) + if got == nil { + t.Fatal("node disappeared") + } + switch { + case tt.wantClobber && !got.DiscoKey.IsZero(): + t.Errorf("v%d: expected DiscoKey clobbered to zero, got %v", tt.version, got.DiscoKey) + case !tt.wantClobber && got.DiscoKey != wantDisco: + t.Errorf("v%d: DiscoKey changed from %v to %v; should have been left alone", + tt.version, wantDisco, got.DiscoKey) + } + }) + } +}