internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857

eshitachandwani · 2024-11-20T09:52:48Z

As part of fixing #7556 , we create a new delegating resolver that that handles proxy configuration and delegates to child resolvers for target and proxy address resolution and does the following:

Use the "dns" resolver for proxy address resolution if environment variables are set.
Use the target's "path" portion directly if the scheme is "dns" and a proxy is configured and target resolution on client is not enabled, mimicking the old "passthrough" behavior.
Combine the resolved target and proxy addresses, setting the proxy CONNECT string attribute.

Coping the context verbatim form the issue:
Background :
Before grpc.NewClient was introduced, grpc.Dial was the primary way to establish gRPC connections. With grpc.Dial, the default "passthrough" resolver simply passed the target address unchanged to the transport layer. When grpc.NewClient was introduced, it switched to the "dns" resolver by default, which resolves the target address to IP addresses before passing them to the transport. This change inadvertently altered how proxies are handled when configured via environment variables. This behavior is inconsistent with what we expect i.e resolution to happen on proxy.

The complete proposal :

Remove environment variable detection from the transport layer. Instead, 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. This resolver does operations stated above
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: N/A

codecov · 2024-11-20T09:58:01Z

Codecov Report

Attention: Patch coverage is 70.28571% with 52 lines in your changes missing coverage. Please review.

Project coverage is 82.03%. Comparing base (e5a4eb0) to head (fc2c41f).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
.../resolver/delegatingresolver/delegatingresolver.go	68.86%	40 Missing and 12 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7857      +/-   ##
==========================================
- Coverage   82.21%   82.03%   -0.19%     
==========================================
  Files         379      381       +2     
  Lines       38261    38535     +274     
==========================================
+ Hits        31458    31612     +154     
- Misses       5514     5603      +89     
- Partials     1289     1320      +31

Files with missing lines	Coverage Δ
internal/proxyattributes/proxyattributes.go	`100.00% <100.00%> (ø)`
.../resolver/delegatingresolver/delegatingresolver.go	`68.86% <68.86%> (ø)`

... and 27 files with indirect coverage changes

purnesh42H

Some style and documentation comments on non test code in the first pass. Will review the test code for same next.

internal/envconfig/envconfig.go

internal/resolver/delegatingresolver/delegatingresolver.go

internal/envconfig/envconfig.go

internal/resolver/delegatingresolver/delegatingresolver.go

arjan-bal · 2024-12-19T07:22:45Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	}
+
+	if r.targetResolver == nil {
+		r.targetResolver = nopResolver{}


Just FYI, don't change this: An optimization for using such stateless no-op implementations is to have a single object of the struct that everyone uses. This saves allocating memory for multiple objects. Here it wouldn't help much.

FYI, empty structs are allocated specially, so if it's an empty struct it will not save memory to have a global instance of it. See this example:

https://go.dev/play/p/LmiMfy_XNP4

Running it (for me, right now) prints 0x574380 0x574380 -- that it matches shows there aren't any wasted allocations.

Not for the struct itself, at least. But if the struct is stored in an interface field, like it is here, then that interface has to be allocated to hold the struct's type + pointer. But this level of optimization also isn't important for us. :)

internal/resolver/delegatingresolver/delegatingresolver.go

arjan-bal

Some minor style comment, otherwise LGTM!

internal/resolver/delegatingresolver/delegatingresolver.go

arjan-bal · 2024-12-19T16:24:49Z

internal/resolver/delegatingresolver/delegatingresolver.go

+
+var (
+	logger = grpclog.Component("delegating-resolver")
+	//HTTPSProxyFromEnvironment will be overwritten in the tests


nit: Space missing after //.

arjan-bal · 2024-12-19T16:35:47Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		r.targetResolverState.Addresses = []resolver.Address{{Addr: target.Endpoint()}}
+		r.targetResolverState.Endpoints = []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: target.Endpoint()}}}}
+		r.targetResolverReady = true


Not actionable: This is essentially the same as using passthrough as the target resolver. If we need to avoid sending the dial option to the delegating resolver, the channel could send passthrough as the target resolver to get the same effect.

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

