Merge pull request #3438 from telepresenceio/thallgren/restore-dns-se…

…arch

The DNS search path on Windows is now restored when Telepresence quits

CHANGELOG.yml

@@ -33,17 +33,32 @@ items:
   - version: 2.17.1
     date: (TBD)
     notes:
+      - type: bugfix
+        title: The DNS search path on Windows is now restored when Telepresence quits
+        body: >-
+          The DNS search path that Telepresence uses to simulate the DNS lookup functionality in the connected
+          cluster namespace was not removed by a <code>telepresence quit</code>, resulting in connectivity problems
+          from the workstation. Telepresence will now remove the entries that it has added to the search list when
+          it quits.
+      - type: bugfix
+        title: The user-daemon would sometimes get killed when used by multiple simultaneous CLI clients.
+        body: >-
+          The user-daemon would die with a fatal &quot;fatal error: concurrent map writes&quot; error in the
+          <code>connector.log</code>, effectively killing the ongoing connection.
       - type: bugfix
         title: Multiple services ports using the same target port would not get intercepted correctly.
-        body: Intercepts didn't work when multiple service ports were using the same container port. Telepresence would
+        body: >-
+          Intercepts didn't work when multiple service ports were using the same container port. Telepresence would
           think that one of the ports wasn't intercepted and therefore disable the intercept of the container port.
       - type: bugfix
         title: Root daemon refuses to disconnect.
-        body: The root daemon would sometimes hang forever when attempting to disconnect due to a deadlock in
+        body: >-
+          The root daemon would sometimes hang forever when attempting to disconnect due to a deadlock in
           the VIF-device.
       - type: bugfix
         title: Fix panic in user daemon when traffic-manager was unreachable
-        body: The user daemon would panic if the traffic-manager was unreachable. It will now instead report
+        body: >-
+          The user daemon would panic if the traffic-manager was unreachable. It will now instead report
           a proper error to the client.
       - type: change
         title: Removal of backward support for versions predating 2.6.0

integration_test/helm_test.go

