delegatingresolver: add default port to addresses

internal/envconfig/envconfig.go

@@ -75,6 +75,14 @@ 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)
+
+	// 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 {

internal/resolver/delegatingresolver/delegatingresolver.go

@@ -22,11 +22,13 @@ package delegatingresolver
 
 import (
 	"fmt"
+	"net"
 	"net/http"
 	"net/url"
 	"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"
@@ -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.
@@ -107,10 +111,18 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti
 		targetResolver: nopResolver{},
 	}
 
+	addr := target.Endpoint()
 	var err error
-	r.proxyURL, err = proxyURLForTarget(target.Endpoint())
+	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 %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
@@ -132,8 +144,8 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti
 	// bypass the target resolver and store the unresolved target address.
 	if target.URL.Scheme == "dns" && !targetResolutionEnabled {
 		r.targetResolverState = &resolver.State{
-			Addresses: []resolver.Address{{Addr: target.Endpoint()}},
-			Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: target.Endpoint()}}}},
+			Addresses: []resolver.Address{{Addr: addr}},
+			Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: addr}}}},
 		}
 		r.updateTargetResolverState(*r.targetResolverState)
 		return r, nil
@@ -202,6 +214,31 @@ func needsProxyResolver(state *resolver.State) bool {
 	return false
 }
 
+func parseTarget(target string) (string, error) {
+	if target == "" {
+		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 "", 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 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), nil
+}
+
 func skipProxy(address resolver.Address) bool {
 	// Avoid proxy when network is not tcp.
 	networkType, ok := networktype.Get(address)

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

@@ -23,10 +23,12 @@ import (
 	"errors"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 	"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"
@@ -246,54 +248,104 @@ 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.
+// 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 (
-		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)
-	}
-
-	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},
+	tests := []struct {
+		name                     string
+		target                   string
+		wantConnectAddress       string
+		wantErrorSubstring       string
+		disableDefaultPortEnvVar bool
+	}{
+		{
+			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",
+		},
+		{
+			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,
 		},
-	})
-
-	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)
+	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")
+			target := targetResolver.Scheme() + ":///" + test.target
+			// Set up a manual DNS resolver to control the proxy address resolution.
+			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 {
+				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, 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)
+			}
+		})
 	}
 }
 

internal/resolver/delegatingresolver/delegatingresolver_test.go

@@ -36,10 +36,7 @@ func Test(t *testing.T) {
 	grpctest.RunSubTests(t, s{})
 }
 
-const (
-	targetTestAddr = "test.com"
-	envProxyAddr   = "proxytest.com"
-)
+const targetTestAddr = "test.com"
 
 // overrideHTTPSProxyFromEnvironment function overwrites HTTPSProxyFromEnvironment and
 // returns a function to restore the default values.

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))