fix(sch): auto hostname type detection (#3789)

* fix hostname detection * Update maintnotifications/config.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update maintnotifications/config.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix log level check * fix ip when tls is off * handle empty address --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2026-10-22 19:49:00 +03:00 · 2026-04-23 21:38:25 +03:00
parent ad21622ce8
commit 44e8b7309c
4 changed files with 287 additions and 55 deletions
--- a/example_instrumentation_test.go
+++ b/example_instrumentation_test.go
@@ -57,8 +57,8 @@ func Example_instrumentation() {
 	// finished dialing tcp :6379
 	// starting processing: <[hello 3]>
 	// finished processing: <[hello 3]>
-	// starting processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
-	// finished processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
+	// starting processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
+	// finished processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
 	// finished processing: <[ping]>
 }

@@ -80,8 +80,8 @@ func ExamplePipeline_instrumentation() {
 	// finished dialing tcp :6379
 	// starting processing: <[hello 3]>
 	// finished processing: <[hello 3]>
-	// starting processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
-	// finished processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
+	// starting processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
+	// finished processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
 	// pipeline finished processing: [[ping] [ping]]
 }

@@ -103,8 +103,8 @@ func ExampleClient_Watch_instrumentation() {
 	// finished dialing tcp :6379
 	// starting processing: <[hello 3]>
 	// finished processing: <[hello 3]>
-	// starting processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
-	// finished processing: <[client maint_notifications on moving-endpoint-type internal-fqdn]>
+	// starting processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
+	// finished processing: <[client maint_notifications on moving-endpoint-type internal-ip]>
 	// finished processing: <[watch foo]>
 	// starting processing: <[ping]>
 	// finished processing: <[ping]>
--- a/maintnotifications/config.go
+++ b/maintnotifications/config.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"net"
 	"runtime"
-	"strings"
 	"time"

 	"github.com/redis/go-redis/v9/internal"
@@ -364,20 +363,46 @@ func (c *Config) applyWorkerDefaults(poolSize int) {
 	}
 }

+// endpointDetectResolveTimeout bounds the DNS lookup performed by
+// DetectEndpointType so a slow or broken resolver cannot block client
+// construction for the full system resolver timeout (often 5-30s).
+const endpointDetectResolveTimeout = 2 * time.Second
+
+// cgnatNet is RFC6598 shared address space (100.64.0.0/10), used by many
+// cloud/carrier NATs and not covered by net.IP.IsPrivate.
+var cgnatNet = &net.IPNet{IP: net.IPv4(100, 64, 0, 0), Mask: net.CIDRMask(10, 32)}
+
+// isPrivateIP reports whether ip belongs to a range that should be treated
+// as "internal" for the purpose of endpoint type detection. It extends
+// net.IP.IsPrivate (RFC1918 + RFC4193) with loopback, link-local and
+// RFC6598 shared address space (CGNAT).
+func isPrivateIP(ip net.IP) bool {
+	if ip == nil {
+		return false
+	}
+	if ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() {
+		return true
+	}
+	if v4 := ip.To4(); v4 != nil && cgnatNet.Contains(v4) {
+		return true
+	}
+	return false
+}
+
 // DetectEndpointType automatically detects the appropriate endpoint type
 // based on the connection address and TLS configuration.
 //
-// For IP addresses:
+// TLS behaviour:
 //   - If TLS is enabled: requests FQDN for proper certificate validation
-//   - If TLS is disabled: requests IP for better performance
-//
-// For hostnames:
-//   - If TLS is enabled: always requests FQDN for proper certificate validation
-//   - If TLS is disabled: requests IP for better performance
+//     (SNI / hostname verification).
+//   - If TLS is disabled: always requests IP for better performance, even
+//     when the configured address is a hostname. In that case the hostname
+//     is resolved to determine whether it belongs to an internal or
+//     external network range.
 //
 // Internal vs External detection:
 //   - For IPs: uses private IP range detection
-//   - For hostnames: uses heuristics based on common internal naming patterns
+//   - For hostnames: resolves the hostname to an IP address and uses the IP range detection
 func DetectEndpointType(addr string, tlsEnabled bool) EndpointType {
 	// Extract host from "host:port" format
 	host, _, err := net.SplitHostPort(addr)
@@ -385,6 +410,16 @@ func DetectEndpointType(addr string, tlsEnabled bool) EndpointType {
 		host = addr // Assume no port
 	}

+	// An empty host (e.g., ":6379") conventionally means the loopback
+	// interface and is treated as internal. With TLS off we return an IP
+	// endpoint; with TLS on the caller still needs an FQDN for SNI.
+	if host == "" {
+		if tlsEnabled {
+			return EndpointTypeInternalFQDN
+		}
+		return EndpointTypeInternalIP
+	}
+
 	// Check if the host is an IP address or hostname
 	ip := net.ParseIP(host)
 	isIPAddress := ip != nil
@@ -392,7 +427,7 @@ func DetectEndpointType(addr string, tlsEnabled bool) EndpointType {

 	if isIPAddress {
 		// Address is an IP - determine if it's private or public
-		isPrivate := ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast()
+		isPrivate := isPrivateIP(ip)

 		if tlsEnabled {
 			// TLS with IP addresses - still prefer FQDN for certificate validation
@@ -410,48 +445,58 @@ func DetectEndpointType(addr string, tlsEnabled bool) EndpointType {
 			}
 		}
 	} else {
-		// Address is a hostname
-		isInternalHostname := isInternalHostname(host)
-		if isInternalHostname {
-			endpointType = EndpointTypeInternalFQDN
+		// Address is a hostname - resolve it under a bounded timeout so a
+		// slow/broken DNS server cannot stall client construction.
+		ctx, cancel := context.WithTimeout(context.Background(), endpointDetectResolveTimeout)
+		defer cancel()
+
+		isInternal, err := isInternalHostname(ctx, host)
+		// Will fallback to external classification if we can't determine
+		// whether the hostname is internal.
+		if err != nil && internal.LogLevel.WarnOrAbove() {
+			internal.Logger.Printf(ctx, "Failed to determine if hostname %q is internal: %v", host, err)
+		}
+
+		if tlsEnabled {
+			// With TLS the server name must be preserved for certificate
+			// validation, so request an FQDN endpoint.
+			if isInternal {
+				endpointType = EndpointTypeInternalFQDN
+			} else {
+				endpointType = EndpointTypeExternalFQDN
+			}
 		} else {
-			endpointType = EndpointTypeExternalFQDN
+			// Without TLS we always prefer IP endpoints for performance,
+			// even if the configured address is a hostname.
+			if isInternal {
+				endpointType = EndpointTypeInternalIP
+			} else {
+				endpointType = EndpointTypeExternalIP
+			}
 		}
 	}

 	return endpointType
 }

-// isInternalHostname determines if a hostname appears to be internal/private.
-// This is a heuristic based on common naming patterns.
-func isInternalHostname(hostname string) bool {
-	// Convert to lowercase for comparison
-	hostname = strings.ToLower(hostname)
-
-	// Common internal hostname patterns
-	internalPatterns := []string{
-		"localhost",
-		".local",
-		".internal",
-		".corp",
-		".lan",
-		".intranet",
-		".private",
+// isInternalHostname resolves the hostname (both IPv4 and IPv6) under the
+// given context and reports whether every resolved address is in a
+// private/internal range. If any address is public the hostname is treated
+// as external. A resolution error returns (false, err). An empty result set
+// returns (false, nil); callers are expected to fall back to an external
+// classification when the hostname cannot be determined to be internal.
+func isInternalHostname(ctx context.Context, hostname string) (bool, error) {
+	ips, err := net.DefaultResolver.LookupIPAddr(ctx, hostname)
+	if err != nil {
+		return false, err
 	}
-
-	// Check for exact match or suffix match
-	for _, pattern := range internalPatterns {
-		if hostname == pattern || strings.HasSuffix(hostname, pattern) {
-			return true
+	if len(ips) == 0 {
+		return false, nil
+	}
+	for _, ia := range ips {
+		if !isPrivateIP(ia.IP) {
+			return false, nil
 		}
 	}
-
-	// Check for RFC 1918 style hostnames (e.g., redis-1, db-server, etc.)
-	// If hostname doesn't contain dots, it's likely internal
-	if !strings.Contains(hostname, ".") {
-		return true
-	}
-
-	// Default to external for fully qualified domain names
-	return false
+	return true, nil
 }
--- a/maintnotifications/config_test.go
+++ b/maintnotifications/config_test.go
@@ -474,3 +474,165 @@ func TestMaxWorkersLogic(t *testing.T) {
 		}
 	})
 }
+
+
+func TestIsPrivateIP(t *testing.T) {
+	cases := []struct {
+		ip      string
+		private bool
+	}{
+		{"10.0.0.1", true},          // RFC1918
+		{"172.16.5.4", true},        // RFC1918
+		{"192.168.1.1", true},       // RFC1918
+		{"127.0.0.1", true},         // loopback
+		{"169.254.1.1", true},       // IPv4 link-local
+		{"100.64.0.1", true},        // RFC6598 CGNAT
+		{"100.127.255.254", true},   // RFC6598 CGNAT upper
+		{"100.63.255.255", false},   // just outside CGNAT
+		{"100.128.0.0", false},      // just outside CGNAT
+		{"8.8.8.8", false},          // public
+		{"1.1.1.1", false},          // public
+		{"::1", true},               // IPv6 loopback
+		{"fe80::1", true},           // IPv6 link-local
+		{"fc00::1", true},           // IPv6 ULA (RFC4193)
+		{"fd12:3456:789a::1", true}, // IPv6 ULA
+		{"2001:4860:4860::8888", false}, // IPv6 public (Google DNS)
+	}
+	for _, tc := range cases {
+		t.Run(tc.ip, func(t *testing.T) {
+			ip := net.ParseIP(tc.ip)
+			if ip == nil {
+				t.Fatalf("failed to parse %q", tc.ip)
+			}
+			if got := isPrivateIP(ip); got != tc.private {
+				t.Errorf("isPrivateIP(%s) = %v, want %v", tc.ip, got, tc.private)
+			}
+		})
+	}
+
+	if isPrivateIP(nil) {
+		t.Errorf("isPrivateIP(nil) should be false")
+	}
+}
+
+func TestDetectEndpointType_IPAddresses(t *testing.T) {
+	cases := []struct {
+		addr string
+		tls  bool
+		want EndpointType
+	}{
+		{"10.0.0.1:6379", false, EndpointTypeInternalIP},
+		{"10.0.0.1:6379", true, EndpointTypeInternalFQDN},
+		{"8.8.8.8:6379", false, EndpointTypeExternalIP},
+		{"8.8.8.8:6379", true, EndpointTypeExternalFQDN},
+		{"127.0.0.1:6379", false, EndpointTypeInternalIP},
+		{"[::1]:6379", false, EndpointTypeInternalIP},
+		{"[fc00::1]:6379", true, EndpointTypeInternalFQDN},
+		{"[2001:4860:4860::8888]:6379", false, EndpointTypeExternalIP},
+		{"100.64.0.1:6379", false, EndpointTypeInternalIP}, // CGNAT
+		// Empty host (":6379" / "") is treated as loopback/internal.
+		{":6379", false, EndpointTypeInternalIP},
+		{":6379", true, EndpointTypeInternalFQDN},
+		{"", false, EndpointTypeInternalIP},
+		{"", true, EndpointTypeInternalFQDN},
+	}
+	for _, tc := range cases {
+		t.Run(tc.addr, func(t *testing.T) {
+			got := DetectEndpointType(tc.addr, tc.tls)
+			if got != tc.want {
+				t.Errorf("DetectEndpointType(%q, tls=%v) = %q, want %q", tc.addr, tc.tls, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestDetectEndpointType_HostnameTLSOffReturnsIP(t *testing.T) {
+	// With TLS disabled, DetectEndpointType must always return an IP
+	// endpoint type, classified as internal/external based on the
+	// addresses the hostname resolves to. localhost resolves to loopback
+	// addresses which are treated as internal.
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+	if _, err := net.DefaultResolver.LookupIPAddr(ctx, "localhost"); err != nil {
+		t.Skipf("cannot resolve localhost in this environment: %v", err)
+	}
+
+	got := DetectEndpointType("localhost:6379", false)
+	if got != EndpointTypeInternalIP {
+		t.Errorf("DetectEndpointType(\"localhost:6379\", tls=false) = %q, want %q",
+			got, EndpointTypeInternalIP)
+	}
+
+	// With TLS enabled, the FQDN should still be preserved for SNI /
+	// certificate validation.
+	got = DetectEndpointType("localhost:6379", true)
+	if got != EndpointTypeInternalFQDN {
+		t.Errorf("DetectEndpointType(\"localhost:6379\", tls=true) = %q, want %q",
+			got, EndpointTypeInternalFQDN)
+	}
+}
+
+func TestIsInternalHostname_AllPrivate(t *testing.T) {
+	// localhost typically resolves to 127.0.0.1 and/or ::1; both are
+	// treated as internal.
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	got, err := isInternalHostname(ctx, "localhost")
+	if err != nil {
+		t.Skipf("cannot resolve localhost in this environment: %v", err)
+	}
+	if !got {
+		t.Errorf("isInternalHostname(\"localhost\") = false, want true")
+	}
+}
+
+func TestIsInternalHostname_ResolveError(t *testing.T) {
+	// .invalid is reserved by RFC2606 and must not resolve. A resolution
+	// error should be surfaced and the hostname should not be classified
+	// as internal.
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	got, err := isInternalHostname(ctx, "redis-does-not-exist.invalid")
+	if err == nil {
+		t.Skip("resolver unexpectedly returned success for .invalid TLD; skipping")
+	}
+	if got {
+		t.Errorf("isInternalHostname on resolution error should return false, got true")
+	}
+}
+
+func TestIsInternalHostname_ContextCancelled(t *testing.T) {
+	// A cancelled context must not leave the lookup hanging; it should
+	// return an error promptly and classify as non-internal.
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	start := time.Now()
+	got, err := isInternalHostname(ctx, "example.com")
+	if time.Since(start) > time.Second {
+		t.Errorf("isInternalHostname took too long with a cancelled context: %v", time.Since(start))
+	}
+	if err == nil {
+		t.Errorf("expected error from cancelled context, got nil")
+	}
+	if got {
+		t.Errorf("cancelled context should not classify as internal")
+	}
+}
+
+func TestDetectEndpointType_HostnameTimeoutBounded(t *testing.T) {
+	// 192.0.2.0/24 (TEST-NET-1) is reserved for documentation; the
+	// "address" below is a non-routable IPv4 literal embedded in a
+	// string that is not a valid hostname, but even if it were routed
+	// as a hostname, DetectEndpointType must return within a reasonable
+	// time bounded by endpointDetectResolveTimeout. Use a clearly
+	// invalid .invalid hostname instead to exercise the DNS path.
+	start := time.Now()
+	_ = DetectEndpointType("redis-detect-timeout-bound.invalid:6379", false)
+	if elapsed := time.Since(start); elapsed > endpointDetectResolveTimeout+time.Second {
+		t.Errorf("DetectEndpointType did not honour resolve timeout: elapsed=%v, limit=%v",
+			elapsed, endpointDetectResolveTimeout+time.Second)
+	}
+}
--- a/maintnotifications/e2e/logcollector_test.go
+++ b/maintnotifications/e2e/logcollector_test.go
@@ -98,6 +98,11 @@ type MatchFunc struct {
 	matchesMu sync.Mutex    // protects matches slice
 	found     chan struct{} // channel to notify when match is found, will be closed
 	done      func()
+	// inflight tracks spawned Printf goroutines that captured this matchFunc
+	// in their snapshot. WaitForLogMatchFunc waits on this before returning so
+	// that variables captured by F can safely be mutated by the caller after
+	// the match has been reported.
+	inflight sync.WaitGroup
 }

 func (tlc *TestLogCollector) Printf(_ context.Context, format string, v ...interface{}) {
@@ -108,14 +113,25 @@ func (tlc *TestLogCollector) Printf(_ context.Context, format string, v ...inter
 	// Use matchFuncsMutex to safely read matchFuncs
 	tlc.matchFuncsMutex.Lock()
 	hasMatchFuncs := len(tlc.matchFuncs) > 0
-	// Create a copy of matchFuncs to avoid holding the lock while processing
+	// Create a copy of matchFuncs to avoid holding the lock while processing.
+	// Register each match function as in-flight while still holding
+	// matchFuncsMutex: this is serialized with done()'s removal from the list,
+	// so no new Add can happen after a matchFunc has been removed.
 	matchFuncsCopy := make([]*MatchFunc, len(tlc.matchFuncs))
 	copy(matchFuncsCopy, tlc.matchFuncs)
+	for _, matchFunc := range matchFuncsCopy {
+		matchFunc.inflight.Add(1)
+	}
 	tlc.matchFuncsMutex.Unlock()

 	if hasMatchFuncs {
-		go func(lstr string) {
-			for _, matchFunc := range matchFuncsCopy {
+		go func(lstr string, matchFuncs []*MatchFunc) {
+			defer func() {
+				for _, matchFunc := range matchFuncs {
+					matchFunc.inflight.Done()
+				}
+			}()
+			for _, matchFunc := range matchFuncs {
 				if matchFunc.F(lstr) {
 					matchFunc.matchesMu.Lock()
 					matchFunc.matches = append(matchFunc.matches, lstr)
@@ -124,7 +140,7 @@ func (tlc *TestLogCollector) Printf(_ context.Context, format string, v ...inter
 					return
 				}
 			}
-		}(lstr)
+		}(lstr, matchFuncsCopy)
 	}
 	if tlc.doPrint {
 		fmt.Println(lstr)
@@ -220,7 +236,14 @@ func (tlc *TestLogCollector) WaitForLogMatchFunc(mf func(string) bool, timeout t
 	select {
 	case <-matchFunc.found:
 		if DebugE2E() {
-			fmt.Printf("[LOG-COLLECTOR] WaitForLogMatchFunc: match found! Acquiring matchesMu...\n")
+			fmt.Printf("[LOG-COLLECTOR] WaitForLogMatchFunc: match found! Waiting for in-flight callbacks...\n")
+		}
+		// Wait for any Printf-spawned goroutines that captured this matchFunc
+		// in their snapshot to finish. Without this, the callers F closure may
+		// still be running and reading variables the caller is about to mutate.
+		matchFunc.inflight.Wait()
+		if DebugE2E() {
+			fmt.Printf("[LOG-COLLECTOR] WaitForLogMatchFunc: in-flight drained, acquiring matchesMu...\n")
 		}
 		matchFunc.matchesMu.Lock()
 		if DebugE2E() {
@@ -241,8 +264,10 @@ func (tlc *TestLogCollector) WaitForLogMatchFunc(mf func(string) bool, timeout t
 		if DebugE2E() {
 			fmt.Printf("[LOG-COLLECTOR] WaitForLogMatchFunc: TIMEOUT after %v\n", timeout)
 		}
-		// Clean up the matchFunc from the list on timeout
+		// Clean up the matchFunc from the list on timeout, then wait for any
+		// in-flight Printf goroutines that still hold a reference to it.
 		matchFunc.done()
+		matchFunc.inflight.Wait()
 		return "", false
 	}
 }