transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

eshitachandwani · 2024-11-29T16:55:03Z

Fixes: #7556

Remove environment variable detection during transport creation. Introduce a per-address attribute to specify the proxy CONNECT string. This change aligns with the xDS design and provides more control over proxy behavior.
Create a new resolver that handles proxy configuration and delegates to child resolvers for target and proxy address resolution.
Introduce a new DialOption to preserve the current behavior of grpc.Dial, where the resolved target address is used in the proxy CONNECT request. This option ensures backward compatibility for users relying on the existing behavior.

RELEASE NOTES:

Target resolution is no longer performed on the client when using default resolvers with grpc.NewClient in conjunction with proxy environment variables.
New WithLocalDNSResolution() dial option added to explicitly force target URI resolution on the client.
When non-default resolvers are used alongside proxy environment variables, target resolution is now performed on the client.

codecov · 2024-11-29T16:59:56Z

Codecov Report

Attention: Patch coverage is 86.08696% with 16 lines in your changes missing coverage. Please review.

Project coverage is 82.34%. Comparing base (66f6471) to head (dd7ee47).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/testutils/proxyserver/proxyserver.go	78.37%	12 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7881      +/-   ##
==========================================
+ Coverage   82.14%   82.34%   +0.20%     
==========================================
  Files         382      383       +1     
  Lines       38711    38776      +65     
==========================================
+ Hits        31798    31929     +131     
+ Misses       5583     5531      -52     
+ Partials     1330     1316      -14

Files with missing lines	Coverage Δ
clientconn.go	`92.18% <100.00%> (+0.03%)`	⬆️
dialoptions.go	`90.80% <100.00%> (+1.82%)`	⬆️
internal/proxyattributes/proxyattributes.go	`100.00% <ø> (ø)`
.../resolver/delegatingresolver/delegatingresolver.go	`76.68% <100.00%> (+7.82%)`	⬆️
internal/transport/http2_client.go	`92.23% <100.00%> (+0.52%)`	⬆️
internal/transport/proxy.go	`70.58% <100.00%> (+2.58%)`	⬆️
internal/transport/transport.go	`91.56% <ø> (ø)`
resolver_wrapper.go	`84.16% <100.00%> (+1.28%)`	⬆️
internal/testutils/proxyserver/proxyserver.go	`78.37% <78.37%> (ø)`

... and 14 files with indirect coverage changes

purnesh42H · 2024-12-03T04:42:29Z

internal/testutils/proxy.go

