delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env #8329

eshitachandwani · 2025-05-15T11:46:03Z

Add a check for resolved addresses to avoid proxy for addresses mentioned in NO_PROXY environment variable. We only had a check for unresolved addresses.

RELEASE NOTES:

client: restore support for NO_PROXY environment variable when connecting to locally-resolved addresses. This fixes a regression and restores historical grpc-go behavior (use case 2 from gRFC A1 ).

codecov · 2025-05-15T13:51:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.29%. Comparing base (1ecde18) to head (11903ba).
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8329      +/-   ##
==========================================
+ Coverage   82.19%   82.29%   +0.09%     
==========================================
  Files         419      419              
  Lines       42025    42048      +23     
==========================================
+ Hits        34543    34603      +60     
+ Misses       6012     5986      -26     
+ Partials     1470     1459      -11

Files with missing lines	Coverage Δ
.../resolver/delegatingresolver/delegatingresolver.go	`86.42% <100.00%> (+0.91%)`	⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

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

purnesh42H · 2025-05-15T19:40:58Z

I think release notes should mention the bug fix. How about something like client: fixed a bug that was attempting to use a proxy for addresses matching NO_PROXY environment variable?

purnesh42H · 2025-05-15T19:43:32Z

internal/resolver/delegatingresolver/delegatingresolver.go

