Skip to content

delegatingresolver: add default port to addresses #8613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+175 −45
Open

delegatingresolver: add default port to addresses #8613

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4186a66
add default port
eshitachandwani Sep 27, 2025
2156251
correct bugs
eshitachandwani Sep 27, 2025
07add50
change error msg
eshitachandwani Sep 28, 2025
abc1773
add test
eshitachandwani Sep 28, 2025
0702e0c
correct test
eshitachandwani Sep 29, 2025
2f50b78
add new test
eshitachandwani Oct 1, 2025
aee0066
vet
eshitachandwani Oct 1, 2025
3d93bc1
minor changes
eshitachandwani Oct 1, 2025
80a3e98
comments
eshitachandwani Oct 6, 2025
c19e88d
change conditions
eshitachandwani Oct 7, 2025
82076ed
changes
eshitachandwani Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions internal/resolver/delegatingresolver/delegatingresolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package delegatingresolver

import (
"fmt"
"net"
"net/http"
"net/url"
"sync"
Expand All @@ -40,6 +41,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.
Expand Down Expand Up @@ -131,9 +134,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, 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()}}}},
Addresses: []resolver.Address{{Addr: add}},
Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: add}}}},
}
r.updateTargetResolverState(*r.targetResolverState)
return r, nil
Expand Down Expand Up @@ -202,6 +209,31 @@ func needsProxyResolver(state *resolver.State) bool {
return false
}

func maybeAddDefaultPort(target, defaultPort string) (string, error) {
if target == "" {
return "", fmt.Errorf("missing address")
}
Comment on lines +230 to +232
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even possible since we already parse the target URI specified by the user in clientconn.go before we get here?

Ideally, it would be nicer to avoid checking conditions that are not possible, since it would simplify the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see an explicit check for missing target in the clientconn.go and this is similar to the check we already have in dns.

if host, port, err := net.SplitHostPort(target); err == nil {
if port == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the documentation of net.SplitHostPort, the port seems to be mandatory and it will return a missing port in address error in that case. So, I don't think we could have err == nil and port == "".

// SplitHostPort splits a network address of the form "host:port",
// "host%zone:port", "[host]:port" or "[host%zone]:port" into host or
// host%zone and port.

Please correct me if I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err == nil and port == "" would be case where the target is something like : host: i.e. they have the colon but no port specified after it.

// If the port field is empty (target ends with colon), e.g. "[::1]:",
// this is an error.
Comment on lines +235 to +236
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: I think it would be better to group these inline comments into a docstring for the function. That way, one can just read that docstring and get to know what the function is doing instead of reading it one piece at a time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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 fmt.Sprintf("%s:%s", 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 "", 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)
Expand Down
85 changes: 53 additions & 32 deletions internal/resolver/delegatingresolver/delegatingresolver_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go/gotip/episodes/117

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you have to unregister the original dns resolver and re-register it at the end of the test, for this test to not affect other tests?

target := targetResolver.Scheme() + ":///" + test.target
// Set up a manual DNS resolver to control the proxy address resolution.
proxyResolver, proxyResolverBuilt := setupDNS(t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see that this helper does re-register the original dns resolver. But maybe this section can still be a little more explicit/readable etc. I'm not able to think of the exact way to make this better right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we had earlier too..in the tests.


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
Expand Down
2 changes: 1 addition & 1 deletion internal/transport/proxy_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Loading