+}
+
+// NewProxyServer create and starts a proxy server.
+func NewProxyServer(lis net.Listener, requestCheck func(*http.Request) error, errCh chan error, doneCh chan struct{}, backendAddr string, resolutionOnClient bool, proxyServerStarted func()) *ProxyServer {


may be we can do something like this fake server in its own package

grpc-go/internal/testutils/xds/fakeserver/server.go

Line 19 in 17d08f7

// Package fakeserver provides a fake implementation of the management server.

? wdyt?

New should only create the object and StartServer/Run should start the go routine to accept requests

purnesh42H · 2024-12-03T04:43:58Z

internal/transport/http2_client.go

 	address := addr.Addr
+
+	//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.


typo: attribute

internal/transport/proxy.go

purnesh42H · 2024-12-03T04:46:30Z

internal/transport/proxy.go

-func proxyDial(ctx context.Context, addr string, grpcUA string) (net.Conn, error) {
-	newAddr := addr
-	proxyURL, err := mapAddress(addr)
+// proxyDial dials, connecting to a proxy first if necessary. Dials, does the


we can remove the necessary part now? It will always dial proxy if called?

test/proxy_test.go

internal/proxyattributes/proxyattributes_test.go

purnesh42H · 2024-12-05T09:06:26Z

internal/proxyattributes/proxyattributes_test.go

+		Addr:       "test-address",
+		Attributes: attributes.New(userAndConnectAddrKey, attr{user: nil, addr: "proxy-address"}),
+	}
+


nix new line

test/proxy_test.go

purnesh42H · 2024-12-05T09:54:59Z

test/proxy_test.go

+
+// Tests grpc.NewClient with the default "dns" resolver and dial option
+// enabling target resolution on the client and verifies that the resolution
+// happens on client.


nit: mention the dial option WithTargetResolutionEnabled()

test/proxy_test.go

purnesh42H · 2024-12-05T09:59:14Z

test/proxy_test.go

+
+// Tests grpc.NewClient with grpc.WithNoProxy() set and verifies that it does
+// not dail to proxy, but directly to backend.
+func (s) TestGrpcNewClientWithNoProxy(t *testing.T) {


for the NoProxy and CustomDialer case, should we still override overrideHTTPSProxyFromEnvironment? That will make sure that even though proxy env is set we should skip it because of NoProxy and CustomDialer or is that not intended?

If WithNoProxy or WithContextDialer are set, the HTTPSProxyFromEnviornment function will never be called, but we can still override and add error if it is being called. Is it a good practice to do so? @purnesh42H

yeah so right now if HTTPSProxyFromEnviornment is not present, it doesn't matter if WithNoProxy or WithContextDialer is set? The resolution will happen at client anyways? So having it present and not being called in these two cases is what we want to verify.

purnesh42H · 2024-12-05T10:00:10Z

test/proxy_test.go

+		}
+		wantProxyAuthStr := "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":"+password))
+		if got := req.Header.Get("Proxy-Authorization"); got != wantProxyAuthStr {
+			gotDecoded, _ := base64.StdEncoding.DecodeString(got)


we should not ignore decoding errors. it should return error if decoding fails

internal/transport/proxy.go

internal/testutils/proxy.go

dfawley · 2025-01-16T18:38:51Z

internal/testutils/proxy.go

+	}()
+	t.Logf("Started proxy at: %q", pLis.Addr())
+	t.Cleanup(p.stop)
+	p.Addr = fmt.Sprintf("localhost:%d", ParsePort(t, pLis.Addr().String()))


Also why can't this just be pLis.Addr().String()? There should be a comment explaining why it's doing all this extra stuff to seemingly just convert localhost:123 into localhost:123.

pLis..Addr() will be a IP address, we convert it to localhost:port to make sure the DNS resolver is working correctly for the proxy and connecting to the proxy server.

Oh true it is converted to IP address. But why is that a problem?

If it's for testing the delegating resolver -> DNS resolver path, then it seems like this should just use the raw listener address, and the tests that care about that should convert it.

Either way, definitely needs a comment.

dfawley · 2025-01-16T18:56:22Z

internal/transport/proxy.go

-		u := t.Username()
-		p, _ := t.Password()
+
+	if user := opts.User; user != (url.Userinfo{}) {


This comparison with a zero-initialized Userinfo looks a little off to me. Should this field in opts be a pointer instead to determine whether it was set intentionally?

Or should it check if either Username() or Password() are non-empty?

I'm not sure exactly what the right behavior is, but this seems questionable, as it's including any unexported fields in the comparison.

Related but potential existing problem to investigate: what if username is set but password is not? Is that valid? If so, is the proper behavior to encode it as username:, as it does currently, or should the : be omitted?

The RFC doesn't clearly mention what is the correct syntax if password is absent , so I think either way should be fine, but adding a semicolon like username: would be more explicit in indication username has ended and the password is empty.

I have added a UserSet option as changing it to a pointer can allow the caller to modify the use info.

SGTM on the behavior of username:.

changing it to a pointer can allow the caller to modify the use info.

"don't do that"?

An API that is impossible to use incorrectly is better than one that can be used incorrectly...but sometimes tradeoffs have to be made.

And with a separate field that has to be manually set, we run the risk of it being set incorrectly, so this alternative doesn't seem any better to me.

If you really want to have a bool then it should be unexported and handled automatically, like how the passwordSet field works in the userinfo itself:

https://cs.opensource.google/go/go/+/refs/tags/go1.23.5:src/net/url/url.go;l=409

Otherwise I think a pointer, and a note telling people not to modify it after setting it, is pretty reasonable and simpler. And doesn't that simplify things? Then we can just directly copy the User field from the URL without all this extra work. Simpler is almost always better.

Got it ! Done.

dfawley · 2025-01-17T15:49:04Z

internal/testutils/proxyserver/proxyserver.go

@@ -98,17 +103,18 @@ func (p *ProxyServer) handleRequest(t *testing.T, in net.Conn, waitForServerHell
 }

 // HTTPProxy initializes and starts a proxy server, registers a cleanup to
-// stop it, and returns the proxy's listener and helper channels.
+// stop it, and returns a ProxyServer.
 func HTTPProxy(t *testing.T, reqCheck func(*http.Request), waitForServerHello bool) *ProxyServer {


dfawley · 2025-01-17T16:11:56Z

internal/transport/proxy.go

-		u := t.Username()
-		p, _ := t.Password()
+
+	if user := opts.User; user != (url.Userinfo{}) {


SGTM on the behavior of username:.

changing it to a pointer can allow the caller to modify the use info.

"don't do that"?

An API that is impossible to use incorrectly is better than one that can be used incorrectly...but sometimes tradeoffs have to be made.

And with a separate field that has to be manually set, we run the risk of it being set incorrectly, so this alternative doesn't seem any better to me.

If you really want to have a bool then it should be unexported and handled automatically, like how the passwordSet field works in the userinfo itself:

https://cs.opensource.google/go/go/+/refs/tags/go1.23.5:src/net/url/url.go;l=409

Otherwise I think a pointer, and a note telling people not to modify it after setting it, is pretty reasonable and simpler. And doesn't that simplify things? Then we can just directly copy the User field from the URL without all this extra work. Simpler is almost always better.

dfawley · 2025-01-17T16:22:58Z

internal/testutils/proxy.go

+	}()
+	t.Logf("Started proxy at: %q", pLis.Addr())
+	t.Cleanup(p.stop)
+	p.Addr = fmt.Sprintf("localhost:%d", ParsePort(t, pLis.Addr().String()))


Oh true it is converted to IP address. But why is that a problem?

If it's for testing the delegating resolver -> DNS resolver path, then it seems like this should just use the raw listener address, and the tests that care about that should convert it.

Either way, definitely needs a comment.

dfawley

One last thing otherwise LGTM. Thanks!

dfawley · 2025-01-22T16:15:34Z

internal/testutils/proxyserver/proxyserver.go

 // stop it, and returns a ProxyServer.
-func HTTPProxy(t *testing.T, reqCheck func(*http.Request), waitForServerHello bool) *ProxyServer {
+func NewHTTPProxy(t *testing.T, reqCheck func(*http.Request), waitForServerHello bool) *ProxyServer {


The thing it returns is a ProxyServer from package proxyserver so this should just be New.

Right! Done!

…ons for target host resolution (grpc#7881) * Change proxy behaviour

eshitachandwani added the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Nov 29, 2024

eshitachandwani added this to the 1.69 Release milestone Nov 29, 2024

eshitachandwani added the Type: Bug label Nov 29, 2024

eshitachandwani requested a review from purnesh42H November 30, 2024 10:44

eshitachandwani assigned purnesh42H Nov 30, 2024

Change proxy behaviour

619dcd4

eshitachandwani force-pushed the proxy_pr2 branch from 7fa029c to 619dcd4 Compare December 3, 2024 04:20

purnesh42H reviewed Dec 3, 2024

View reviewed changes

test/proxy_test.go Outdated Show resolved Hide resolved

test/proxy_test.go Outdated Show resolved Hide resolved

test/proxy_test.go Outdated Show resolved Hide resolved

eshitachandwani added 3 commits December 5, 2024 00:07

change delegating resolver

65b33bd

Improving code

94364e2

correct import

9f6a067

eshitachandwani requested a review from purnesh42H December 5, 2024 06:15

improve tests

5ce86d0

arjan-bal self-requested a review December 5, 2024 07:31

arjan-bal self-assigned this Dec 5, 2024

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H reviewed Dec 5, 2024

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Dec 5, 2024

eshitachandwani added 6 commits December 5, 2024 17:05

address comments

fbef3a4

add warning and httpfunc test

342c332

add warning and httpfunc test

8726188

from delegating_pr

1cfb089

improve code

32d9de5

improve code

34b902f

eshitachandwani requested a review from purnesh42H December 8, 2024 11:55

eshitachandwani assigned purnesh42H Dec 8, 2024

change proxy_utils

32fa83c

eshitachandwani requested a review from dfawley January 16, 2025 07:32

eshitachandwani assigned dfawley and unassigned eshitachandwani Jan 16, 2025

dfawley reviewed Jan 16, 2025

View reviewed changes

dfawley assigned eshitachandwani and unassigned dfawley Jan 16, 2025

eshitachandwani added 2 commits January 17, 2025 03:30

addr comments

382bcb1

add userSet option

69037b0

eshitachandwani requested a review from dfawley January 17, 2025 08:52

eshitachandwani assigned dfawley and unassigned eshitachandwani Jan 17, 2025

dfawley reviewed Jan 21, 2025

View reviewed changes

arjan-bal modified the milestones: 1.70 Release, 1.71 Release Jan 22, 2025

change userinfo to pointer

127f485

eshitachandwani requested a review from dfawley January 22, 2025 11:01

change proxy server return ip address

2a4f24c

dfawley approved these changes Jan 23, 2025

View reviewed changes

dfawley assigned eshitachandwani and unassigned dfawley Jan 23, 2025

eshitachandwani and others added 3 commits January 24, 2025 00:45

chamge HTTPProxy to new

9f26c4f

Merge branch 'grpc:master' into proxy_pr2

a3dcaa7

Merge branch 'grpc:master' into proxy_pr2

dd7ee47

eshitachandwani merged commit 2fd426d into grpc:master Jan 24, 2025
15 checks passed

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Jan 30, 2025

transport,grpc: Integrate delegating resolver and introduce dial opti…

021aac0

…ons for target host resolution (grpc#7881) * Change proxy behaviour

MrAlias mentioned this pull request Feb 3, 2025

OTLP GRPC exporters don't respect domain NO_PROXY settings due to grpc bug open-telemetry/opentelemetry-go#5976

Open

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 13, 2025

transport,grpc: Integrate delegating resolver and introduce dial opti…

c9aa7dd

…ons for target host resolution (grpc#7881) * Change proxy behaviour

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 13, 2025

transport,grpc: Integrate delegating resolver and introduce dial opti…

58786c4

…ons for target host resolution (grpc#7881) * Change proxy behaviour

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 15, 2025

transport,grpc: Integrate delegating resolver and introduce dial opti…

d6da91d

…ons for target host resolution (grpc#7881) * Change proxy behaviour

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

eshitachandwani commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

purnesh42H Dec 3, 2024

purnesh42H Dec 3, 2024

eshitachandwani Dec 4, 2024

purnesh42H Dec 3, 2024

eshitachandwani Dec 4, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

purnesh42H Dec 5, 2024

eshitachandwani Dec 5, 2024

dfawley Jan 16, 2025

eshitachandwani Jan 16, 2025

dfawley Jan 17, 2025

dfawley Jan 16, 2025

eshitachandwani Jan 16, 2025

eshitachandwani Jan 17, 2025

dfawley Jan 17, 2025

eshitachandwani Jan 22, 2025

dfawley Jan 17, 2025

eshitachandwani Jan 22, 2025

dfawley Jan 17, 2025

dfawley Jan 17, 2025

dfawley left a comment

dfawley Jan 22, 2025

eshitachandwani Jan 23, 2025

		address := addr.Addr

		//if the ProxyConnectAddr is set in the aattribute, do a proxy dial.

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

transport,grpc: Integrate delegating resolver and introduce dial options for target host resolution #7881

Conversation

eshitachandwani commented Nov 29, 2024 • edited Loading

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshitachandwani commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading