internal/delegatingresolver: avoid proxy if networktype of target address is not tcp #8215

eshitachandwani · 2025-04-02T05:32:28Z

As mentioned in gRFC A1 , we support TCP-level proxy through the HTTPS_PROXY or the HTTP_PROXY environment variables. So if the resolved addresses for target have addresses that do not have network type of tcp , proxy needs to be avoided for such addresses.
Changes made in delegating resolver:

Check if no addresses in the resolver state for target address has network type tcp. If it does not , do not wait for proxy resolver update since we do not want to connect to proxy for any address.
If even one of the address is tcp type , we wait for proxy resolver update and do the check on per address basis, modifying the address with tcp network type to indicate use of proxy and leave the other addresses as is in the new state update.

RELEASE NOTES:

Proxy connections are no longer attempted for targets with non-TCP network types.

codecov · 2025-04-02T07:08:46Z

Codecov Report

Attention: Patch coverage is 90.32258% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.12%. Comparing base (5edab9e) to head (fa78c83).
Report is 36 commits behind head on master.

Files with missing lines	Patch %	Lines
.../resolver/delegatingresolver/delegatingresolver.go	90.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8215      +/-   ##
==========================================
- Coverage   82.17%   82.12%   -0.05%     
==========================================
  Files         410      417       +7     
  Lines       40236    41412    +1176     
==========================================
+ Hits        33065    34011     +946     
- Misses       5822     5970     +148     
- Partials     1349     1431      +82

Files with missing lines	Coverage Δ
internal/transport/http2_client.go	`92.44% <100.00%> (+0.36%)`	⬆️
internal/transport/http_util.go	`92.06% <100.00%> (ø)`
.../resolver/delegatingresolver/delegatingresolver.go	`85.50% <90.00%> (+3.28%)`	⬆️

... and 98 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

arjan-bal · 2025-04-02T10:37:35Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	if len(curState.Endpoints) != 0 {
+		networkType, ok = networktype.Get(curState.Endpoints[0].Addresses[0])
+	} else {
+		networkType, ok = networktype.Get(curState.Addresses[0])
+	}


Here we're making an assumption that the network type of the first address of the endpoint is the same the address type of all remaining addresses. This may be true for most real-world usecases, but it isn't a requirement specified in the resolver API. It should not be too difficult to avoid proxying on a per-address basis. There are two places below where we do the following, one for resolver.State.Addresses and once for resolver.State.Endpoints:

proxyattributes.Set(proxyAddr, proxyattributes.Options{ User: r.proxyURL.User, ConnectAddr: targetAddr.Addr, })

We can refactor this into a method that takes in the target address and proxy address and returns the combined address.

fn (r *delegatingResolver) combineAddresses(targetAddr, proxyAddr resolver.Address) resolver.Address { if networktype.Get(targetAddr) != "tcp" { return targetAddr } return proxyattributes.Set(proxyAddr, proxyattributes.Options{ User: r.proxyURL.User, ConnectAddr: targetAddr.Addr, }) }

The above comment end up adding duplicate addresses in the loop on targetResolverState.Endpoints. We will probably need to invert the loop on r.proxyAddrs and endpt.Addresses and break early:

for _, endpt := range (*r.targetResolverState).Endpoints { var addrs []resolver.Address for _, targetAddr := range endpt.Addresses { if networktype.Get(targetAddr) != "tcp" { addrs = append(addrs, targetAddr) continue } for _, proxyAddr := range r.proxyAddrs { addrs = append(addrs, proxyattributes.Set(proxyAddr, proxyattributes.Options{ User: r.proxyURL.User, ConnectAddr: targetAddr.Addr, })) } } endpoints = append(endpoints, resolver.Endpoint{Addresses: addrs}) }

Right, done ! Also added a check in beginning , if there is not address with network type TCP , we do not wait for proxy update and just update the cc state.

arjan-bal · 2025-04-02T10:38:28Z

Please also fix the format of the release notes.

arjan-bal · 2025-04-08T04:29:17Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		if networkType, ok := networktype.Get(addr); !ok || networkType == "tcp" {
+			isTCP = true
+			break
+		}


Out here we're assuming !ok to be the same of tcp. However, the old code in the dialer used to parse the address string when !ok to determine the network type, we should probably maintain the same behaviour.

grpc-go/internal/transport/http2_client.go

Lines 177 to 182 in 172fc5b

if !ok {

networkType, address = parseDialTarget(address)

}

if networkType == "tcp" && useProxy {

return proxyDial(ctx, address, grpcUA)

}

arjan-bal · 2025-04-08T04:37:12Z

internal/resolver/delegatingresolver/delegatingresolver.go

