From 4186a66bf12771a4e4e681cd7c597a5f1bb729ef Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Sat, 27 Sep 2025 17:54:08 +0530 Subject: [PATCH 01/11] add default port --- .../delegatingresolver/delegatingresolver.go | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index 20b8fb098ac3..60a6a5d664dd 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -22,12 +22,14 @@ package delegatingresolver import ( "fmt" + "net" "net/http" "net/url" "sync" "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/proxyattributes" + "google.golang.org/grpc/internal/resolver/dns/internal" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/internal/transport/networktype" "google.golang.org/grpc/resolver" @@ -40,6 +42,8 @@ var ( HTTPSProxyFromEnvironment = http.ProxyFromEnvironment ) +const defaultPort = "443" + // delegatingResolver manages both target URI and proxy address resolution by // delegating these tasks to separate child resolvers. Essentially, it acts as // an intermediary between the gRPC ClientConn and the child resolvers. @@ -131,6 +135,10 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti // resolution should be handled by the proxy, not the client. Therefore, we // bypass the target resolver and store the unresolved target address. if target.URL.Scheme == "dns" && !targetResolutionEnabled { + add, err := maybeAddDefaultPort(target.Endpoint(), defaultPort) + if err != nil { + return nil, fmt.Errorf("delegating_resolver: invalid target address %s: %v", target.Endpoint(), err) + } r.targetResolverState = &resolver.State{ Addresses: []resolver.Address{{Addr: target.Endpoint()}}, Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: target.Endpoint()}}}}, @@ -202,6 +210,31 @@ func needsProxyResolver(state *resolver.State) bool { return false } +func maybeAddDefaultPort(target, defaultPort string) (string, error) { + if target == "" { + return "", internal.ErrMissingAddr + } + if host, port, err := net.SplitHostPort(target); err == nil { + if port == "" { + // If the port field is empty (target ends with colon), e.g. "[::1]:", + // this is an error. + return "", internal.ErrEndsWithColon + } + // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port + if host == "" { + // Keep consistent with net.Dial(): If the host is empty, as in ":80", + // the local system is assumed. + host = "localhost" + } + return "", internal.ErrMissingAddr + } + if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { + // target doesn't have port + return fmt.Sprintf("%s:%s", host, port), nil + } + return "", fmt.Errorf("invalid target address %v", target) +} + func skipProxy(address resolver.Address) bool { // Avoid proxy when network is not tcp. networkType, ok := networktype.Get(address) From 215625139336901c598d19659232d88a62d0f268 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Sat, 27 Sep 2025 17:59:59 +0530 Subject: [PATCH 02/11] correct bugs --- internal/resolver/delegatingresolver/delegatingresolver.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index 60a6a5d664dd..57bea2a1d1e3 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -140,8 +140,8 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti return nil, fmt.Errorf("delegating_resolver: invalid target address %s: %v", target.Endpoint(), err) } r.targetResolverState = &resolver.State{ - Addresses: []resolver.Address{{Addr: target.Endpoint()}}, - Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: target.Endpoint()}}}}, + Addresses: []resolver.Address{{Addr: add}}, + Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: add}}}}, } r.updateTargetResolverState(*r.targetResolverState) return r, nil @@ -226,7 +226,7 @@ func maybeAddDefaultPort(target, defaultPort string) (string, error) { // the local system is assumed. host = "localhost" } - return "", internal.ErrMissingAddr + return fmt.Sprintf("%s:%s", host, port), nil } if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { // target doesn't have port From 07add50da8ee802186350a903412953ed8509f46 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Sun, 28 Sep 2025 14:33:35 +0530 Subject: [PATCH 03/11] change error msg --- internal/resolver/delegatingresolver/delegatingresolver.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index 57bea2a1d1e3..f9ef433ff269 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -29,7 +29,6 @@ import ( "google.golang.org/grpc/grpclog" "google.golang.org/grpc/internal/proxyattributes" - "google.golang.org/grpc/internal/resolver/dns/internal" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/internal/transport/networktype" "google.golang.org/grpc/resolver" @@ -212,13 +211,13 @@ func needsProxyResolver(state *resolver.State) bool { func maybeAddDefaultPort(target, defaultPort string) (string, error) { if target == "" { - return "", internal.ErrMissingAddr + return "", fmt.Errorf("missing address") } if host, port, err := net.SplitHostPort(target); err == nil { if port == "" { // If the port field is empty (target ends with colon), e.g. "[::1]:", // this is an error. - return "", internal.ErrEndsWithColon + return "", fmt.Errorf("missing port after port-separator colon") } // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port if host == "" { From abc177334de696f7bb997d05f5681450f09a2c79 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Sun, 28 Sep 2025 18:37:20 +0530 Subject: [PATCH 04/11] add test --- .../delegatingresolver_ext_test.go | 85 ++++++++++++------- internal/transport/proxy_ext_test.go | 2 +- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index 92ea2ab6ea8e..9bd3ee11fed1 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -253,48 +253,69 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithTargetResolution(t *testing.T) // of the proxy address. func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing.T) { const ( - targetTestAddr = "test.com" envProxyAddr = "proxytest.com" resolvedProxyTestAddr1 = "11.11.11.11:7687" ) - overrideTestHTTPSProxy(t, envProxyAddr) - - targetResolver := manual.NewBuilderWithScheme("dns") - target := targetResolver.Scheme() + ":///" + targetTestAddr - // Set up a manual DNS resolver to control the proxy address resolution. - proxyResolver, proxyResolverBuilt := setupDNS(t) - - tcc, stateCh, _ := createTestResolverClientConn(t) - if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { - t.Fatalf("Failed to create delegating resolver: %v", err) + tests := []struct { + name string + target string + wantConnectAddress string + }{ + { + name: "no port in target", + target: "test.com", + wantConnectAddress: "test.com:443", + }, + { + name: "port specified in target", + target: "test.com:8080", + wantConnectAddress: "test.com:8080", + }, } - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + overrideTestHTTPSProxy(t, envProxyAddr) - // Wait for the proxy resolver to be built before calling UpdateState. - mustBuildResolver(ctx, t, proxyResolverBuilt) - proxyResolver.UpdateState(resolver.State{ - Addresses: []resolver.Address{ - {Addr: resolvedProxyTestAddr1}, - }, - }) + targetResolver := manual.NewBuilderWithScheme("dns") + target := targetResolver.Scheme() + ":///" + test.target + // Set up a manual DNS resolver to control the proxy address resolution. + proxyResolver, proxyResolverBuilt := setupDNS(t) - wantState := resolver.State{ - Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}, - Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}}}, - } + tcc, stateCh, _ := createTestResolverClientConn(t) + if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { + t.Fatalf("Failed to create delegating resolver: %v", err) + } - var gotState resolver.State - select { - case gotState = <-stateCh: - case <-ctx.Done(): - t.Fatal("Context timed out when waiting for a state update from the delegating resolver") - } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() - if diff := cmp.Diff(gotState, wantState); diff != "" { - t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff) + // Wait for the proxy resolver to be built before calling UpdateState. + mustBuildResolver(ctx, t, proxyResolverBuilt) + proxyResolver.UpdateState(resolver.State{ + Addresses: []resolver.Address{ + {Addr: resolvedProxyTestAddr1}, + }, + }) + + wantState := resolver.State{ + Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, test.wantConnectAddress)}, + Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, test.wantConnectAddress)}}}, + } + + var gotState resolver.State + select { + case gotState = <-stateCh: + case <-ctx.Done(): + t.Fatal("Context timed out when waiting for a state update from the delegating resolver") + } + + if diff := cmp.Diff(gotState, wantState); diff != "" { + t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff) + } + }) } + } // Tests the scenario where a proxy is configured, and the target URI scheme is diff --git a/internal/transport/proxy_ext_test.go b/internal/transport/proxy_ext_test.go index 9cd3472299f6..c24f5a70e013 100644 --- a/internal/transport/proxy_ext_test.go +++ b/internal/transport/proxy_ext_test.go @@ -393,7 +393,7 @@ func (s) TestBasicAuthInNewClientWithProxy(t *testing.T) { proxyCalled := false reqCheck := func(req *http.Request) { proxyCalled = true - if got, want := req.URL.Host, "example.test"; got != want { + if got, want := req.URL.Host, "example.test:443"; got != want { t.Errorf(" Unexpected request host: %s, want = %s ", got, want) } wantProxyAuthStr := "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":"+password)) From 0702e0c22b0ffb4bc03e67eb126aa652135124cf Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Mon, 29 Sep 2025 15:04:46 +0530 Subject: [PATCH 05/11] correct test --- .../delegatingresolver/delegatingresolver.go | 8 ++-- .../delegatingresolver_ext_test.go | 47 +++++++++++++++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index f9ef433ff269..c1bddce11913 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -113,7 +113,7 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti var err error r.proxyURL, err = proxyURLForTarget(target.Endpoint()) if err != nil { - return nil, fmt.Errorf("delegating_resolver: failed to determine proxy URL for target %s: %v", target, err) + return nil, fmt.Errorf("delegating_resolver: failed to determine proxy URL for target %q: %v", target, err) } // proxy is not configured or proxy address excluded using `NO_PROXY` env @@ -136,7 +136,7 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti if target.URL.Scheme == "dns" && !targetResolutionEnabled { add, err := maybeAddDefaultPort(target.Endpoint(), defaultPort) if err != nil { - return nil, fmt.Errorf("delegating_resolver: invalid target address %s: %v", target.Endpoint(), err) + return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) } r.targetResolverState = &resolver.State{ Addresses: []resolver.Address{{Addr: add}}, @@ -225,11 +225,11 @@ func maybeAddDefaultPort(target, defaultPort string) (string, error) { // the local system is assumed. host = "localhost" } - return fmt.Sprintf("%s:%s", host, port), nil + return net.JoinHostPort(host, port), nil } if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { // target doesn't have port - return fmt.Sprintf("%s:%s", host, port), nil + return net.JoinHostPort(host, port), nil } return "", fmt.Errorf("invalid target address %v", target) } diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index 9bd3ee11fed1..a69a86844cdb 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -23,6 +23,7 @@ import ( "errors" "net/http" "net/url" + "strings" "testing" "time" @@ -246,11 +247,13 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithTargetResolution(t *testing.T) } } -// Tests the scenario where a proxy is configured, the target URI contains the -// "dns" scheme, and target resolution is disabled(default behavior). The test -// verifies that the addresses returned by the delegating resolver include the -// proxy resolver's addresses, with the unresolved target URI as an attribute -// of the proxy address. +// TestDelegatingResolverWithProxy tests the creation of a delegating resolver +// when a proxy is configured. It verifies both successful creation for valid +// targets and correct error handling for invalid ones. +// +// For successful cases, it ensures the final address is from the proxy resolver +// and contains the original, correctly-formatted target address as an +// attribute. func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing.T) { const ( envProxyAddr = "proxytest.com" @@ -260,6 +263,7 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. name string target string wantConnectAddress string + wantErrorSubstring string }{ { name: "no port in target", @@ -271,6 +275,21 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. target: "test.com:8080", wantConnectAddress: "test.com:8080", }, + { + name: "no host specified in target", + target: ":8080", + wantConnectAddress: "localhost:8080", + }, + { + name: "colon after host in target but no post", + target: "test.com:", + wantErrorSubstring: "missing port after port-separator colon", + }, + { + name: "missing target", + target: "", + wantErrorSubstring: "missing address", + }, } for _, test := range tests { @@ -283,10 +302,23 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. proxyResolver, proxyResolverBuilt := setupDNS(t) tcc, stateCh, _ := createTestResolverClientConn(t) - if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { - t.Fatalf("Failed to create delegating resolver: %v", err) + _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false) + if test.wantErrorSubstring != "" { + // Case 1: We expected an error. + if err == nil { + t.Fatalf("Delegating resolver created, want error containing %q", test.wantErrorSubstring) + } + if !strings.Contains(err.Error(), test.wantErrorSubstring) { + t.Fatalf("Delegating resolver failed with error %q, want error containing %q", err.Error(), test.wantErrorSubstring) + } + // Expected error was found, so the test case for this part is done. + return } + // Case 2: We did NOT expect an error. + if err != nil { + t.Fatalf("Delegating resolver creation failed unexpectedly with error: %v", err) + } ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() @@ -315,7 +347,6 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. } }) } - } // Tests the scenario where a proxy is configured, and the target URI scheme is From 2f50b78e60be22f73531f5642bbd7de9440dc465 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Wed, 1 Oct 2025 17:59:21 +0530 Subject: [PATCH 06/11] add new test --- internal/envconfig/envconfig.go | 5 ++ .../delegatingresolver/delegatingresolver.go | 61 ++++++++++-------- .../delegatingresolver_ext_test.go | 63 ++++++++++++++++--- .../delegatingresolver_test.go | 4 +- 4 files changed, 95 insertions(+), 38 deletions(-) diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 7e060f5ed132..1063567e30c8 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -75,6 +75,11 @@ var ( // ALTSHandshakerKeepaliveParams is set if we should add the // KeepaliveParams when dial the ALTS handshaker service. ALTSHandshakerKeepaliveParams = boolFromEnv("GRPC_EXPERIMENTAL_ALTS_HANDSHAKER_KEEPALIVE_PARAMS", false) + + // AddDefaultPort is set if the resolver should add the default port when + // the target address doesn't contain a port. We will add the default port + // 443 for default scheme. + AddDefaultPort = boolFromEnv("GRPC_EXPERIMENTAL_RESOLVER_ADD_DEFAULT_PORT", true) ) func boolFromEnv(envVar string, def bool) bool { diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index c1bddce11913..bef5b2e770ad 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -28,6 +28,7 @@ import ( "sync" "google.golang.org/grpc/grpclog" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/proxyattributes" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/internal/transport/networktype" @@ -83,6 +84,7 @@ func (nopResolver) Close() {} // - non-nil URL, nil error: A proxy is configured, and the proxy URL was // retrieved successfully without any errors. func proxyURLForTarget(address string) (*url.URL, error) { + parseTarget(address) req := &http.Request{URL: &url.URL{ Scheme: "https", Host: address, @@ -110,8 +112,11 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti targetResolver: nopResolver{}, } - var err error - r.proxyURL, err = proxyURLForTarget(target.Endpoint()) + addr, err := parseTarget(target.Endpoint()) + if err != nil { + return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) + } + r.proxyURL, err = proxyURLForTarget(addr) if err != nil { return nil, fmt.Errorf("delegating_resolver: failed to determine proxy URL for target %q: %v", target, err) } @@ -134,9 +139,12 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti // resolution should be handled by the proxy, not the client. Therefore, we // bypass the target resolver and store the unresolved target address. if target.URL.Scheme == "dns" && !targetResolutionEnabled { - add, err := maybeAddDefaultPort(target.Endpoint(), defaultPort) - if err != nil { - return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) + add := target.Endpoint() + if envconfig.AddDefaultPort { + add, err = maybeAddDefaultPort(target.Endpoint(), defaultPort) + if err != nil { + return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) + } } r.targetResolverState = &resolver.State{ Addresses: []resolver.Address{{Addr: add}}, @@ -209,29 +217,32 @@ func needsProxyResolver(state *resolver.State) bool { return false } -func maybeAddDefaultPort(target, defaultPort string) (string, error) { - if target == "" { - return "", fmt.Errorf("missing address") +func parseTarget(target string) (string, error) { + host, port, err := net.SplitHostPort(target) + if err != nil { + return target, nil } - if host, port, err := net.SplitHostPort(target); err == nil { - if port == "" { - // If the port field is empty (target ends with colon), e.g. "[::1]:", - // this is an error. - return "", fmt.Errorf("missing port after port-separator colon") - } - // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port - if host == "" { - // Keep consistent with net.Dial(): If the host is empty, as in ":80", - // the local system is assumed. - host = "localhost" - } - return net.JoinHostPort(host, port), nil + if port == "" { + // If the port field is empty (target ends with colon), e.g. "[::1]:", + // this is an error. + return "", fmt.Errorf("missing port after port-separator colon") + } + // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port + if host == "" { + // Keep consistent with net.Dial(): If the host is empty, as in ":80", + // the local system is assumed. + host = "localhost" } - if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { - // target doesn't have port - return net.JoinHostPort(host, port), nil + return net.JoinHostPort(host, port), nil +} + +func maybeAddDefaultPort(target, defaultPort string) (string, error) { + if _, _, err := net.SplitHostPort(target); err == nil { + // target already has port + return target, nil } - return "", fmt.Errorf("invalid target address %v", target) + return net.JoinHostPort(target, defaultPort), nil + } func skipProxy(address resolver.Address) bool { diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index a69a86844cdb..7f8d58709970 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -28,6 +28,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/proxyattributes" "google.golang.org/grpc/internal/resolver/delegatingresolver" @@ -275,21 +276,11 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. target: "test.com:8080", wantConnectAddress: "test.com:8080", }, - { - name: "no host specified in target", - target: ":8080", - wantConnectAddress: "localhost:8080", - }, { name: "colon after host in target but no post", target: "test.com:", wantErrorSubstring: "missing port after port-separator colon", }, - { - name: "missing target", - target: "", - wantErrorSubstring: "missing address", - }, } for _, test := range tests { @@ -349,6 +340,58 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. } } +// Tests the scenario where a proxy is configured, the target URI scheme is +// "dns", target resolution is disabled, and the environment variable to add +// default port is disabled. The test verifies that the addresses returned by +// the delegating resolver include the resolved proxy address and the unresolved +// target address without a port as attributes of the proxy address. +func (s) TestDelegatingResolverEnvVarForDefaultPortDisabled(t *testing.T) { + const ( + targetTestAddr = "test.com" + envProxyAddr = "proxytest.com" + resolvedProxyTestAddr1 = "11.11.11.11:7687" + ) + + testutils.SetEnvConfig(t, &envconfig.AddDefaultPort, false) + overrideTestHTTPSProxy(t, envProxyAddr) + + targetResolver := manual.NewBuilderWithScheme("dns") + target := targetResolver.Scheme() + ":///" + targetTestAddr + // Set up a manual DNS resolver to control the proxy address resolution. + proxyResolver, proxyResolverBuilt := setupDNS(t) + + tcc, stateCh, _ := createTestResolverClientConn(t) + if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { + t.Fatalf("Delegating resolver creation failed unexpectedly with error: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + // Wait for the proxy resolver to be built before calling UpdateState. + mustBuildResolver(ctx, t, proxyResolverBuilt) + proxyResolver.UpdateState(resolver.State{ + Addresses: []resolver.Address{ + {Addr: resolvedProxyTestAddr1}, + }, + }) + + wantState := resolver.State{ + Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}, + Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}}}, + } + + var gotState resolver.State + select { + case gotState = <-stateCh: + case <-ctx.Done(): + t.Fatal("Context timed out when waiting for a state update from the delegating resolver") + } + + if diff := cmp.Diff(gotState, wantState); diff != "" { + t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff) + } +} + // Tests the scenario where a proxy is configured, and the target URI scheme is // not "dns". The test verifies that the addresses returned by the delegating // resolver include the resolved proxy address and the custom resolved target diff --git a/internal/resolver/delegatingresolver/delegatingresolver_test.go b/internal/resolver/delegatingresolver/delegatingresolver_test.go index c99afff422e6..f575cb057c8a 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_test.go @@ -37,9 +37,7 @@ func Test(t *testing.T) { } const ( - targetTestAddr = "test.com" - envProxyAddr = "proxytest.com" -) + targetTestAddr = "test.com") // overrideHTTPSProxyFromEnvironment function overwrites HTTPSProxyFromEnvironment and // returns a function to restore the default values. From aee0066903885cfe9fdf97e5ae496c94c11a7bb3 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Wed, 1 Oct 2025 18:00:53 +0530 Subject: [PATCH 07/11] vet --- .../resolver/delegatingresolver/delegatingresolver_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver_test.go b/internal/resolver/delegatingresolver/delegatingresolver_test.go index f575cb057c8a..154d7194db56 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_test.go @@ -36,8 +36,7 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -const ( - targetTestAddr = "test.com") +const targetTestAddr = "test.com" // overrideHTTPSProxyFromEnvironment function overwrites HTTPSProxyFromEnvironment and // returns a function to restore the default values. From 3d93bc19de95d8e51195b453099ee4ce4f40420e Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Wed, 1 Oct 2025 18:09:39 +0530 Subject: [PATCH 08/11] minor changes --- .../delegatingresolver/delegatingresolver.go | 18 +++++++----------- .../delegatingresolver_ext_test.go | 3 ++- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index bef5b2e770ad..f62bfcb963c9 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -84,7 +84,6 @@ func (nopResolver) Close() {} // - non-nil URL, nil error: A proxy is configured, and the proxy URL was // retrieved successfully without any errors. func proxyURLForTarget(address string) (*url.URL, error) { - parseTarget(address) req := &http.Request{URL: &url.URL{ Scheme: "https", Host: address, @@ -139,16 +138,13 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti // resolution should be handled by the proxy, not the client. Therefore, we // bypass the target resolver and store the unresolved target address. if target.URL.Scheme == "dns" && !targetResolutionEnabled { - add := target.Endpoint() + addr := target.Endpoint() if envconfig.AddDefaultPort { - add, err = maybeAddDefaultPort(target.Endpoint(), defaultPort) - if err != nil { - return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) - } + addr = maybeAddDefaultPort(target.Endpoint(), defaultPort) } r.targetResolverState = &resolver.State{ - Addresses: []resolver.Address{{Addr: add}}, - Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: add}}}}, + Addresses: []resolver.Address{{Addr: addr}}, + Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: addr}}}}, } r.updateTargetResolverState(*r.targetResolverState) return r, nil @@ -236,12 +232,12 @@ func parseTarget(target string) (string, error) { return net.JoinHostPort(host, port), nil } -func maybeAddDefaultPort(target, defaultPort string) (string, error) { +func maybeAddDefaultPort(target, defaultPort string) string { if _, _, err := net.SplitHostPort(target); err == nil { // target already has port - return target, nil + return target } - return net.JoinHostPort(target, defaultPort), nil + return net.JoinHostPort(target, defaultPort) } diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index 7f8d58709970..bff993108492 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -362,8 +362,9 @@ func (s) TestDelegatingResolverEnvVarForDefaultPortDisabled(t *testing.T) { tcc, stateCh, _ := createTestResolverClientConn(t) if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { - t.Fatalf("Delegating resolver creation failed unexpectedly with error: %v", err) + t.Fatalf("Failed to create delegating resolver: %v", err) } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() From 80a3e98e72089161ead7352386fe35d9ee296abe Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Mon, 6 Oct 2025 19:22:22 +0530 Subject: [PATCH 09/11] comments --- internal/envconfig/envconfig.go | 11 +++++++---- .../resolver/delegatingresolver/delegatingresolver.go | 5 ++--- .../delegatingresolver/delegatingresolver_ext_test.go | 11 +++++------ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 1063567e30c8..293a9a40b241 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -76,10 +76,13 @@ var ( // KeepaliveParams when dial the ALTS handshaker service. ALTSHandshakerKeepaliveParams = boolFromEnv("GRPC_EXPERIMENTAL_ALTS_HANDSHAKER_KEEPALIVE_PARAMS", false) - // AddDefaultPort is set if the resolver should add the default port when - // the target address doesn't contain a port. We will add the default port - // 443 for default scheme. - AddDefaultPort = boolFromEnv("GRPC_EXPERIMENTAL_RESOLVER_ADD_DEFAULT_PORT", true) + // EnableDefaultPortForProxyTarget controls whether the resolver adds a default port 443 + // to a target address that lacks one. This flag only has an effect when all of + // the following conditions are met: + // - A connect proxy is being used. + // - Target resolution is disabled. + // - The DNS resolver is being used. + EnableDefaultPortForProxyTarget = boolFromEnv("GRPC_EXPERIMENTAL_ENABLE_DEFAULT_PORT_FOR_PROXY_TARGET", true) ) func boolFromEnv(envVar string, def bool) bool { diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index f62bfcb963c9..c7f3b26a7196 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -138,9 +138,8 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti // resolution should be handled by the proxy, not the client. Therefore, we // bypass the target resolver and store the unresolved target address. if target.URL.Scheme == "dns" && !targetResolutionEnabled { - addr := target.Endpoint() - if envconfig.AddDefaultPort { - addr = maybeAddDefaultPort(target.Endpoint(), defaultPort) + if envconfig.EnableDefaultPortForProxyTarget { + addr = maybeAddDefaultPort(addr, defaultPort) } r.targetResolverState = &resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index bff993108492..4a1176b76ec0 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -248,9 +248,9 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithTargetResolution(t *testing.T) } } -// TestDelegatingResolverWithProxy tests the creation of a delegating resolver -// when a proxy is configured. It verifies both successful creation for valid -// targets and correct error handling for invalid ones. +// Tests the creation of a delegating resolver when a proxy is configured. It +// verifies both successful creation for valid targets and correct error +// handling for invalid ones. // // For successful cases, it ensures the final address is from the proxy resolver // and contains the original, correctly-formatted target address as an @@ -300,9 +300,8 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. t.Fatalf("Delegating resolver created, want error containing %q", test.wantErrorSubstring) } if !strings.Contains(err.Error(), test.wantErrorSubstring) { - t.Fatalf("Delegating resolver failed with error %q, want error containing %q", err.Error(), test.wantErrorSubstring) + t.Fatalf("Delegating resolver failed with error %v, want error containing %v", err.Error(), test.wantErrorSubstring) } - // Expected error was found, so the test case for this part is done. return } @@ -352,7 +351,7 @@ func (s) TestDelegatingResolverEnvVarForDefaultPortDisabled(t *testing.T) { resolvedProxyTestAddr1 = "11.11.11.11:7687" ) - testutils.SetEnvConfig(t, &envconfig.AddDefaultPort, false) + testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, false) overrideTestHTTPSProxy(t, envProxyAddr) targetResolver := manual.NewBuilderWithScheme("dns") From c19e88d8b955033c3fd503bccc4474269c05eebb Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Tue, 7 Oct 2025 18:28:50 +0530 Subject: [PATCH 10/11] change conditions --- .../delegatingresolver/delegatingresolver.go | 55 +++++++------- .../delegatingresolver_ext_test.go | 71 ++++--------------- 2 files changed, 41 insertions(+), 85 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index c7f3b26a7196..a29dad709196 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -111,10 +111,15 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti targetResolver: nopResolver{}, } - addr, err := parseTarget(target.Endpoint()) - if err != nil { - return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) + addr := target.Endpoint() + var err error + if target.URL.Scheme == "dns" && !targetResolutionEnabled && envconfig.EnableDefaultPortForProxyTarget { + addr, err = parseTarget(addr) + if err != nil { + return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err) + } } + r.proxyURL, err = proxyURLForTarget(addr) if err != nil { return nil, fmt.Errorf("delegating_resolver: failed to determine proxy URL for target %q: %v", target, err) @@ -138,9 +143,6 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti // resolution should be handled by the proxy, not the client. Therefore, we // bypass the target resolver and store the unresolved target address. if target.URL.Scheme == "dns" && !targetResolutionEnabled { - if envconfig.EnableDefaultPortForProxyTarget { - addr = maybeAddDefaultPort(addr, defaultPort) - } r.targetResolverState = &resolver.State{ Addresses: []resolver.Address{{Addr: addr}}, Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: addr}}}}, @@ -213,31 +215,28 @@ func needsProxyResolver(state *resolver.State) bool { } func parseTarget(target string) (string, error) { - host, port, err := net.SplitHostPort(target) - if err != nil { - return target, nil - } - if port == "" { - // If the port field is empty (target ends with colon), e.g. "[::1]:", - // this is an error. - return "", fmt.Errorf("missing port after port-separator colon") + if target == "" { + return "", fmt.Errorf("missing address") } - // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port - if host == "" { - // Keep consistent with net.Dial(): If the host is empty, as in ":80", - // the local system is assumed. - host = "localhost" + if host, port, err := net.SplitHostPort(target); err == nil { + if port == "" { + // If the port field is empty (target ends with colon), e.g. "[::1]:", + // this is an error. + return "", fmt.Errorf("missing port after port-separator colon") + } + // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port + if host == "" { + // Keep consistent with net.Dial(): If the host is empty, as in ":80", + // the local system is assumed. + host = "localhost" + } + return net.JoinHostPort(host, port), nil } - return net.JoinHostPort(host, port), nil -} - -func maybeAddDefaultPort(target, defaultPort string) string { - if _, _, err := net.SplitHostPort(target); err == nil { - // target already has port - return target + if host, port, err := net.SplitHostPort(target + ":" + defaultPort); err == nil { + // target doesn't have port + return net.JoinHostPort(host, port), nil } - return net.JoinHostPort(target, defaultPort) - + return net.JoinHostPort(target, defaultPort), nil } func skipProxy(address resolver.Address) bool { diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index 4a1176b76ec0..eddd86dcf18c 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -261,10 +261,11 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. resolvedProxyTestAddr1 = "11.11.11.11:7687" ) tests := []struct { - name string - target string - wantConnectAddress string - wantErrorSubstring string + name string + target string + wantConnectAddress string + wantErrorSubstring string + disableDefaultPortEnvVar bool }{ { name: "no port in target", @@ -281,10 +282,19 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. target: "test.com:", wantErrorSubstring: "missing port after port-separator colon", }, + { + name: "add default port env variable diabled, no port in target", + target: "test.com", + wantConnectAddress: "test.com", + disableDefaultPortEnvVar: true, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { + if test.disableDefaultPortEnvVar { + testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, false) + } overrideTestHTTPSProxy(t, envProxyAddr) targetResolver := manual.NewBuilderWithScheme("dns") @@ -339,59 +349,6 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. } } -// Tests the scenario where a proxy is configured, the target URI scheme is -// "dns", target resolution is disabled, and the environment variable to add -// default port is disabled. The test verifies that the addresses returned by -// the delegating resolver include the resolved proxy address and the unresolved -// target address without a port as attributes of the proxy address. -func (s) TestDelegatingResolverEnvVarForDefaultPortDisabled(t *testing.T) { - const ( - targetTestAddr = "test.com" - envProxyAddr = "proxytest.com" - resolvedProxyTestAddr1 = "11.11.11.11:7687" - ) - - testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, false) - overrideTestHTTPSProxy(t, envProxyAddr) - - targetResolver := manual.NewBuilderWithScheme("dns") - target := targetResolver.Scheme() + ":///" + targetTestAddr - // Set up a manual DNS resolver to control the proxy address resolution. - proxyResolver, proxyResolverBuilt := setupDNS(t) - - tcc, stateCh, _ := createTestResolverClientConn(t) - if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { - t.Fatalf("Failed to create delegating resolver: %v", err) - } - - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - - // Wait for the proxy resolver to be built before calling UpdateState. - mustBuildResolver(ctx, t, proxyResolverBuilt) - proxyResolver.UpdateState(resolver.State{ - Addresses: []resolver.Address{ - {Addr: resolvedProxyTestAddr1}, - }, - }) - - wantState := resolver.State{ - Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}, - Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{proxyAddressWithTargetAttribute(resolvedProxyTestAddr1, targetTestAddr)}}}, - } - - var gotState resolver.State - select { - case gotState = <-stateCh: - case <-ctx.Done(): - t.Fatal("Context timed out when waiting for a state update from the delegating resolver") - } - - if diff := cmp.Diff(gotState, wantState); diff != "" { - t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff) - } -} - // Tests the scenario where a proxy is configured, and the target URI scheme is // not "dns". The test verifies that the addresses returned by the delegating // resolver include the resolved proxy address and the custom resolved target From 82076ed34ab33890099be8015fdb52152d379895 Mon Sep 17 00:00:00 2001 From: eshitachandwani Date: Wed, 8 Oct 2025 17:16:27 +0530 Subject: [PATCH 11/11] changes --- .../delegatingresolver/delegatingresolver.go | 16 ++- .../delegatingresolver_ext_test.go | 106 +++++++++++------- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/internal/resolver/delegatingresolver/delegatingresolver.go b/internal/resolver/delegatingresolver/delegatingresolver.go index a29dad709196..70ef95ec3191 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver.go +++ b/internal/resolver/delegatingresolver/delegatingresolver.go @@ -214,6 +214,18 @@ func needsProxyResolver(state *resolver.State) bool { return false } +// parseTarget takes a target string and ensures it is a valid "host:port" target. +// +// It does the following: +// 1. If the target already has a port (e.g., "host:port", "[ipv6]:port"), +// it is returned as is. +// 2. If the host part is empty (e.g., ":80"), it defaults to "localhost", +// returning "localhost:80". +// 3. If the target is missing a port (e.g., "host", "ipv6"), the defaultPort +// is added. +// +// An error is returned for empty targets or targets with a trailing colon +// but no port (e.g., "host:"). func parseTarget(target string) (string, error) { if target == "" { return "", fmt.Errorf("missing address") @@ -224,10 +236,8 @@ func parseTarget(target string) (string, error) { // this is an error. return "", fmt.Errorf("missing port after port-separator colon") } - // target has port, i.e ipv4-host:port, [ipv6-host]:port, host-name:port + // target has port if host == "" { - // Keep consistent with net.Dial(): If the host is empty, as in ":80", - // the local system is assumed. host = "localhost" } return net.JoinHostPort(host, port), nil diff --git a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go index eddd86dcf18c..caa1fdd84b59 100644 --- a/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go +++ b/internal/resolver/delegatingresolver/delegatingresolver_ext_test.go @@ -249,52 +249,43 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithTargetResolution(t *testing.T) } // Tests the creation of a delegating resolver when a proxy is configured. It -// verifies both successful creation for valid targets and correct error -// handling for invalid ones. -// -// For successful cases, it ensures the final address is from the proxy resolver -// and contains the original, correctly-formatted target address as an -// attribute. -func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing.T) { +// verifies successful creation for valid targets and ensures the final address +// is from the proxy resolver and contains the original, correctly-formatted +// target address as an attribute. +func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolutionHappyPaths(t *testing.T) { const ( envProxyAddr = "proxytest.com" resolvedProxyTestAddr1 = "11.11.11.11:7687" ) tests := []struct { - name string - target string - wantConnectAddress string - wantErrorSubstring string - disableDefaultPortEnvVar bool + name string + target string + wantConnectAddress string + defaultPortEnvVar bool }{ { - name: "no port in target", + name: "no port ", target: "test.com", wantConnectAddress: "test.com:443", + defaultPortEnvVar: true, }, { - name: "port specified in target", + name: "complete target", target: "test.com:8080", wantConnectAddress: "test.com:8080", + defaultPortEnvVar: true, }, { - name: "colon after host in target but no post", - target: "test.com:", - wantErrorSubstring: "missing port after port-separator colon", - }, - { - name: "add default port env variable diabled, no port in target", - target: "test.com", - wantConnectAddress: "test.com", - disableDefaultPortEnvVar: true, + name: "no port with default port env variable diabled", + target: "test.com", + wantConnectAddress: "test.com", + defaultPortEnvVar: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.disableDefaultPortEnvVar { - testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, false) - } + testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, test.defaultPortEnvVar) overrideTestHTTPSProxy(t, envProxyAddr) targetResolver := manual.NewBuilderWithScheme("dns") @@ -303,20 +294,7 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. proxyResolver, proxyResolverBuilt := setupDNS(t) tcc, stateCh, _ := createTestResolverClientConn(t) - _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false) - if test.wantErrorSubstring != "" { - // Case 1: We expected an error. - if err == nil { - t.Fatalf("Delegating resolver created, want error containing %q", test.wantErrorSubstring) - } - if !strings.Contains(err.Error(), test.wantErrorSubstring) { - t.Fatalf("Delegating resolver failed with error %v, want error containing %v", err.Error(), test.wantErrorSubstring) - } - return - } - - // Case 2: We did NOT expect an error. - if err != nil { + if _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false); err != nil { t.Fatalf("Delegating resolver creation failed unexpectedly with error: %v", err) } ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) @@ -349,6 +327,54 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolution(t *testing. } } +// Tests the creation of a delegating resolver when a proxy is configured. It +// verifies correct error handling for invalid targets. +func (s) TestDelegatingResolverwithDNSAndProxyWithNoTargetResolutionWithErrorTargets(t *testing.T) { + const ( + envProxyAddr = "proxytest.com" + resolvedProxyTestAddr1 = "11.11.11.11:7687" + ) + tests := []struct { + name string + target string + wantErrorSubstring string + defaultPortEnvVar bool + }{ + + { + name: "colon after host but no port", + target: "test.com:", + wantErrorSubstring: "missing port after port-separator colon", + defaultPortEnvVar: true, + }, + { + name: "empty target", + target: "", + wantErrorSubstring: "missing address", + defaultPortEnvVar: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + testutils.SetEnvConfig(t, &envconfig.EnableDefaultPortForProxyTarget, test.defaultPortEnvVar) + overrideTestHTTPSProxy(t, envProxyAddr) + + targetResolver := manual.NewBuilderWithScheme("dns") + target := targetResolver.Scheme() + ":///" + test.target + + tcc, _, _ := createTestResolverClientConn(t) + _, err := delegatingresolver.New(resolver.Target{URL: *testutils.MustParseURL(target)}, tcc, resolver.BuildOptions{}, targetResolver, false) + if err == nil { + t.Fatalf("Delegating resolver created, want error containing %q", test.wantErrorSubstring) + } + if !strings.Contains(err.Error(), test.wantErrorSubstring) { + t.Fatalf("Delegating resolver failed with error %v, want error containing %v", err.Error(), test.wantErrorSubstring) + } + }) + } +} + // Tests the scenario where a proxy is configured, and the target URI scheme is // not "dns". The test verifies that the addresses returned by the delegating // resolver include the resolved proxy address and the custom resolved target