@@ -118,7 +118,7 @@ func (s *helmSuite) Test_HelmMultipleInstalls() {
 		s.Contains(stdout, "Connected to context")
 		s.Eventually(func() bool {
 			return itest.Run(ctx, "curl", "--silent", "--connect-timeout", "1", fmt.Sprintf("%s.%s", svc, s.appSpace2)) == nil
-		}, 7*time.Second, 1*time.Second)
+		}, 15*time.Second, 3*time.Second)
 	})
 
 	s.Run("Can intercept", func() {

integration_test/restore_dns_search_windows_test.go

@@ -0,0 +1,40 @@
+package integration_test
+
+import (
+	"os"
+	"strings"
+
+	"golang.org/x/sys/windows/registry"
+
+	"github.com/telepresenceio/telepresence/v2/integration_test/itest"
+)
+
+func (s *notConnectedSuite) getGlobalSearchList() []string {
+	const (
+		tcpParamKey   = `System\CurrentControlSet\Services\Tcpip\Parameters`
+		searchListKey = `SearchList`
+	)
+	rk, err := registry.OpenKey(registry.LOCAL_MACHINE, tcpParamKey, registry.QUERY_VALUE)
+	if os.IsNotExist(err) {
+		err = nil
+	}
+	s.Require().NoError(err)
+	defer rk.Close()
+	csv, _, err := rk.GetStringValue(searchListKey)
+	if os.IsNotExist(err) {
+		err = nil
+	}
+	s.Require().NoError(err)
+	return strings.Split(csv, ",")
+}
+
+func (s *notConnectedSuite) Test_DNSSearchRestored() {
+	beforeConnect := s.getGlobalSearchList()
+	ctx := s.Context()
+	s.TelepresenceConnect(ctx)
+	afterConnect := s.getGlobalSearchList()
+	s.Assert().NotEqual(beforeConnect, afterConnect)
+	itest.TelepresenceQuitOk(ctx)
+	afterQuit := s.getGlobalSearchList()
+	s.Assert().Equal(beforeConnect, afterQuit)
+}

pkg/client/rootd/dns/server_windows.go

@@ -31,6 +31,11 @@ func (s *Server) Worker(c context.Context, dev vif.Device, configureDNS func(net
 	g := dgroup.NewGroup(c, dgroup.GroupConfig{})
 	g.Go("Server", func(c context.Context) error {
 		// No need to close listener. It's closed by the dns server.
+		defer func() {
+			c, cancel := context.WithTimeout(context.WithoutCancel(c), 5*time.Second)
+			_ = dev.SetDNS(c, s.clusterDomain, s.config.RemoteIp, nil)
+			cancel()
+		}()
 		s.processSearchPaths(g, s.updateRouterDNS, dev)
 		return s.Run(c, make(chan struct{}), []net.PacketConn{listener}, nil, s.resolveInCluster)
 	})

pkg/client/scout/os_metadata_darwin.go

@@ -10,12 +10,10 @@ import (
 	"github.com/datawire/dlib/dlog"
 )
 
-func getOsMetadata(ctx context.Context) map[string]any {
-	osMeta := map[string]any{
-		"os_version":       "unknown",
-		"os_build_version": "unknown",
-		"os_name":          "unknown",
-	}
+func setOsMetadata(ctx context.Context, osMeta map[string]any) {
+	osMeta["os_version"] = "unknown"
+	osMeta["os_build_version"] = "unknown"
+	osMeta["os_name"] = "unknown"
 	cmd := dexec.CommandContext(ctx, "sw_vers")
 	cmd.DisableLogging = true
 	if r, err := cmd.Output(); err != nil {
@@ -36,5 +34,4 @@ func getOsMetadata(ctx context.Context) map[string]any {
 			}
 		}
 	}
-	return osMeta
 }

pkg/client/scout/os_metadata_linux.go

@@ -28,8 +28,7 @@ func isWSL(ctx context.Context) bool {
 	return strings.Contains(v, "WSL") || strings.Contains(v, "Windows")
 }
 
-func getOsMetadata(ctx context.Context) map[string]any {
-	osMeta := map[string]any{}
+func setOsMetadata(ctx context.Context, osMeta map[string]any) {
 	osMeta["os_docker"] = isDocker(ctx)
 	osMeta["os_wsl"] = isWSL(ctx)
 	f, err := os.Open("/etc/os-release")
@@ -38,7 +37,7 @@ func getOsMetadata(ctx context.Context) map[string]any {
 	}
 	if err != nil {
 		dlog.Warnf(ctx, "Unable to open /etc/os-release or /usr/lib/os-release: %v", err)
-		return osMeta
+		return
 	}
 	scanner := bufio.NewScanner(f)
 	osRelease := map[string]string{}
@@ -49,7 +48,7 @@ func getOsMetadata(ctx context.Context) map[string]any {
 	}
 	if err := scanner.Err(); err != nil {
 		dlog.Warnf(ctx, "Unable to scan contents of /etc/os-release: %v", err)
-		return osMeta
+		return
 	}
 	// Different Linuxes will report things in different ways, so this will scan the
 	// contents of osRelease and look for each of the different keys that a value might be under
@@ -65,5 +64,4 @@ func getOsMetadata(ctx context.Context) map[string]any {
 	osMeta["os_name"] = getFromOSRelease("ID", "NAME")
 	osMeta["os_version"] = getFromOSRelease("VERSION", "VERSION_ID")
 	osMeta["os_build_version"] = getFromOSRelease("BUILD_ID")
-	return osMeta
 }

pkg/client/scout/os_metadata_test.go

@@ -8,7 +8,8 @@ import (
 
 func TestOsMetadata(t *testing.T) {
 	ctx := dlog.NewTestContext(t, false)
-	osMeta := getOsMetadata(ctx)
+	osMeta := make(map[string]any)
+	setOsMetadata(ctx, osMeta)
 	for _, k := range []string{"os_version", "os_name"} {
 		v := osMeta[k]
 		if v == "" || v == "unknown" {

pkg/client/scout/os_metadata_windows.go

@@ -10,14 +10,13 @@ import (
 	"github.com/telepresenceio/telepresence/v2/pkg/proc"
 )
 
-func getOsMetadata(ctx context.Context) map[string]any {
+func setOsMetadata(ctx context.Context, osMeta map[string]any) {
 	cmd := proc.CommandContext(ctx, "wmic", "os", "get", "Caption,Version,BuildNumber", "/value")
 	cmd.DisableLogging = true
 	r, err := cmd.Output()
-	osMeta := map[string]any{}
 	if err != nil {
 		dlog.Warnf(ctx, "Error running wmic: %v", err)
-		return osMeta
+		return
 	}
 	scanner := bufio.NewScanner(bytes.NewReader(r))
 	for scanner.Scan() {
@@ -42,5 +41,4 @@ func getOsMetadata(ctx context.Context) map[string]any {
 	if err := scanner.Err(); err != nil {
 		dlog.Warnf(ctx, "Unable to scan wmic output: %v", err)
 	}
-	return osMeta
 }

pkg/client/scout/reporter.go

@@ -69,9 +69,11 @@ var idFiles = map[InstallType]string{ //nolint:gochecknoglobals // constant
 	Docker: "docker_id",
 }
 
-// getInstallIDFromFilesystem returns the telepresence install ID, and also sets the reporter base
-// metadata to include any conflicting install IDs written by old versions of the product.
-func getInstallIDFromFilesystem(ctx context.Context, reporter *metriton.Reporter, installType InstallType) (string, error) {
+// setInstallIDFromFilesystem sets the telepresence install ID in the given map, including any conflicting
+// install IDs written by old versions of the product.
+//
+//nolint:gochecknoglobals // can be overridden for test purposes
+var setInstallIDFromFilesystem = func(ctx context.Context, installType InstallType, md map[string]any) (string, error) {
 	type filecacheEntry struct {
 		Body string
 		Err  error
@@ -146,7 +148,7 @@ func getInstallIDFromFilesystem(ctx context.Context, reporter *metriton.Reporter
 		}
 	}
 
-	reporter.BaseMetadata["new_install"] = len(allIDs) == 0
+	md["new_install"] = len(allIDs) == 0
 
 	// We don't want to add the extra ids until we've decided if it's a new install or not
 	// this is because we'd like a new install of type A to be reported even if there's already
@@ -167,7 +169,7 @@ func getInstallIDFromFilesystem(ctx context.Context, reporter *metriton.Reporter
 
 	for product, id := range allIDs {
 		if id != retID {
-			reporter.BaseMetadata["install_id_"+product] = id
+			md["install_id_"+product] = id
 		}
 	}
 	return retID, nil
@@ -178,23 +180,28 @@ func getInstallIDFromFilesystem(ctx context.Context, reporter *metriton.Reporter
 const bufferSize = 40
 
 func NewReporterForInstallType(ctx context.Context, mode string, installType InstallType, reportAnnotators []ReportAnnotator, reportMutators []ReportMutator) Reporter {
-	r := &reporter{
-		reporter: &metriton.Reporter{
-			Application: "telepresence2",
-			Version:     client.Version(),
-			GetInstallID: func(r *metriton.Reporter) (string, error) {
-				id, err := getInstallIDFromFilesystem(ctx, r, installType)
-				if err != nil {
-					id = "00000000-0000-0000-0000-000000000000"
-					r.BaseMetadata["new_install"] = true
-					r.BaseMetadata["install_id_error"] = err.Error()
-				}
-				return id, nil
-			},
-		},
-		reportAnnotators: reportAnnotators,
-		reportMutators:   reportMutators,
+	md := make(map[string]any, 12)
+	setOsMetadata(ctx, md)
+	installID, err := setInstallIDFromFilesystem(ctx, installType, md)
+	if err != nil {
+		installID = "00000000-0000-0000-0000-000000000000"
+		md["new_install"] = true
+		md["install_id_error"] = err.Error()
 	}
+	// Fixed (growing) metadata passed with every report
+	md["mode"] = mode
+	md["trace_id"] = uuid.NewString() //  It's sent as JSON so might as well convert it to a string once here.
+	md["goos"] = runtime.GOOS
+	md["goarch"] = runtime.GOARCH
+
+	// Discover how Telepresence was installed based on the binary's location
+	installMethod, err := client.GetInstallMechanism()
+	if err != nil {
+		dlog.Errorf(ctx, "scout error getting executable: %s", err)
+	}
+	md["install_method"] = installMethod
+	setDefaultEnvironmentMetadata(md)
+
 	if env := client.GetEnv(ctx); env != nil && !env.ScoutDisable {
 		// Some tests disable scout reporting by setting the host IP to 127.0.0.1. This spams
 		// the logs with lots of "connection refused" messages and makes them hard to read.
@@ -208,8 +215,21 @@ func NewReporterForInstallType(ctx context.Context, mode string, installType Ins
 			}
 		}
 	}
-	r.initialize(ctx, mode, runtime.GOOS, runtime.GOARCH)
-	return r
+
+	return &reporter{
+		reporter: &metriton.Reporter{
+			Application: "telepresence2",
+			Version:     client.Version(),
+			GetInstallID: func(r *metriton.Reporter) (string, error) {
+				return installID, nil
+			},
+			BaseMetadata: md,
+		},
+		reportAnnotators: reportAnnotators,
+		reportMutators:   reportMutators,
+		buffer:           make(chan bufEntry, bufferSize),
+		done:             make(chan struct{}),
+	}
 }
 
 // DefaultReportAnnotators are the default annotator functions that the NewReporter function will pass to NewReporterForInstallType.
@@ -267,30 +287,6 @@ func SetMetadatum(ctx context.Context, key string, value any) {
 	}
 }
 
-// initialization broken out or constructor for the benefit of testing.
-func (r *reporter) initialize(ctx context.Context, mode, goos, goarch string) {
-	r.buffer = make(chan bufEntry, bufferSize)
-	r.done = make(chan struct{})
-
-	// Fixed (growing) metadata passed with every report
-	baseMeta := getOsMetadata(ctx)
-	baseMeta["mode"] = mode
-	baseMeta["trace_id"] = uuid.NewString() //  It's sent as JSON so might as well convert it to a string once here.
-	baseMeta["goos"] = goos
-	baseMeta["goarch"] = goarch
-
-	// Discover how Telepresence was installed based on the binary's location
-	installMethod, err := client.GetInstallMechanism()
-	if err != nil {
-		dlog.Errorf(ctx, "scout error getting executable: %s", err)
-	}
-	baseMeta["install_method"] = installMethod
-	for k, v := range getDefaultEnvironmentMetadata() {
-		baseMeta[k] = v
-	}
-	r.reporter.BaseMetadata = baseMeta
-}
-
 func (r *reporter) InstallID() string {
 	return r.reporter.InstallID()
 }
@@ -399,15 +395,13 @@ func (r *reporter) doReport(ctx context.Context, be *bufEntry) {
 	}
 }
 
-// Returns a metadata map containing all the additional environment variables to be reported.
-func getDefaultEnvironmentMetadata() map[string]string {
-	metadata := map[string]string{}
+// setDefaultEnvironmentMetadata sets all the additional environment variables to be reported.
+func setDefaultEnvironmentMetadata(metadata map[string]any) {
 	for _, e := range os.Environ() {
 		pair := strings.SplitN(e, "=", 2)
 		if strings.HasPrefix(pair[0], EnvironmentMetadataPrefix) {
 			key := strings.ToLower(strings.TrimPrefix(pair[0], EnvironmentMetadataPrefix))
 			metadata[key] = pair[1]
 		}
 	}
-	return metadata
 }