+			if networkType, ok := networktype.Get(addr); !ok || networkType == "tcp" || isTCP {
+				isTCP = true
+				break
+			}


I think the code will be simpler if we refactor finding a TCP address in a helper function. The helper function can return early instead of keeping track of a isTCP boolean and breaking out of nested loops.

Right! Done!

arjan-bal · 2025-04-08T04:38:06Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		} else {
+			addresses = append(addresses, proxyattributes.Set(proxyAddr, proxyattributes.Options{
+				User:        r.proxyURL.User,
+				ConnectAddr: targetAddr.Addr,
+			}))
+		}


nit: You can use continue in the if block to avoid indenting the else block.

arjan-bal · 2025-04-08T04:38:15Z

internal/resolver/delegatingresolver/delegatingresolver.go

+			} else {
+				for _, proxyAddr := range r.proxyAddrs {
+					addrs = append(addrs, proxyattributes.Set(proxyAddr, proxyattributes.Options{
+						User:        r.proxyURL.User,
+						ConnectAddr: targetAddr.Addr,
+					}))
+				}
 			}


nit: You can use continue in the if block to avoid indenting the else block.

internal/resolver/delegatingresolver/delegatingresolver.go

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

internal/resolver/delegatingresolver/delegatingresolver.go

arjan-bal

LGTM with minor comments.