@@ -241,7 +253,7 @@ func (r *delegatingResolver) updateClientConnStateLocked() error {
 	var addresses []resolver.Address
 	for _, targetAddr := range (*r.targetResolverState).Addresses {
 		// Avoid proxy when network is not tcp.
-		if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" {
+		if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" || !isProxyNeeded(targetAddr.Addr) {


Suggested change

if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" || !isProxyNeeded(targetAddr.Addr) {

!isProxyNeeded(targetAddr.Addr) {

addresses = append(addresses, targetAddr)

continue

}

if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" {

addresses = append(addresses, targetAddr)

continue

}

to avoid checking network type if proxy is anyway not needed?

You can also combine both the network type check and NO_PROXY check in the same helper function

purnesh42H · 2025-05-15T19:45:29Z

internal/resolver/delegatingresolver/delegatingresolver.go

@@ -259,7 +271,7 @@ func (r *delegatingResolver) updateClientConnStateLocked() error {
 		var addrs []resolver.Address
 		for _, targetAddr := range endpt.Addresses {
 			// Avoid proxy when network is not tcp.
-			if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" {
+			if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" || !isProxyNeeded(targetAddr.Addr) {


same suggestion as above

purnesh42H · 2025-05-15T19:48:09Z

internal/resolver/delegatingresolver/delegatingresolver.go

@@ -210,6 +210,18 @@ func isTCPAddressPresent(state *resolver.State) bool {
 	return false
 }

+func isProxyNeeded(address string) bool {


how about skipProxy?

purnesh42H · 2025-05-15T20:02:33Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+			Host:   envProxyAddr,
+		}, nil
+	}
+	originalhpfe := delegatingresolver.HTTPSProxyFromEnvironment


i don't have much context but is it possible to set the NO_PROXY env var for this test instead of just overriding from here? That would be a more reliable test.

I found one example here https://github.com/grpc/grpc-go/blob/master/stats/opentelemetry/csm/pluginoption_test.go#L437

Also might be good to verify putting in the IP range as mention in the bug?

I remember having some problem with using the proxy environment variable and so we switched to replacing the function.
SOmething on the lines that the function used to retrieve proxy address uses Do.Once function and that gets done the first time we run the test suite and so every time we do not get the value set in proxy environments and so we decided to replace the function in tests.

purnesh42H · 2025-05-15T20:08:58Z

I don't have much knowledge about the recent changes here. So I will defer to @easwars and @arjan-bal

internal/resolver/delegatingresolver/delegatingresolver.go

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

arjan-bal · 2025-05-16T17:58:16Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		return true
+	}
+	// Avoid proxy when network is not tcp.
+	if networkType := networkTypeFromAddr(address); networkType != "tcp" {


We can make isTCPAddressPresent use skipProxy instead and inline networkTypeFromAddr . isTCPAddressPresent would also need to be renamed to something more appropriate then. This would unify the checks in both places.

Can we inline networkTypeFromAddr since there is only one caller now? This would also prevent people from accidentally bypassing the "ignore proxy" check in the future.

Right! Missed that. Done!

arjan-bal

LGTM

arjan-bal · 2025-05-19T06:36:52Z

Please open a flake for the failing advancedtls test and assign it to someone from the security team.

https://github.com/grpc/grpc-go/actions/runs/15105499577/job/42453426886?pr=8329

nicholasngai · 2025-05-20T20:35:01Z

FWIW, I think it is worth mentioning that this behavior is probably non-idiomatic and is really only being put in for backwards compatibility. For example, it would not make sense for dns:///localhost:1234 to match NO_PROXY=127.0.0.1—browsers and anyone else checking NO_PROXY traditionally only match against the target URL, not the underlying resolved addresses.

There is potential for inconsistent behavior here: For example, this would mean that dns:///example.com:1234 with NO_PROXY=1.2.3.4 would route to the proxy if the DNS for example.com pointed to 5.6.7.8 one day but then not route to the proxy if the DNS pointed to 1.2.3.4 the next day—applications generally would only match against NO_PROXY=example.com instead.

eshitachandwani · 2025-05-21T19:34:45Z

FWIW, I think it is worth mentioning that this behavior is probably non-idiomatic and is really only being put in for backwards compatibility. For example, it would not make sense for dns:///localhost:1234 to match NO_PROXY=127.0.0.1—browsers and anyone else checking NO_PROXY traditionally only match against the target URL, not the underlying resolved addresses.

There is potential for inconsistent behavior here: For example, this would mean that dns:///example.com:1234 with NO_PROXY=1.2.3.4 would route to the proxy if the DNS for example.com pointed to 5.6.7.8 one day but then not route to the proxy if the DNS pointed to 1.2.3.4 the next day—applications generally would only match against NO_PROXY=example.com instead.

IMO including IP addresses in the NO_PROXY environment variable should not be considered inconsistent behavior. The Go proxy configuration explicitly supports IP addresses in NO_PROXY, and it is common for users to intentionally set this environment variable to exclude certain internal or reserved IP ranges from proxying. This is a practical and expected use case—particularly in environments where internal services are hosted on private network ranges. This behavior aligns with Go’s documented proxy semantics and is consistent with standard practices in private and enterprise deployments.

dfawley · 2025-05-21T20:04:02Z

I agree we should support IP addresses, but it seems we should also support domain names here, too. If the primary resolver is DNS and NO_PROXY includes the hostname we are looking up, we should not use a proxy.

eshitachandwani · 2025-05-21T20:10:41Z

I agree we should support IP addresses, but it seems we should also support domain names here, too. If the primary resolver is DNS and NO_PROXY includes the hostname we are looking up, we should not use a proxy.

Right! That is there, I have not changed that! Just added another check for IP address also after the addresses are resolved.

dfawley · 2025-05-21T21:51:27Z

I see. So this is happening later in the process, after addresses are resolved, when we are doing local resolution. And since our behavior for many years had been to respect http_proxy and no_proxy for locally-resolved addresses, this change is restoring that behavior.

This is basically use case 2 from gRFC A1. And I think as long as we are doing local name resolution, then it is pretty reasonable to check no_proxy for these resolved addresses. And since it's also restoring backward compatibility, then there's no reason to not do this. Note that Java doesn't support use case 2, and C++ doesn't use the environment variables for that use case, but custom logic instead.

dfawley · 2025-05-22T15:27:51Z

internal/resolver/delegatingresolver/delegatingresolver.go

 	// If no addresses returned by resolver have network type as tcp , do not
 	// wait for proxy update.


Please update.

dfawley · 2025-05-22T15:34:29Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

@@ -177,14 +177,10 @@ func (s) TestDelegatingResolverwithDNSAndProxyWithTargetResolution(t *testing.T)
 		resolvedProxyTestAddr1  = "11.11.11.11:7687"
 	)
 	hpfe := func(req *http.Request) (*url.URL, error) {


There seems to be a lot of repetition of this, which required you to change it in many many places. Can you factor this out and share it between the test cases since it seems to be identical every time?

dfawley · 2025-05-22T15:35:38Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+// the delegating resolver doesn't add proxyatrribute to adresses excluded , but
+// adds the proxyattribute to all other addresses.


Please make sure you don't have spaces before commas. (I've noticed this elsewhere, too.)

dfawley · 2025-05-22T15:37:02Z

internal/transport/proxy_ext_test.go

@@ -97,14 +97,10 @@ func (s) TestGRPCDialWithProxy(t *testing.T) {

 	// Overwrite the function in the test and restore them in defer.
 	hpfe := func(req *http.Request) (*url.URL, error) {


Same comments for this file re: re-use.

grpc#8329)

#8329) (#8352)

#8329) (#8354)

#8329) (#8353)

no_proxy

6c3d4d9

eshitachandwani added this to the 1.73 Release milestone May 15, 2025

eshitachandwani requested a review from purnesh42H May 15, 2025 11:46

eshitachandwani assigned purnesh42H May 15, 2025

remove comment

3d6580a

eshitachandwani requested review from dfawley and removed request for purnesh42H and dfawley May 15, 2025 11:51

eshitachandwani unassigned purnesh42H May 15, 2025

add test

d4375b5

eshitachandwani added Type: Bug Area: Resolvers/Balancers labels May 15, 2025

eshitachandwani and others added 4 commits May 15, 2025 18:28

Merge branch 'grpc:master' into derr

0e24ae9

update

b6d2512

update

d0dc0d6

race

8991ff0

eshitachandwani requested a review from purnesh42H May 15, 2025 14:23

eshitachandwani assigned purnesh42H May 15, 2025

eshitachandwani requested a review from easwars May 15, 2025 14:23

eshitachandwani assigned easwars May 15, 2025

purnesh42H reviewed May 15, 2025

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H May 15, 2025

refactor

362a092

eshitachandwani requested a review from arjan-bal May 16, 2025 06:45

eshitachandwani assigned arjan-bal May 16, 2025

arjan-bal reviewed May 16, 2025

View reviewed changes

arjan-bal removed their assignment May 16, 2025

refactor

e5a0dd1

eshitachandwani requested a review from arjan-bal May 18, 2025 16:04

eshitachandwani assigned arjan-bal and unassigned eshitachandwani May 18, 2025

arjan-bal assigned eshitachandwani and unassigned arjan-bal May 19, 2025

refactor

8d743ea

arjan-bal approved these changes May 19, 2025

View reviewed changes

eshitachandwani removed their assignment May 19, 2025

dfawley approved these changes May 22, 2025

View reviewed changes

dfawley assigned eshitachandwani and unassigned easwars May 22, 2025

refactor

11903ba

eshitachandwani merged commit 443caad into grpc:master May 23, 2025
22 of 25 checks passed

eshitachandwani added a commit to eshitachandwani/grpc-go that referenced this pull request May 23, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

8be06c5

grpc#8329)

eshitachandwani mentioned this pull request May 23, 2025

Cherry-pick #8329 to v1.71.x #8352

Merged

eshitachandwani added a commit to eshitachandwani/grpc-go that referenced this pull request May 23, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

531df95

grpc#8329)

eshitachandwani mentioned this pull request May 23, 2025

Cherry-pick #8329 to v1.72.x #8353

Merged

eshitachandwani added a commit to eshitachandwani/grpc-go that referenced this pull request May 23, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

3429088

grpc#8329)

eshitachandwani mentioned this pull request May 23, 2025

Cherry-pick #8329 to v1.73.x #8354

Merged

eshitachandwani added a commit that referenced this pull request May 23, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

b50e3c2

#8329) (#8352)

eshitachandwani added a commit that referenced this pull request May 23, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

b610465

#8329) (#8354)

eshitachandwani added a commit that referenced this pull request May 24, 2025

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env (

eef8c9c

#8329) (#8353)

-		if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" || !isProxyNeeded(targetAddr.Addr) {
+                !isProxyNeeded(targetAddr.Addr) {
+                    addresses = append(addresses, targetAddr)
+                    continue
+                }
+		if networkType := networkTypeFromAddr(targetAddr); networkType != "tcp" {
+		     addresses = append(addresses, targetAddr)
+                      continue
+		}

		// If no addresses returned by resolver have network type as tcp , do not
		// wait for proxy update.

		// the delegating resolver doesn't add proxyatrribute to adresses excluded , but
		// adds the proxyattribute to all other addresses.

		@@ -97,14 +97,10 @@ func (s) TestGRPCDialWithProxy(t *testing.T) {

		// Overwrite the function in the test and restore them in defer.
		hpfe := func(req http.Request) (url.URL, error) {

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env #8329

delegatingresolver: avoid proxy for resolved addresses in NO_PROXY env #8329

Uh oh!

Conversation

eshitachandwani commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

purnesh42H commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purnesh42H commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

arjan-bal commented May 19, 2025

Uh oh!

nicholasngai commented May 20, 2025

Uh oh!

eshitachandwani commented May 21, 2025

Uh oh!

dfawley commented May 21, 2025

Uh oh!

eshitachandwani commented May 21, 2025

Uh oh!

dfawley commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eshitachandwani commented May 15, 2025 •

edited

Loading

codecov bot commented May 15, 2025 •

edited

Loading

purnesh42H May 15, 2025 •

edited

Loading

purnesh42H May 15, 2025 •

edited

Loading