From 44e8b7309cf547f620b9849a5a4dd9013cc3660a Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> Date: Thu, 23 Apr 2026 21:38:25 +0300 Subject: [PATCH] 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> --- example_instrumentation_test.go | 12 +- maintnotifications/config.go | 131 ++++++++++------ maintnotifications/config_test.go | 162 ++++++++++++++++++++ maintnotifications/e2e/logcollector_test.go | 37 ++++- 4 files changed, 287 insertions(+), 55 deletions(-) diff --git a/example_instrumentation_test.go b/example_instrumentation_test.go index fa776fcf..76a67789 100644 --- 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]> diff --git a/maintnotifications/config.go b/maintnotifications/config.go index db666f3a..70d5acdc 100644 --- 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 } diff --git a/maintnotifications/config_test.go b/maintnotifications/config_test.go index a1e664ea..3dfdcfd1 100644 --- 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) + } +} diff --git a/maintnotifications/e2e/logcollector_test.go b/maintnotifications/e2e/logcollector_test.go index 98881a34..9214e216 100644 --- 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 } }