Merge pull request #3622 from telepresenceio/thallgren/windows-simple…

…-dns

Windows DNS improvements

build-aux/main.mk

@@ -400,7 +400,11 @@ check-unit: build-deps $(tools/test-report) ## (QA) Run the test suite
 	# is only used for extensions. Therefore, we want to validate that our tests, and
 	# telepresence, run without requiring any outside dependencies.
 	set -o pipefail
+ifeq ($(GOOS),linux)
 	TELEPRESENCE_MAX_LOGFILES=300 SCOUT_DISABLE=1 TELEPRESENCE_LOGIN_DOMAIN=127.0.0.1 CGO_ENABLED=$(CGO_ENABLED) go test -json -failfast -timeout=20m ./cmd/... ./pkg/... | $(tools/test-report)
+else
+	TELEPRESENCE_MAX_LOGFILES=300 SCOUT_DISABLE=1 TELEPRESENCE_LOGIN_DOMAIN=127.0.0.1 CGO_ENABLED=$(CGO_ENABLED) go test -json -failfast -timeout=20m ./pkg/... | $(tools/test-report)
+endif
 
 .PHONY: check-integration
 ifeq ($(GOHOSTOS), linux)

cmd/traffic/cmd/manager/service.go

@@ -780,6 +780,39 @@ func (s *service) WatchDial(session *rpc.SessionInfo, stream rpc.Manager_WatchDi
 	}
 }
 