arjan-bal · 2025-04-29T20:10:49Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		go func() {
+			r.childMu.Lock()
+			defer r.childMu.Unlock()
+			if _, ok := r.proxyResolver.(nopResolver); ok {


nit: We can invert the check to return early, reducing one level of indentation.

Right! Done!

arjan-bal · 2025-04-29T20:11:36Z

internal/resolver/delegatingresolver/delegatingresolver.go

+			r.childMu.Lock()
+			defer r.childMu.Unlock()
+			if _, ok := r.proxyResolver.(nopResolver); ok {
+				proxyResolver, err := r.proxyURIResolver(resolver.BuildOptions{})


In case of failures, we will still be retrying creation of the proxy resolver every time the target resolver produces an update. Not sure if this is a problem, maybe other reviewers can chime in.

arjan-bal · 2025-04-29T20:18:32Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+	// ResolveNow of manual proxy resolver will not be called since proxy
+	// resolver is only built when we get the first update from target resolver
+	// and so , in the first resolveNow, proxy resolver will be a no-op resolver
+	// and only target resolver's ResolveNow will be called.


nit: Can you please break this long sentence up? There's also an extra space before the ,.

arjan-bal · 2025-04-29T20:19:46Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+	// Wait for proxy resolver to be built.
+	select {
+	case <-proxyResolverBuilt:
+	case <-time.After(defaultTestTimeout):


We should use the existing context here to ensure the test runs for a max of defaultTestTimeout and not defaultTestTimeout per assertion.

arjan-bal · 2025-04-29T20:22:51Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+	// resolver, since we want to avoid proxy for any network type apart from
+	// tcp.
+	if diff := cmp.Diff(gotState, wantState); diff != "" {
+		t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff)


nit: Since diff is a string, we should use %s to format it. This allows linters to catch data type mismatches.

arjan-bal · 2025-04-29T20:23:51Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+}
+
+// Tests the scenario where a proxy is configured, and the resolver returns
+// addresses with varied network type. The test verifies that the delegating


nit: I think you should be more precise and say "tcp and non-tcp addresses" instead of "varied".

arjan-bal · 2025-04-29T20:25:04Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+	select {
+	case <-stateCh:
+		t.Fatalf("Delegating resolver invoked UpdateState before both the proxy and target resolvers had updated their states.")
+	case <-time.After(defaultTestShortTimeout):


Same here, create a single context and use it to wait for the remaining assertions also.

defaultTestShortTimeout is used here because we do not want a state update , I dont think waiting for ctx.Done() will make sense here , and we wait at just one other place for defaultTestTimeout , do let me know if we should do ctx.Done() there?

internal/resolver/delegatingresolver/delegatingresolver.go

easwars · 2025-05-01T23:19:38Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	// Create a list of combined endpoints by pairing all proxy endpoints with
+	// every target endpoint. Each time, it constructs a new [resolver.Endpoint]
+	// using the all addresses from all the proxy endpoint and the target
+	// addresses from one endpoint. The target address and user information from
+	// the proxy URL are added as attributes to the proxy address.The resulting
+	// list of addresses is then grouped into endpoints, covering all
+	// combinations of proxy and target endpoints.


There is a lot of repetition between this comment and the one at the top of this method. Can you make this one more concise?

easwars · 2025-05-01T23:21:53Z

internal/resolver/delegatingresolver/delegatingresolver.go

-// triggering a state update if both resolvers are ready. If the ClientConn
-// returns a non-nil error, it calls `ResolveNow()` on the proxy resolver. It
-// is a StateListener function of wrappingClientConn passed to the target resolver.
+// updateTargetResolverState is the StateListener function provided to the


I see this StateListener being mentioned in a couple of places. There is no such method on the resolver.ClientConn interface.

Its a method on the wrappingClientConn , used to call different update state functions for proxy and target resolver. Should that not be mentioned in the comments? or is the use not clear ?

no proxy if not tcp

751dd5e

eshitachandwani added the Type: Bug label Apr 2, 2025

eshitachandwani added this to the 1.72 Release milestone Apr 2, 2025

eshitachandwani added the Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. label Apr 2, 2025

eshitachandwani added 2 commits April 2, 2025 11:12

endpoint check

4476246

passing tests

39d3ded

eshitachandwani and others added 4 commits April 2, 2025 14:18

test

fcbc507

correct test

618877a

Merge branch 'master' into d_error

d150d8e

merge conflicts

325e8cb

eshitachandwani added Type: Bug and removed Type: Bug labels Apr 2, 2025

eshitachandwani requested a review from arjan-bal April 2, 2025 09:53

eshitachandwani assigned arjan-bal Apr 2, 2025

arjan-bal reviewed Apr 2, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Apr 2, 2025

eshitachandwani modified the milestones: 1.72 Release, 1.73 Release Apr 3, 2025

eshitachandwani added 3 commits April 7, 2025 12:16

check

f2a6133

var change

8742ea7

var change

607ec91

eshitachandwani requested a review from arjan-bal April 7, 2025 08:07

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Apr 7, 2025

arjan-bal reviewed Apr 8, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Apr 8, 2025

blank line

94c1f47

eshitachandwani requested a review from easwars April 24, 2025 19:45

eshitachandwani assigned easwars and arjan-bal and unassigned eshitachandwani Apr 24, 2025

arjan-bal reviewed Apr 25, 2025

View reviewed changes

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go Outdated Show resolved Hide resolved

arjan-bal reviewed Apr 25, 2025

View reviewed changes

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go Outdated Show resolved Hide resolved

arjan-bal assigned eshitachandwani and unassigned arjan-bal Apr 25, 2025

test

7c5ad78

eshitachandwani requested a review from arjan-bal April 25, 2025 11:40

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Apr 25, 2025

arjan-bal reviewed Apr 28, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Apr 28, 2025

eshitachandwani added 2 commits April 28, 2025 17:52

test

1aa9f1c

minor changes

0049fa9

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Apr 29, 2025

assignment

bf57c2c

eshitachandwani requested a review from arjan-bal April 29, 2025 11:58

arjan-bal approved these changes Apr 29, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Apr 29, 2025

minors

6e77ff3

easwars approved these changes May 1, 2025

View reviewed changes

easwars removed their assignment May 1, 2025

minor

fa78c83

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/delegatingresolver: avoid proxy if networktype of target address is not tcp #8215

internal/delegatingresolver: avoid proxy if networktype of target address is not tcp #8215

eshitachandwani commented Apr 2, 2025 •

edited

Loading

codecov bot commented Apr 2, 2025 •

edited

Loading

arjan-bal Apr 2, 2025

arjan-bal Apr 2, 2025

eshitachandwani Apr 7, 2025

arjan-bal commented Apr 2, 2025

arjan-bal Apr 8, 2025

eshitachandwani Apr 11, 2025

arjan-bal Apr 8, 2025

eshitachandwani Apr 11, 2025

arjan-bal Apr 8, 2025

eshitachandwani Apr 11, 2025

arjan-bal Apr 8, 2025

eshitachandwani Apr 11, 2025

arjan-bal left a comment

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

arjan-bal Apr 29, 2025

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

arjan-bal Apr 29, 2025

eshitachandwani Apr 30, 2025

easwars May 1, 2025

eshitachandwani May 2, 2025

easwars May 1, 2025

eshitachandwani May 2, 2025

	if !ok {
	networkType, address = parseDialTarget(address)
	}
	if networkType == "tcp" && useProxy {
	return proxyDial(ctx, address, grpcUA)
	}

internal/delegatingresolver: avoid proxy if networktype of target address is not tcp #8215

Are you sure you want to change the base?

internal/delegatingresolver: avoid proxy if networktype of target address is not tcp #8215

Conversation

eshitachandwani commented Apr 2, 2025 • edited Loading

codecov bot commented Apr 2, 2025 • 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

arjan-bal commented Apr 2, 2025

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

arjan-bal left a 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

eshitachandwani commented Apr 2, 2025 •

edited

Loading

codecov bot commented Apr 2, 2025 •

edited

Loading