From 2a06fb66d0fb462585d27d2ba7957cabc61eec6a Mon Sep 17 00:00:00 2001 From: Fernando Serboncini Date: Thu, 14 May 2026 10:30:59 -0400 Subject: [PATCH] cmd/cloner: preserve nil-valued entries when cloning map (#19749) The codegen path for map-of-slice-of-pointer fields, skipped nil-valued entries. That dropped the key from the map. This broke how dns.Config.Routes uses nil values sentinels. Fixes #19730 Fixes #19732 Fixes #19746 Fixes #19744 Change-Id: Ic6400227f4ab21b3ca0e8c0eeecf9b83d145a9ab Signed-off-by: Fernando Serboncini --- cmd/cloner/cloner.go | 1 + cmd/cloner/cloner_test.go | 15 +++++ cmd/cloner/clonerex/clonerex_clone.go | 1 + cmd/viewer/tests/tests_clone.go | 6 ++ net/dns/dns_clone.go | 1 + tailcfg/tailcfg_clone.go | 1 + wgengine/userspace_test.go | 80 +++++++++++++++++++++++++++ 7 files changed, 105 insertions(+) diff --git a/cmd/cloner/cloner.go b/cmd/cloner/cloner.go index 576edd2b1..8b4cacf7a 100644 --- a/cmd/cloner/cloner.go +++ b/cmd/cloner/cloner.go @@ -176,6 +176,7 @@ func gen(buf *bytes.Buffer, it *codegen.ImportTracker, typ *types.Named) { if codegen.ContainsPointers(sliceType.Elem()) { writef("\tfor k, sv := range src.%s {", fname) writef("\t\tif sv == nil {") + writef("\t\t\tdst.%s[k] = nil", fname) writef("\t\t\tcontinue") writef("\t\t}") writef("\t\tdst.%s[k] = make([]%s, len(sv))", fname, n) diff --git a/cmd/cloner/cloner_test.go b/cmd/cloner/cloner_test.go index 834d1be7b..f8beb4a88 100644 --- a/cmd/cloner/cloner_test.go +++ b/cmd/cloner/cloner_test.go @@ -7,6 +7,7 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" "tailscale.com/cmd/cloner/clonerex" ) @@ -208,6 +209,20 @@ func TestMapSlicePointerContainer(t *testing.T) { } } +func TestMapSlicePointerContainerNilValue(t *testing.T) { + num := 7 + orig := &clonerex.MapSlicePointerContainer{ + Routes: map[string][]*clonerex.SliceContainer{ + "nil-value": nil, + "non-nil": {{Slice: []*int{&num}}}, + }, + } + cloned := orig.Clone() + if diff := cmp.Diff(orig.Routes, cloned.Routes); diff != "" { + t.Errorf("Clone() Routes mismatch (-orig +cloned):\n%s", diff) + } +} + func TestDeeplyNestedMap(t *testing.T) { num := 123 orig := &clonerex.DeeplyNestedMap{ diff --git a/cmd/cloner/clonerex/clonerex_clone.go b/cmd/cloner/clonerex/clonerex_clone.go index 9e21ff9d0..9a4413177 100644 --- a/cmd/cloner/clonerex/clonerex_clone.go +++ b/cmd/cloner/clonerex/clonerex_clone.go @@ -188,6 +188,7 @@ func (src *MapSlicePointerContainer) Clone() *MapSlicePointerContainer { dst.Routes = map[string][]*SliceContainer{} for k, sv := range src.Routes { if sv == nil { + dst.Routes[k] = nil continue } dst.Routes[k] = make([]*SliceContainer, len(sv)) diff --git a/cmd/viewer/tests/tests_clone.go b/cmd/viewer/tests/tests_clone.go index 3b4182aa0..bc576ec97 100644 --- a/cmd/viewer/tests/tests_clone.go +++ b/cmd/viewer/tests/tests_clone.go @@ -98,6 +98,7 @@ func (src *Map) Clone() *Map { dst.SlicesWithPtrs = map[string][]*StructWithPtrs{} for k, sv := range src.SlicesWithPtrs { if sv == nil { + dst.SlicesWithPtrs[k] = nil continue } dst.SlicesWithPtrs[k] = make([]*StructWithPtrs, len(sv)) @@ -114,6 +115,7 @@ func (src *Map) Clone() *Map { dst.SlicesWithoutPtrs = map[string][]*StructWithoutPtrs{} for k, sv := range src.SlicesWithoutPtrs { if sv == nil { + dst.SlicesWithoutPtrs[k] = nil continue } dst.SlicesWithoutPtrs[k] = make([]*StructWithoutPtrs, len(sv)) @@ -137,6 +139,7 @@ func (src *Map) Clone() *Map { dst.SliceIntPtr = map[string][]*int{} for k, sv := range src.SliceIntPtr { if sv == nil { + dst.SliceIntPtr[k] = nil continue } dst.SliceIntPtr[k] = make([]*int, len(sv)) @@ -431,6 +434,7 @@ func (src *GenericCloneableStruct[T, V]) Clone() *GenericCloneableStruct[T, V] { dst.SliceMap = map[string][]T{} for k, sv := range src.SliceMap { if sv == nil { + dst.SliceMap[k] = nil continue } dst.SliceMap[k] = make([]T, len(sv)) @@ -538,6 +542,7 @@ func (src *StructWithTypeAliasFields) Clone() *StructWithTypeAliasFields { dst.MapOfSlicesWithPtrs = map[string][]*StructWithPtrsAlias{} for k, sv := range src.MapOfSlicesWithPtrs { if sv == nil { + dst.MapOfSlicesWithPtrs[k] = nil continue } dst.MapOfSlicesWithPtrs[k] = make([]*StructWithPtrsAlias, len(sv)) @@ -554,6 +559,7 @@ func (src *StructWithTypeAliasFields) Clone() *StructWithTypeAliasFields { dst.MapOfSlicesWithoutPtrs = map[string][]*StructWithoutPtrsAlias{} for k, sv := range src.MapOfSlicesWithoutPtrs { if sv == nil { + dst.MapOfSlicesWithoutPtrs[k] = nil continue } dst.MapOfSlicesWithoutPtrs[k] = make([]*StructWithoutPtrsAlias, len(sv)) diff --git a/net/dns/dns_clone.go b/net/dns/dns_clone.go index 760cd514d..724e36dac 100644 --- a/net/dns/dns_clone.go +++ b/net/dns/dns_clone.go @@ -35,6 +35,7 @@ func (src *Config) Clone() *Config { dst.Routes = map[dnsname.FQDN][]*dnstype.Resolver{} for k, sv := range src.Routes { if sv == nil { + dst.Routes[k] = nil continue } dst.Routes[k] = make([]*dnstype.Resolver, len(sv)) diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index f22656306..df2d6d9aa 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -264,6 +264,7 @@ func (src *DNSConfig) Clone() *DNSConfig { dst.Routes = map[string][]*dnstype.Resolver{} for k, sv := range src.Routes { if sv == nil { + dst.Routes[k] = nil continue } dst.Routes[k] = make([]*dnstype.Resolver, len(sv)) diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 918c466c1..b2f40fada 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -9,6 +9,8 @@ import ( "net/netip" "os" "runtime" + "slices" + "sync" "testing" "go4.org/mem" @@ -17,11 +19,16 @@ import ( "tailscale.com/envknob" "tailscale.com/health" "tailscale.com/net/dns" + "tailscale.com/net/dns/resolver" "tailscale.com/net/netaddr" + "tailscale.com/net/netmon" "tailscale.com/tailcfg" + "tailscale.com/types/dnstype" "tailscale.com/types/key" + "tailscale.com/types/logger" "tailscale.com/types/netmap" "tailscale.com/types/opt" + "tailscale.com/util/dnsname" "tailscale.com/util/eventbus/eventbustest" "tailscale.com/util/usermetric" "tailscale.com/wgengine/router" @@ -530,3 +537,76 @@ func BenchmarkGenLocalAddrFunc(b *testing.B) { }) b.Logf("x = %v", x) } + +// Regression test for #19730: on major link change, MatchDomains Routes must +// be preserved. +func TestLinkChangeReapplyPreservesMagicDNSRoutes(t *testing.T) { + switch runtime.GOOS { + case "linux", "android", "darwin", "ios", "openbsd": + default: + t.Skipf("linkChange DNS reapply path not exercised on %s", runtime.GOOS) + } + + bus := eventbustest.NewBus(t) + noop, err := dns.NewNoopManager() + if err != nil { + t.Fatal(err) + } + e, err := NewUserspaceEngine(t.Logf, Config{ + HealthTracker: health.NewTracker(bus), + Metrics: new(usermetric.Registry), + EventBus: bus, + DNS: noop, + RespondToPing: true, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(e.Close) + + var ( + mu sync.Mutex + last resolver.Config + ) + e.(*userspaceEngine).dns.Resolver().TestOnlySetHook(func(cfg resolver.Config) { + mu.Lock() + defer mu.Unlock() + last = cfg + }) + snapshot := func() []dnsname.FQDN { + mu.Lock() + defer mu.Unlock() + return slices.Clone(last.LocalDomains) + } + + dnsCfg := &dns.Config{ + Routes: map[dnsname.FQDN][]*dnstype.Resolver{ + "ts.net.": {{Addr: "199.247.155.53"}}, + "foo.ts.net.": nil, + "64.100.in-addr.arpa.": nil, + }, + Hosts: map[dnsname.FQDN][]netip.Addr{ + "node.foo.ts.net.": {netip.MustParseAddr("100.64.0.5")}, + }, + SearchDomains: []dnsname.FQDN{"foo.ts.net."}, + } + if err := e.Reconfig(&wgcfg.Config{}, &router.Config{}, dnsCfg); err != nil { + t.Fatalf("Reconfig: %v", err) + } + initial := snapshot() + + cd, err := netmon.NewChangeDelta(nil, &netmon.State{HaveV4: true}, 0, true) + if err != nil { + t.Fatal(err) + } + cd.RebindLikelyRequired = true + e.(*userspaceEngine).linkChange(cd) + + after := snapshot() + slices.Sort(initial) + slices.Sort(after) + if !slices.Equal(initial, after) { + t.Errorf("resolver LocalDomains changed after linkChange:\n initial: %s\n after: %s", + logger.AsJSON(initial), logger.AsJSON(after)) + } +}