+// hasDomainSuffix checks if the given name is suffixed with the given suffix. The following
+// rules apply:
+//
+//   - The name must end with a dot.
+//   - The suffix may optionally end with a dot.
+//   - The suffix may not be empty.
+//   - The suffix match must follow after a dot in the name, or match the whole name.
+func hasDomainSuffix(name, suffix string) bool {
+	sl := len(suffix)
+	if sl == 0 {
+		return false
+	}
+	nl := len(name)
+	sfp := nl - sl
+	if sfp < 0 {
+		return false
+	}
+	if name[nl-1] != '.' {
+		return false
+	}
+	if suffix[sl-1] != '.' {
+		if sfp == 0 {
+			return false
+		}
+		sfp--
+		name = name[0 : nl-1]
+	}
+	if sfp == 0 {
+		return name == suffix
+	}
+	return name[sfp-1] == '.' && name[sfp:] == suffix
+}
+
 func (s *service) LookupDNS(ctx context.Context, request *rpc.DNSRequest) (*rpc.DNSResponse, error) {
 	ctx = managerutil.WithSessionInfo(ctx, request.GetSession())
 	qType := uint16(request.Type)
@@ -816,8 +849,8 @@ func (s *service) LookupDNS(ctx context.Context, request *rpc.DNSRequest) (*rpc.
 		dlog.Debugf(ctx, "LookupDNS on traffic-manager: %s", name)
 		rrs, rCode, err = dnsproxy.Lookup(ctx, qType, name)
 		if err != nil {
-			// Could still be x.y.<client namespace>
-			if client != nil && nDots > 1 && client.Namespace != tmNamespace && !strings.HasSuffix(name, s.ClusterInfo().ClusterDomain()) {
+			// Could still be x.y.<client namespace>, but let's avoid x.<cluster domain>.<client namespace> and x.<client-namespace>.<client namespace>
+			if client != nil && nDots > 1 && client.Namespace != tmNamespace && !hasDomainSuffix(name, s.ClusterInfo().ClusterDomain()) && !hasDomainSuffix(name, client.Namespace) {
 				name += client.Namespace + "."
 				restoreName = true
 				dlog.Debugf(ctx, "LookupDNS on traffic-manager: %s", name)

cmd/traffic/cmd/manager/service_test.go

@@ -1,4 +1,4 @@
-package manager_test
+package manager
 
 import (
 	"context"
@@ -23,7 +23,6 @@ import (
 	"github.com/datawire/dlib/dlog"
 	"github.com/datawire/k8sapi/pkg/k8sapi"
 	rpc "github.com/telepresenceio/telepresence/rpc/v2/manager"
-	"github.com/telepresenceio/telepresence/v2/cmd/traffic/cmd/manager"
 	"github.com/telepresenceio/telepresence/v2/cmd/traffic/cmd/manager/managerutil"
 	"github.com/telepresenceio/telepresence/v2/cmd/traffic/cmd/manager/mutator"
 	testdata "github.com/telepresenceio/telepresence/v2/cmd/traffic/cmd/manager/test"
@@ -329,7 +328,7 @@ func getTestClientConn(ctx context.Context, t *testing.T) *grpc.ClientConn {
 	agentmap.GeneratorConfigFunc = env.GeneratorConfig
 
 	s := grpc.NewServer()
-	mgr, g, err := manager.NewServiceFunc(ctx)
+	mgr, g, err := NewServiceFunc(ctx)
 	if err != nil {
 		t.Fatalf("failed to build manager: %v", err)
 	}
@@ -353,3 +352,74 @@ func getTestClientConn(ctx context.Context, t *testing.T) *grpc.ClientConn {
 	})
 	return conn
 }
+
+func Test_hasDomainSuffix(t *testing.T) {
+	tests := []struct {
+		name   string
+		qn     string
+		suffix string
+		want   bool
+	}{
+		{
+			"empty suffix",
+			"aa.bb.",
+			"",
+			false,
+		},
+		{
+			"suffix with dot",
+			"aa.bb.",
+			"bb.",
+			true,
+		},
+		{
+			"suffix without dot",
+			"aa.bb.",
+			"bb",
+			true,
+		},
+		{
+			"suffix partial match",
+			"aa.bb.",
+			"b.",
+			false,
+		},
+		{
+			"suffix partial match no dot",
+			"foo.bar.",
+			"b",
+			false,
+		},
+		{
+			"name without dot",
+			"aa.bb",
+			"bb",
+			false,
+		},
+		{
+			"equal",
+			"a.",
+			"a.",
+			true,
+		},
+		{
+			"equal no dot",
+			"a.",
+			"a",
+			true,
+		},
+		{
+			"empty qn",
+			".",
+			"a",
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := hasDomainSuffix(tt.qn, tt.suffix); got != tt.want {
+				t.Errorf("hasDomainSuffix() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

integration_test/multi_connect_test.go

@@ -196,7 +196,7 @@ func (s *multiConnectSuite) doubleConnectCheck(ctx1, ctx2 context.Context, n1, n
 			// condition
 			func() bool {
 				out, err := itest.Output(ctx,
-					"docker", "run", "--network", "container:"+"tp-"+cn, "--rm", "curlimages/curl", "--silent", "--max-time", "1", svc)
+					"docker", "run", "--network", "container:"+"tp-"+cn, "--rm", "curlimages/curl", "--silent", "--max-time", "2", svc)
 				if err != nil {
 					dlog.Errorf(ctx, "%s:%v", out, err)
 					return false
@@ -205,7 +205,7 @@ func (s *multiConnectSuite) doubleConnectCheck(ctx1, ctx2 context.Context, n1, n
 				return expectedOutput.MatchString(out)
 			},
 			10*time.Second, // waitFor
-			2*time.Second,  // polling interval
+			3*time.Second,  // polling interval
 			`body of %q matches %q`, "http://"+svc, expectedOutput,
 		)
 	}

pkg/client/agentpf/clients.go

@@ -79,6 +79,7 @@ func (ac *client) connect(ctx context.Context, deleteMe func()) {
 	defer close(ac.ready)
 	pfDialer := dnet.GetPortForwardDialer(ctx)
 	if pfDialer == nil {
+		ac.ready <- errors.New("no port-forward dialer configured for context")
 		return
 	}
 

pkg/client/rootd/dns/server.go

@@ -876,35 +876,49 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 		}
 	}
 
-	if strings.Contains(q.Name, tel2SubDomainDot) {
-		// This is a bogus name because it has some domain after
-		// the tel2-search domain. Should normally never happen, but
-		// will happen if someone queries for the tel2-search domain
-		// as a single label name.
-		msg.SetRcode(r, dns.RcodeNameError)
-		return
-	}
+	var answer dnsproxy.RRs
+	var rCode int
+	var err error
+
+	switch q.Qtype {
+	case dns.TypeA, dns.TypeAAAA, dns.TypeCNAME:
+		if strings.Contains(q.Name, tel2SubDomainDot) {
+			// This is a bogus name because it has some domain after
+			// the tel2-search domain. Should normally never happen, but
+			// will happen if someone queries for the tel2-search domain
+			// as a single label name.
+			msg.SetRcode(r, dns.RcodeNameError)
+			return
+		}
 
-	// try and resolve any mappings before consulting the cache, so that mapping hits don't
-	// end up in the cache.
-	answer, rCode, err := s.resolveMapping(q)
-	if err == errNoMapping {
+		// try and resolve any mappings before consulting the cache, so that mapping hits don't
+		// end up in the cache.
+		answer, rCode, err = s.resolveMapping(q)
+		if err == errNoMapping {
+			answer, rCode, err = s.resolveWithRecursionCheck(q)
+		}
+	case dns.TypePTR:
+		// Respond with cluster domain if the queried IP is the IP of this DNS server.
+		if ip, err := dnsproxy.PtrAddress(q.Name); err == nil && ip.Equal(s.remoteIP) {
+			answer = dnsproxy.RRs{
+				&dns.PTR{
+					Hdr: dnsproxy.NewHeader(q.Name, q.Qtype),
+					Ptr: s.clusterDomain,
+				},
+			}
+			rCode = dns.RcodeSuccess
+			break
+		}
+		fallthrough
+	default:
 		answer, rCode, err = s.resolveWithRecursionCheck(q)
 	}
 
 	if err == nil && rCode == dns.RcodeSuccess {
-		if rCode != dns.RcodeSuccess {
-			msg.SetRcode(r, rCode)
-		} else {
-			msg.SetReply(r)
-		}
+		msg.SetReply(r)
 		msg.Answer = answer
 		msg.Authoritative = true
-		// mac dns seems to fallback if you don't
-		// support recursion, if you have more than a
-		// single dns server, this will prevent us
-		// from intercepting all queries
-		msg.RecursionAvailable = true
+		msg.RecursionAvailable = s.fallbackPool != nil
 		txt = func() string { return answer.String() }
 		return
 	}
@@ -918,9 +932,7 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 		strings.HasPrefix(q.Name, recursionCheck2) ||
 		strings.HasSuffix(q.Name, cd) ||
 		strings.HasSuffix(origName, tel2SubDomainDot) {
-		if err == nil {
-			rCode = dns.RcodeNameError
-		} else {
+		if err != nil {
 			rCode = dns.RcodeServerFailure
 			if errors.Is(err, context.DeadlineExceeded) {
 				txt = func() string { return "timeout" }
@@ -929,17 +941,20 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 			}
 		}
 		msg.SetRcode(r, rCode)
-		return
+	} else {
+		// Use the original query name when sending things to the fallback resolver.
+		q.Name = origName
+		pfx = func() string { return fmt.Sprintf("(%s) ", s.fallbackPool.RemoteAddr()) }
+		msg, txt = s.fallbackExchange(c, msg, r)
 	}
+}
 
-	// Use original query name when sending things to the fallback resolver.
-	q.Name = origName
-	pfx = func() string { return fmt.Sprintf("(%s) ", s.fallbackPool.RemoteAddr()) }
+func (s *Server) fallbackExchange(c context.Context, msg, r *dns.Msg) (*dns.Msg, func() string) {
 	dc := &dns.Client{Net: "udp", Timeout: s.lookupTimeout}
-	var poolMsg *dns.Msg
-	poolMsg, _, err = s.fallbackPool.Exchange(c, dc, r)
+	poolMsg, _, err := s.fallbackPool.Exchange(c, dc, r)
+	var txt func() string
 	if err != nil {
-		rCode = dns.RcodeServerFailure
+		rCode := dns.RcodeServerFailure
 		txt = err.Error
 		if netErr, ok := err.(net.Error); ok {
 			switch {
@@ -953,8 +968,10 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 		msg.SetRcode(r, rCode)
 	} else {
 		msg = poolMsg
+		msg.RecursionAvailable = true
 		txt = func() string { return dnsproxy.RRs(msg.Answer).String() }
 	}
+	return msg, txt
 }
 
 var errNoMapping = errors.New("no mapping") //nolint:gochecknoglobals // constant