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 <fserb@tailscale.com>

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)

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{

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))

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))

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))

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))

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))
+	}
+}