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 71.34503% with 49 lines in your changes missing coverage. Please review.

Project coverage is 82.10%. Comparing base (e5a4eb0) to head (69253c7).

Files with missing lines	Patch %	Lines
.../resolver/delegatingresolver/delegatingresolver.go	69.93%	38 Missing and 11 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7857      +/-   ##
==========================================
- Coverage   82.21%   82.10%   -0.12%     
==========================================
  Files         379      381       +2     
  Lines       38261    38432     +171     
==========================================
+ Hits        31458    31555      +97     
- Misses       5514     5576      +62     
- Partials     1289     1301      +12

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

... and 19 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

dfawley · 2024-12-17T18:12:19Z

internal/internal.go

+	// HTTPSProxyFromEnvironment returns the URL of the proxy to use
+	// for testing purposes. It is used to override the `http.ProxyFromEnvironment`
+	// function for testing purposes.
+	HTTPSProxyFromEnvironment any //func(*http.Request) (*url.URL, error)


Nit: space after slashes for consistency..

But also: why bother with making it an any? It can just have the proper type. The reason for the anys is to avoid circular dependencies, but the http and url packages in OSS will never import grpc/internal. :)

Or even, let's just move this directly into internal/resolver/delegatingresolver as an exported thing there since it won't need to be changed by any non-grpc code.

I asked to keep it any to limit the number of packages that internal imports.

External imports from internal should be fine. Internal imports from internal should be zero, or as close as possible.

Either way, I don't think this needs to be here at all.

dfawley · 2024-12-17T18:15:38Z

internal/proxyattributes/proxyattributes.go

+// Populate returns a copy of addr with attributes containing the provided user
+// and connect address, which are needed during the CONNECT handshake for a
+// proxy connection.
+func Populate(addr resolver.Address, opts Options) resolver.Address {


Nit: we use Set and Get for these same function names everywhere else; please rename for consistency.

There is a style guide recommendation against the use of the Get prefix for getters. See: go/go-style/decisions#getters

We have a few options with alternative verbs for Set. I like Extract here since it makes sense:

Rename Populate to SetOptions and rename Option to ExtractOptions

Rename Populate to SetConnectOptions and rename Option to ExtractConnectOptions and rename the struct Options to ConnectOptions since Options is very generic even with the package name added to it, i.e. proxyattributes.Options

As a last option, we could get rid of the options struct altogether and go with

func SetConnectOptions(addr resolver.Address, user url.UserInfo, connectAddr string) resolver.Address) {}

func ExtractConnectOptions(addr resolver.Address) (user url.UserInfo, connectAddr string, ok bool) {}

Went with Populate to SetOptions and rename Option to ExtractOptions

We already use Get and Set here quite extensively, and I don't think would be considered a getter/setter, either.

grpc-go/internal/hierarchy/hierarchy.go

Line 64 in b3bdacb

func Get(addr resolver.Address) []string {

grpc-go/internal/metadata/metadata.go

Line 61 in b3bdacb

func Get(addr resolver.Address) metadata.MD {

If the name of the package is the thing you want to set/get in the attributes, then it works out very naturally in usages.

Done. Changed to Get and Set

I'm clearly not reading things properly. Yes, Set and Get work great here since they would show up as proxyattributes.Get and proxyattributes.Set.

In the meantime, I also had a discussion with some folks on the readability team, trying to understand why they don't recommend the use of Get prefix for getters. A few things to note from that conversation:

Mostly getters and setters refer to methods on a type. So, package level functions that get and set something don't count here.

There is no clear rationale for why a Get prefix is not recommended other than: https://go.dev/doc/effective_go#Getters (and the fact that this recommendation has been around for a long time and most existing code follows this pattern)

And the pattern suggested by @dfawley in this comment is also a recommended way of doing things for package level functions.

internal/resolver/delegatingresolver/delegatingresolver.go

dfawley · 2024-12-17T18:24:41Z

internal/resolver/delegatingresolver/delegatingresolver.go

+}
+
+func (r *delegatingResolver) ResolveNow(o resolver.ResolveNowOptions) {
+	if r.targetResolver != nil {


Consider implementing a nopResolver instead that just ignores all calls, so you don't need to spread != nil checks throughout the code (and get a panic if you forget one).

But since delegating resolver is the default resolver in most cases, doesn't it make sense that if ClientConn calls ResolveNow or Close , we propagate the same to the child resolvers?

What I meant:

func New() { r := delegatingResolver{} if useTargetResolver { r.targetResolver = build() } else { r.targetResolver = nopResolver{} } if useProxyResolver { r.proxyResolver = build() } else { r.proxyResolver = nopResolver{} }

Then you never need to check if these fields are nil, since they are always initialized.

internal/resolver/delegatingresolver/delegatingresolver.go

internal/resolver/delegatingresolver/delegatingresolver_ext_test.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.

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

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+// mock the DNS resolution for the proxy resolver. It also registers the
+// original DNS resolver after the test is done.
+func setupDNS(t *testing.T) *manual.Resolver {
+	mr := manual.NewBuilderWithScheme("dns")


Call t.Helper() at the top to mark this is a test helper for better error reporting.

https://google.github.io/styleguide/go/decisions#test-helpers

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.

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

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+		delegatingresolver.HTTPSProxyFromEnvironment = originalhpfe
+	}()
+
+	targetResolver := manual.NewBuilderWithScheme("dns") // Manual resolver to control the target resolution.


nit: Move the code comments to newlines everywhere to be consistent.

arjan-bal · 2024-12-19T16:53:04Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+// createTestResolverClientConn initializes a [testutils.ResolverClientConn] and
+// returns it along with channels for resolver state updates and errors.
+func createTestResolverClientConn(t *testing.T) (*testutils.ResolverClientConn, chan resolver.State, chan error) {
+	stateCh := make(chan resolver.State, 1)


Mark this as a test helper.

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

dfawley reviewed Dec 17, 2024

View reviewed changes

arjan-bal self-assigned this Dec 18, 2024

arjan-bal reviewed Dec 18, 2024

View reviewed changes

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

		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

Are you sure you want to change the base?

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

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

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

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 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