[client] Support dns upstream failover for nameserver groups with same match domain (#3178)

client/internal/dns/server_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/miekg/dns"
 	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
 
@@ -88,6 +89,18 @@ func init() {
 	formatter.SetTextFormatter(log.StandardLogger())
 }
 
+func generateDummyHandler(domain string, servers []nbdns.NameServer) *upstreamResolverBase {
+	var srvs []string
+	for _, srv := range servers {
+		srvs = append(srvs, getNSHostPort(srv))
+	}
+	return &upstreamResolverBase{
+		domain:          domain,
+		upstreamServers: srvs,
+		cancel:          func() {},
+	}
+}
+
 func TestUpdateDNSServer(t *testing.T) {
 	nameServers := []nbdns.NameServer{
 		{
@@ -140,15 +153,37 @@ func TestUpdateDNSServer(t *testing.T) {
 					},
 				},
 			},
-			expectedUpstreamMap: registeredHandlerMap{"netbird.io": dummyHandler, "netbird.cloud": dummyHandler, nbdns.RootZone: dummyHandler},
-			expectedLocalMap:    registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}},
+			expectedUpstreamMap: registeredHandlerMap{
+				generateDummyHandler("netbird.io", nameServers).id(): handlerWrapper{
+					domain:   "netbird.io",
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+				dummyHandler.id(): handlerWrapper{
+					domain:   "netbird.cloud",
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+				generateDummyHandler(".", nameServers).id(): handlerWrapper{
+					domain:   nbdns.RootZone,
+					handler:  dummyHandler,
+					priority: PriorityDefault,
+				},
+			},
+			expectedLocalMap: registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}},
 		},
 		{
-			name:            "New Config Should Succeed",
-			initLocalMap:    registrationMap{"netbird.cloud": struct{}{}},
-			initUpstreamMap: registeredHandlerMap{buildRecordKey(zoneRecords[0].Name, 1, 1): dummyHandler},
-			initSerial:      0,
-			inputSerial:     1,
+			name:         "New Config Should Succeed",
+			initLocalMap: registrationMap{"netbird.cloud": struct{}{}},
+			initUpstreamMap: registeredHandlerMap{
+				generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{
+					domain:   buildRecordKey(zoneRecords[0].Name, 1, 1),
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+			},
+			initSerial:  0,
+			inputSerial: 1,
 			inputUpdate: nbdns.Config{
 				ServiceEnable: true,
 				CustomZones: []nbdns.CustomZone{
@@ -164,8 +199,19 @@ func TestUpdateDNSServer(t *testing.T) {
 					},
 				},
 			},
-			expectedUpstreamMap: registeredHandlerMap{"netbird.io": dummyHandler, "netbird.cloud": dummyHandler},
-			expectedLocalMap:    registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}},
+			expectedUpstreamMap: registeredHandlerMap{
+				generateDummyHandler("netbird.io", nameServers).id(): handlerWrapper{
+					domain:   "netbird.io",
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+				"local-resolver": handlerWrapper{
+					domain:   "netbird.cloud",
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+			},
+			expectedLocalMap: registrationMap{buildRecordKey(zoneRecords[0].Name, 1, 1): struct{}{}},
 		},
 		{
 			name:            "Smaller Config Serial Should Be Skipped",
@@ -242,9 +288,15 @@ func TestUpdateDNSServer(t *testing.T) {
 			shouldFail: true,
 		},
 		{
-			name:                "Empty Config Should Succeed and Clean Maps",
-			initLocalMap:        registrationMap{"netbird.cloud": struct{}{}},
-			initUpstreamMap:     registeredHandlerMap{zoneRecords[0].Name: dummyHandler},
+			name:         "Empty Config Should Succeed and Clean Maps",
+			initLocalMap: registrationMap{"netbird.cloud": struct{}{}},
+			initUpstreamMap: registeredHandlerMap{
+				generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{
+					domain:   zoneRecords[0].Name,
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+			},
 			initSerial:          0,
 			inputSerial:         1,
 			inputUpdate:         nbdns.Config{ServiceEnable: true},
@@ -252,9 +304,15 @@ func TestUpdateDNSServer(t *testing.T) {
 			expectedLocalMap:    make(registrationMap),
 		},
 		{
-			name:                "Disabled Service Should clean map",
-			initLocalMap:        registrationMap{"netbird.cloud": struct{}{}},
-			initUpstreamMap:     registeredHandlerMap{zoneRecords[0].Name: dummyHandler},
+			name:         "Disabled Service Should clean map",
+			initLocalMap: registrationMap{"netbird.cloud": struct{}{}},
+			initUpstreamMap: registeredHandlerMap{
+				generateDummyHandler(zoneRecords[0].Name, nameServers).id(): handlerWrapper{
+					domain:   zoneRecords[0].Name,
+					handler:  dummyHandler,
+					priority: PriorityMatchDomain,
+				},
+			},
 			initSerial:          0,
 			inputSerial:         1,
 			inputUpdate:         nbdns.Config{ServiceEnable: false},
@@ -421,7 +479,13 @@ func TestDNSFakeResolverHandleUpdates(t *testing.T) {
 		}
 	}()
 
-	dnsServer.dnsMuxMap = registeredHandlerMap{zoneRecords[0].Name: &localResolver{}}
+	dnsServer.dnsMuxMap = registeredHandlerMap{
+		"id1": handlerWrapper{
+			domain:   zoneRecords[0].Name,
+			handler:  &localResolver{},
+			priority: PriorityMatchDomain,
+		},
+	}
 	dnsServer.localResolver.registeredMap = registrationMap{"netbird.cloud": struct{}{}}
 	dnsServer.updateSerial = 0
 
@@ -562,9 +626,8 @@ func TestDNSServerUpstreamDeactivateCallback(t *testing.T) {
 		localResolver: &localResolver{
 			registeredMap: make(registrationMap),
 		},
-		handlerChain:      NewHandlerChain(),
-		handlerPriorities: make(map[string]int),
-		hostManager:       hostManager,
+		handlerChain: NewHandlerChain(),
+		hostManager:  hostManager,
 		currentConfig: HostDNSConfig{
 			Domains: []DomainConfig{
 				{false, "domain0", false},
@@ -593,7 +656,7 @@ func TestDNSServerUpstreamDeactivateCallback(t *testing.T) {
 		NameServers: []nbdns.NameServer{
 			{IP: netip.MustParseAddr("8.8.0.0"), NSType: nbdns.UDPNameServerType, Port: 53},
 		},
-	}, nil)
+	}, nil, 0)
 
 	deactivate(nil)
 	expected := "domain0,domain2"
@@ -903,8 +966,8 @@ func TestHandlerChain_DomainPriorities(t *testing.T) {
 		Subdomains: true,
 	}
 
-	chain.AddHandler("example.com.", dnsRouteHandler, PriorityDNSRoute, nil)
-	chain.AddHandler("example.com.", upstreamHandler, PriorityMatchDomain, nil)
+	chain.AddHandler("example.com.", dnsRouteHandler, PriorityDNSRoute)
+	chain.AddHandler("example.com.", upstreamHandler, PriorityMatchDomain)
 
 	testCases := []struct {
 		name            string
@@ -959,3 +1022,421 @@ func TestHandlerChain_DomainPriorities(t *testing.T) {
 		})
 	}
 }
+
+type mockHandler struct {
+	Id string
+}
+
+func (m *mockHandler) ServeDNS(dns.ResponseWriter, *dns.Msg) {}
+func (m *mockHandler) stop()                                 {}
+func (m *mockHandler) probeAvailability()                    {}
+func (m *mockHandler) id() handlerID                         { return handlerID(m.Id) }
+
+type mockService struct{}
+
+func (m *mockService) Listen() error                   { return nil }
+func (m *mockService) Stop()                           {}
+func (m *mockService) RuntimeIP() string               { return "127.0.0.1" }
+func (m *mockService) RuntimePort() int                { return 53 }
+func (m *mockService) RegisterMux(string, dns.Handler) {}
+func (m *mockService) DeregisterMux(string)            {}
+
+func TestDefaultServer_UpdateMux(t *testing.T) {
+	baseMatchHandlers := registeredHandlerMap{
+		"upstream-group1": {
+			domain: "example.com",
+			handler: &mockHandler{
+				Id: "upstream-group1",
+			},
+			priority: PriorityMatchDomain,
+		},
+		"upstream-group2": {
+			domain: "example.com",
+			handler: &mockHandler{
+				Id: "upstream-group2",
+			},
+			priority: PriorityMatchDomain - 1,
+		},
+	}
+
+	baseRootHandlers := registeredHandlerMap{
+		"upstream-root1": {
+			domain: ".",
+			handler: &mockHandler{
+				Id: "upstream-root1",
+			},
+			priority: PriorityDefault,
+		},
+		"upstream-root2": {
+			domain: ".",
+			handler: &mockHandler{
+				Id: "upstream-root2",
+			},
+			priority: PriorityDefault - 1,
+		},
+	}
+
+	baseMixedHandlers := registeredHandlerMap{
+		"upstream-group1": {
+			domain: "example.com",
+			handler: &mockHandler{
+				Id: "upstream-group1",
+			},
+			priority: PriorityMatchDomain,
+		},
+		"upstream-group2": {
+			domain: "example.com",
+			handler: &mockHandler{
+				Id: "upstream-group2",
+			},
+			priority: PriorityMatchDomain - 1,
+		},
+		"upstream-other": {
+			domain: "other.com",
+			handler: &mockHandler{
+				Id: "upstream-other",
+			},
+			priority: PriorityMatchDomain,
+		},
+	}
+
+	tests := []struct {
+		name             string
+		initialHandlers  registeredHandlerMap
+		updates          []handlerWrapper
+		expectedHandlers map[string]string // map[handlerID]domain
+		description      string
+	}{
+		{
+			name:            "Remove group1 from update",
+			initialHandlers: baseMatchHandlers,
+			updates: []handlerWrapper{
+				// Only group2 remains
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group2",
+					},
+					priority: PriorityMatchDomain - 1,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group2": "example.com",
+			},
+			description: "When group1 is not included in the update, it should be removed while group2 remains",
+		},
+		{
+			name:            "Remove group2 from update",
+			initialHandlers: baseMatchHandlers,
+			updates: []handlerWrapper{
+				// Only group1 remains
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group1",
+					},
+					priority: PriorityMatchDomain,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group1": "example.com",
+			},
+			description: "When group2 is not included in the update, it should be removed while group1 remains",
+		},
+		{
+			name:            "Add group3 in first position",
+			initialHandlers: baseMatchHandlers,
+			updates: []handlerWrapper{
+				// Add group3 with highest priority
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group3",
+					},
+					priority: PriorityMatchDomain + 1,
+				},
+				// Keep existing groups with their original priorities
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group1",
+					},
+					priority: PriorityMatchDomain,
+				},
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group2",
+					},
+					priority: PriorityMatchDomain - 1,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group1": "example.com",
+				"upstream-group2": "example.com",
+				"upstream-group3": "example.com",
+			},
+			description: "When adding group3 with highest priority, it should be first in chain while maintaining existing groups",
+		},
+		{
+			name:            "Add group3 in last position",
+			initialHandlers: baseMatchHandlers,
+			updates: []handlerWrapper{
+				// Keep existing groups with their original priorities
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group1",
+					},
+					priority: PriorityMatchDomain,
+				},
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group2",
+					},
+					priority: PriorityMatchDomain - 1,
+				},
+				// Add group3 with lowest priority
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group3",
+					},
+					priority: PriorityMatchDomain - 2,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group1": "example.com",
+				"upstream-group2": "example.com",
+				"upstream-group3": "example.com",
+			},
+			description: "When adding group3 with lowest priority, it should be last in chain while maintaining existing groups",
+		},
+		// Root zone tests
+		{
+			name:            "Remove root1 from update",
+			initialHandlers: baseRootHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root2",
+					},
+					priority: PriorityDefault - 1,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-root2": ".",
+			},
+			description: "When root1 is not included in the update, it should be removed while root2 remains",
+		},
+		{
+			name:            "Remove root2 from update",
+			initialHandlers: baseRootHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root1",
+					},
+					priority: PriorityDefault,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-root1": ".",
+			},
+			description: "When root2 is not included in the update, it should be removed while root1 remains",
+		},
+		{
+			name:            "Add root3 in first position",
+			initialHandlers: baseRootHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root3",
+					},
+					priority: PriorityDefault + 1,
+				},
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root1",
+					},
+					priority: PriorityDefault,
+				},
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root2",
+					},
+					priority: PriorityDefault - 1,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-root1": ".",
+				"upstream-root2": ".",
+				"upstream-root3": ".",
+			},
+			description: "When adding root3 with highest priority, it should be first in chain while maintaining existing root handlers",
+		},
+		{
+			name:            "Add root3 in last position",
+			initialHandlers: baseRootHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root1",
+					},
+					priority: PriorityDefault,
+				},
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root2",
+					},
+					priority: PriorityDefault - 1,
+				},
+				{
+					domain: ".",
+					handler: &mockHandler{
+						Id: "upstream-root3",
+					},
+					priority: PriorityDefault - 2,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-root1": ".",
+				"upstream-root2": ".",
+				"upstream-root3": ".",
+			},
+			description: "When adding root3 with lowest priority, it should be last in chain while maintaining existing root handlers",
+		},
+		// Mixed domain tests
+		{
+			name:            "Update with mixed domains - remove one of duplicate domain",
+			initialHandlers: baseMixedHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group1",
+					},
+					priority: PriorityMatchDomain,
+				},
+				{
+					domain: "other.com",
+					handler: &mockHandler{
+						Id: "upstream-other",
+					},
+					priority: PriorityMatchDomain,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group1": "example.com",
+				"upstream-other":  "other.com",
+			},
+			description: "When updating mixed domains, should correctly handle removal of one duplicate while maintaining other domains",
+		},
+		{
+			name:            "Update with mixed domains - add new domain",
+			initialHandlers: baseMixedHandlers,
+			updates: []handlerWrapper{
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group1",
+					},
+					priority: PriorityMatchDomain,
+				},
+				{
+					domain: "example.com",
+					handler: &mockHandler{
+						Id: "upstream-group2",
+					},
+					priority: PriorityMatchDomain - 1,
+				},
+				{
+					domain: "other.com",
+					handler: &mockHandler{
+						Id: "upstream-other",
+					},
+					priority: PriorityMatchDomain,
+				},
+				{
+					domain: "new.com",
+					handler: &mockHandler{
+						Id: "upstream-new",
+					},
+					priority: PriorityMatchDomain,
+				},
+			},
+			expectedHandlers: map[string]string{
+				"upstream-group1": "example.com",
+				"upstream-group2": "example.com",
+				"upstream-other":  "other.com",
+				"upstream-new":    "new.com",
+			},
+			description: "When updating mixed domains, should maintain existing duplicates and add new domain",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			server := &DefaultServer{
+				dnsMuxMap:    tt.initialHandlers,
+				handlerChain: NewHandlerChain(),
+				service:      &mockService{},
+			}
+
+			// Perform the update
+			server.updateMux(tt.updates)
+
+			// Verify the results
+			assert.Equal(t, len(tt.expectedHandlers), len(server.dnsMuxMap),
+				"Number of handlers after update doesn't match expected")
+
+			// Check each expected handler
+			for id, expectedDomain := range tt.expectedHandlers {
+				handler, exists := server.dnsMuxMap[handlerID(id)]
+				assert.True(t, exists, "Expected handler %s not found", id)
+				if exists {
+					assert.Equal(t, expectedDomain, handler.domain,
+						"Domain mismatch for handler %s", id)
+				}
+			}
+
+			// Verify no unexpected handlers exist
+			for handlerID := range server.dnsMuxMap {
+				_, expected := tt.expectedHandlers[string(handlerID)]
+				assert.True(t, expected, "Unexpected handler found: %s", handlerID)
+			}
+
+			// Verify the handlerChain state and order
+			previousPriority := 0
+			for _, chainEntry := range server.handlerChain.handlers {
+				// Verify priority order
+				if previousPriority > 0 {
+					assert.True(t, chainEntry.Priority <= previousPriority,
+						"Handlers in chain not properly ordered by priority")
+				}
+				previousPriority = chainEntry.Priority
+
+				// Verify handler exists in mux
+				foundInMux := false
+				for _, muxEntry := range server.dnsMuxMap {
+					if chainEntry.Handler == muxEntry.handler &&
+						chainEntry.Priority == muxEntry.priority &&
+						chainEntry.Pattern == dns.Fqdn(muxEntry.domain) {
+						foundInMux = true
+						break
+					}
+				}
+				assert.True(t, foundInMux,
+					"Handler in chain not found in dnsMuxMap")
+			}
+		})
+	}
+}