arjan-bal · 2024-12-19T16:50:06Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+// the delegating resolver creates only a target resolver and that the
+// addresses returned by the delegating resolver exactly match those returned
+// by the target resolver.
+func (s) TestDelegatingResolverNoProxyEnvVarsSet(t *testing.T) {


The package name is delegatingresolver_test, we don't need to add DelegatingResolver to all the test functions.

https://google.github.io/styleguide/go/decisions#package-vs-exported-symbol-name

Ignore my previous comment. The test name can be used to filter tests in subdirectories, so having the TestDelegatingResolver prefix is helpful.

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

internal/resolver/delegatingresolver/delegatingresolver.go

dfawley · 2024-12-19T21:10:28Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	targetResolverReady bool               // indicates if an update from the target resolver has been received
+	proxyResolverReady  bool               // indicates if an update from the proxy resolver has been received


Consider removing these and using nil for targetResolverState and nil for proxyAddrs to indicate that the state/addresses are "valid".

Having two different pieces of state that represent parts of the same thing should be avoided. I.e. what does it mean if "!targetResolverReady && len(targetResolverState.Endpoints) > 0"? That's illegal. So you can couple the two things into one field and avoid such illegal state combinations.

Changed the condition as well as change the field targetResolverState to a pointer *resolver.State to facilitate differentiation in a no update and an empty update from the target resolver in the condition. Because as per the flow, we want load balancer to handle an empty update.

internal/resolver/delegatingresolver/delegatingresolver.go

internal/proxyattributes/proxyattributes.go

internal/resolver/delegatingresolver/delegatingresolver.go

dfawley

LGTM pending the two small changes requested.

dfawley · 2024-12-20T21:21:10Z

internal/resolver/delegatingresolver/delegatingresolver.go

-	// curState contains all necessary information when passed to UpdateState.
-	// The state update is only sent after both the target and proxy resolvers
-	// have sent their updates, and curState has been updated with the combined
-	// addresses.
 	curState := *r.targetResolverState


Please move this down to where you commented about it.

dfawley · 2024-12-20T21:21:19Z

internal/resolver/delegatingresolver/delegatingresolver.go

@@ -259,12 +254,17 @@ func (r *delegatingResolver) updateProxyResolverState(state resolver.State) erro
 		logger.Infof("Addresses received from proxy resolver: %s", state.Addresses)
 	}
 	if len(state.Endpoints) > 0 {
+


Please remove this blank line

eshitachandwani added 6 commits November 20, 2024 13:12

delegating resolver

bccef6b

correct comments

6b89e0d

correct comments

7c2fb21

use env from envconfig

e8bb6bc

use env from envconfig

36d0573

improve comment and var

5a87284

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

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

eshitachandwani added the Type: Bug label Nov 20, 2024

eshitachandwani added 2 commits November 20, 2024 15:33

add comment

e05a895

add newaddress function

e724a9a

eshitachandwani requested a review from purnesh42H November 20, 2024 10:33

eshitachandwani added 2 commits November 21, 2024 01:05

add target resolution option and test

975e235

correct NewAddress function

4b8ade9

eshitachandwani assigned purnesh42H Nov 20, 2024

purnesh42H changed the title ~~resolver/delegatingresolver: default resolver for proxy support~~ internal/revolver: add a new delegating resolver as default resolver for proxy support Nov 21, 2024

purnesh42H changed the title ~~internal/revolver: add a new delegating resolver as default resolver for proxy support~~ internal/resolver: add a new delegating resolver as default resolver for proxy support Nov 21, 2024

purnesh42H reviewed Nov 21, 2024

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Nov 21, 2024

eshitachandwani added 3 commits November 22, 2024 12:12

improve style and comments

4ba3578

change variable name

fa55e5b

change variable name

df11d7c

eshitachandwani requested a review from purnesh42H November 22, 2024 06:59

eshitachandwani assigned purnesh42H and unassigned eshitachandwani Nov 22, 2024

purnesh42H reviewed Nov 22, 2024

View reviewed changes

purnesh42H assigned eshitachandwani and unassigned purnesh42H Nov 22, 2024

arjan-bal removed their assignment Dec 18, 2024

eshitachandwani added 3 commits December 18, 2024 21:49

address comments

a0a2e50

remove internal

ca2cc24

add no op res

1f1e7f5

eshitachandwani requested review from arjan-bal and dfawley December 18, 2024 23:26

eshitachandwani assigned dfawley and arjan-bal and unassigned eshitachandwani Dec 18, 2024

arjan-bal reviewed Dec 19, 2024

View reviewed changes

comments

402badf

arjan-bal approved these changes Dec 19, 2024

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Dec 19, 2024

eshitachandwani and others added 2 commits December 20, 2024 01:46

improve

163ad20

Merge branch 'grpc:master' into delegating_pr

db8b92c

dfawley reviewed Dec 19, 2024

View reviewed changes

eshitachandwani and others added 5 commits December 20, 2024 14:39

Merge branch 'grpc:master' into delegating_pr

6623eb5

address comments

c4bde98

change target resolver state to pointer

1c4b416

tests

5dc893c

pointer

69253c7

dfawley reviewed Dec 20, 2024

View reviewed changes

address comments

c5f8d6f

eshitachandwani requested a review from dfawley December 20, 2024 20:47

dfawley approved these changes Dec 20, 2024

View reviewed changes

dfawley removed their assignment Dec 20, 2024

eshitachandwani added 2 commits December 23, 2024 10:51

formatting

bbea8c4

address comments

fc2c41f

arjan-bal merged commit 063d352 into grpc:master Dec 23, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857

internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857

eshitachandwani commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

purnesh42H left a comment

arjan-bal Dec 19, 2024

dfawley Dec 19, 2024

eshitachandwani Dec 20, 2024

arjan-bal left a comment

arjan-bal Dec 19, 2024 •

edited

Loading

eshitachandwani Dec 19, 2024

arjan-bal Dec 19, 2024 •

edited

Loading

arjan-bal Dec 19, 2024

arjan-bal Dec 19, 2024

dfawley Dec 19, 2024

eshitachandwani Dec 20, 2024

dfawley left a comment

dfawley Dec 20, 2024

eshitachandwani Dec 23, 2024

eshitachandwani Dec 23, 2024

dfawley Dec 20, 2024

eshitachandwani Dec 23, 2024

		targetResolverReady bool // indicates if an update from the target resolver has been received
		proxyResolverReady bool // indicates if an update from the proxy resolver has been received

internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857

internal/resolver: introduce a new delegating resolver to handle both target URI and proxy address resolution #7857

Conversation

eshitachandwani commented Nov 20, 2024 • edited Loading

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

purnesh42H 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

arjan-bal left a comment

Choose a reason for hiding this comment

arjan-bal Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal Dec 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshitachandwani commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

arjan-bal Dec 19, 2024 •

edited

Loading

arjan-bal Dec 19, 2024 •

edited

Loading