Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 75 commits into from
Dec 23, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
bccef6b
delegating resolver
eshitachandwani Nov 20, 2024
6b89e0d
correct comments
eshitachandwani Nov 20, 2024
7c2fb21
correct comments
eshitachandwani Nov 20, 2024
e8bb6bc
use env from envconfig
eshitachandwani Nov 20, 2024
36d0573
use env from envconfig
eshitachandwani Nov 20, 2024
5a87284
improve comment and var
eshitachandwani Nov 20, 2024
e05a895
add comment
eshitachandwani Nov 20, 2024
e724a9a
add newaddress function
eshitachandwani Nov 20, 2024
975e235
add target resolution option and test
eshitachandwani Nov 20, 2024
4b8ade9
correct NewAddress function
eshitachandwani Nov 20, 2024
4ba3578
improve style and comments
eshitachandwani Nov 22, 2024
fa55e5b
change variable name
eshitachandwani Nov 22, 2024
df11d7c
change variable name
eshitachandwani Nov 22, 2024
8bcf2b6
test example
eshitachandwani Nov 22, 2024
a5aeb9a
revert commit
eshitachandwani Nov 22, 2024
47e7936
cleanup test and address comments
eshitachandwani Nov 25, 2024
a2e1415
address comments
eshitachandwani Nov 26, 2024
df87a68
address comments
eshitachandwani Nov 26, 2024
ed3d577
add comment
eshitachandwani Nov 26, 2024
dd5bdb3
add comment
eshitachandwani Nov 27, 2024
d24ec8b
address comments
eshitachandwani Nov 27, 2024
74c6cd0
address comments
eshitachandwani Nov 27, 2024
31d8c2c
address comments
eshitachandwani Nov 28, 2024
1643768
mae unexported key
eshitachandwani Nov 28, 2024
4a5ab5b
add backoff
eshitachandwani Nov 29, 2024
fd061c7
add backoff
eshitachandwani Nov 29, 2024
c6dbcbc
Revert "add backoff"
eshitachandwani Nov 29, 2024
2be9db3
revert backoff
eshitachandwani Nov 29, 2024
67e3799
add attributes package
eshitachandwani Nov 29, 2024
f5e1d37
correct atrributes
eshitachandwani Nov 29, 2024
019f621
address comments
eshitachandwani Dec 4, 2024
8348b1f
change tests
eshitachandwani Dec 4, 2024
f0d5b7b
correct comment
eshitachandwani Dec 4, 2024
4ac31f9
improve code
eshitachandwani Dec 6, 2024
ed413f6
improve code
eshitachandwani Dec 6, 2024
402316c
add endpt support
eshitachandwani Dec 7, 2024
dea9ddd
improve
eshitachandwani Dec 9, 2024
40f1035
add comment
eshitachandwani Dec 9, 2024
95d98b9
Merge branch 'grpc:master' into delegating_pr
eshitachandwani Dec 9, 2024
d5f4d8f
address comments
eshitachandwani Dec 10, 2024
e743cbf
add endpnt support
eshitachandwani Dec 10, 2024
e008feb
improve endpt
eshitachandwani Dec 10, 2024
4aea07d
add test for endpoints
eshitachandwani Dec 10, 2024
ed345a1
add test for endpoints
eshitachandwani Dec 10, 2024
7f416b0
condition and comment for endpoint
eshitachandwani Dec 11, 2024
e68d5de
remove condition
eshitachandwani Dec 11, 2024
8848105
refactors
eshitachandwani Dec 12, 2024
61edca1
correct
eshitachandwani Dec 12, 2024
a01cc7a
change endpoint support
eshitachandwani Dec 13, 2024
845680d
correct endpoint behvaiour for DNS
eshitachandwani Dec 13, 2024
c752bd8
correct endpoint behvaiour for DNS
eshitachandwani Dec 13, 2024
d53100c
address comments
eshitachandwani Dec 16, 2024
99292ea
improve
eshitachandwani Dec 17, 2024
74b3cf6
improve
eshitachandwani Dec 17, 2024
c745144
improve
eshitachandwani Dec 17, 2024
628a2a4
test for addresses
eshitachandwani Dec 17, 2024
eeae936
correct var names
eshitachandwani Dec 17, 2024
bc0adeb
rerun test
eshitachandwani Dec 17, 2024
9c2ce28
address comments
eshitachandwani Dec 17, 2024
4482e51
address comments
eshitachandwani Dec 17, 2024
2bafe77
improve comment
eshitachandwani Dec 17, 2024
a0a2e50
address comments
eshitachandwani Dec 18, 2024
ca2cc24
remove internal
eshitachandwani Dec 18, 2024
1f1e7f5
add no op res
eshitachandwani Dec 18, 2024
402badf
comments
eshitachandwani Dec 19, 2024
163ad20
improve
eshitachandwani Dec 19, 2024
db8b92c
Merge branch 'grpc:master' into delegating_pr
eshitachandwani Dec 19, 2024
6623eb5
Merge branch 'grpc:master' into delegating_pr
eshitachandwani Dec 20, 2024
c4bde98
address comments
eshitachandwani Dec 20, 2024
1c4b416
change target resolver state to pointer
eshitachandwani Dec 20, 2024
5dc893c
tests
eshitachandwani Dec 20, 2024
69253c7
pointer
eshitachandwani Dec 20, 2024
c5f8d6f
address comments
eshitachandwani Dec 20, 2024
bbea8c4
formatting
eshitachandwani Dec 23, 2024
fc2c41f
address comments
eshitachandwani Dec 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions internal/envconfig/envconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ var (
// setting the environment variable "GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST"
// to "true".
NewPickFirstEnabled = boolFromEnv("GRPC_EXPERIMENTAL_ENABLE_NEW_PICK_FIRST", false)
//HTTPProxy is set to empty string if HTTPS_PROXY env it is not set.
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
HTTPProxy = stringFromEnv("HTTPS_PROXY", "")
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
)

func boolFromEnv(envVar string, def bool) bool {
Expand All @@ -79,3 +81,11 @@ func uint64FromEnv(envVar string, def, min, max uint64) uint64 {
}
return v
}

func stringFromEnv(envVar string, def string) string {
val := os.Getenv(envVar)
if val == "" {
return def
}
return val
}
241 changes: 241 additions & 0 deletions internal/resolver/delegatingresolver/delegatingresolver.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

// Package delegatingresolver implements the default resolver for gRPC.
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// It handles both target URI and proxy address resolution, unless:
// - A custom dialer is set using WithContextDialer dialoption.
// - Proxy usage is explicitly disabled using WithNoProxy dialoption.
// - Client-side resolution is explicitly enforced using WithTargetResolutionEnabled.
package delegatingresolver

import (
"fmt"
"net/http"
"net/url"
"sync"

"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/serviceconfig"
)

var (
// The following variable will be overwritten in the tests.
httpProxyFromEnvironment = http.ProxyFromEnvironment
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved

logger = grpclog.Component("delegatingresolver")
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
)

// delegatingResolver manages the resolution of both a target URI and an
// optional proxy URI. It acts as an intermediary between the underlying
// resolvers and the gRPC ClientConn. This type implements the
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// resolver.ClientConn interface to intercept state updates from both the target
// and proxy resolvers, combining the results before forwarding them to the
// client connection.
type delegatingResolver struct {
target resolver.Target // parsed target URI to be resolved
cc resolver.ClientConn // gRPC ClientConn
targetResolver resolver.Resolver // resolver for the target URI, based on its scheme
proxyResolver resolver.Resolver // resolver for the proxy URI; nil if no proxy is configured
targetAddrs []resolver.Address // resolved or unresolved target addresses, depending on proxy configuration
proxyAddrs []resolver.Address // resolved proxy addresses; empty if no proxy is configured
proxyURL *url.URL // proxy URL, derived from proxy environment and target
mu sync.Mutex // protects access to the resolver state during updates
easwars marked this conversation as resolved.
Show resolved Hide resolved
}

func mapAddress(address string) (*url.URL, error) {
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
req := &http.Request{
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
URL: &url.URL{
Scheme: "https",
Host: address,
},
}
url, err := httpProxyFromEnvironment(req)
if err != nil {
return nil, err
}
return url, nil
}

// New creates a new delegating resolver that will call the target and proxy
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// child resolvers based on the proxy enviorinment configurations.
func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions, targetResolverBuilder resolver.Builder, targetResolutionEnabled bool) (resolver.Resolver, error) {
dfawley marked this conversation as resolved.
Show resolved Hide resolved
r := &delegatingResolver{
easwars marked this conversation as resolved.
Show resolved Hide resolved
target: target,
cc: cc,
}

var err error
r.proxyURL, err = mapAddress(target.Endpoint())
if err != nil {
return nil, fmt.Errorf("failed to determine proxy URL for target : %v", err)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
}

if r.proxyURL == nil {
easwars marked this conversation as resolved.
Show resolved Hide resolved
logger.Info("No proxy URL detected")
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return targetResolverBuilder.Build(target, cc, opts)
}

// When the scheme is 'dns', resolution should be handled by the proxy, not
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// the client. Therefore, we bypass the target resolver and store the
// unresolved target address.
if target.URL.Scheme == "dns" && !targetResolutionEnabled {
r.targetAddrs = []resolver.Address{{Addr: target.Endpoint()}}
} else {
// targetResolverBuilder must not be nil. If it is nil, the channel should
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// error out and not call this function.
r.targetResolver, err = targetResolverBuilder.Build(target, &wrappingClientConn{r, "target"}, opts)
easwars marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("delegating_resolver: unable to build the target resolver : %v", err)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
}
}
r.proxyResolver, err = proxyURIResolver(r.proxyURL, opts, r)
if err != nil {
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
return r, nil
}

func proxyURIResolver(proxyURL *url.URL, opts resolver.BuildOptions, presolver *delegatingResolver) (resolver.Resolver, error) {
scheme := "dns"
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
proxyBuilder := resolver.Get(scheme)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
if proxyBuilder == nil {
return nil, fmt.Errorf("delegating_resolver: resolver for proxy not found")
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
}
host := "dns:///" + proxyURL.Host
u, err := url.Parse(host)
if err != nil {
return nil, err
}

proxyTarget := resolver.Target{URL: *u}
return proxyBuilder.Build(proxyTarget, &wrappingClientConn{presolver, "proxy"}, opts)
}

func (r *delegatingResolver) ResolveNow(o resolver.ResolveNowOptions) {
if r.targetResolver != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

r.targetResolver.ResolveNow(o)
}
if r.proxyResolver != nil {
r.proxyResolver.ResolveNow(o)
}
}

func (r *delegatingResolver) Close() {
if r.targetResolver != nil {
r.targetResolver.Close()
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
}
if r.proxyResolver != nil {
r.proxyResolver.Close()
}
}

type keyType string

const proxyConnectAddrKey = keyType("grpc.resolver.delegatingresolver.proxyConnectAddr")
const userKey = keyType("grpc.resolver.delegatingresolver.user")

// SetConnectAddr returns a copy of the provided resolver.Address with attributes
// containing address to be sent in connect request to proxy. It's data should
// not be mutated after calling SetConnectAddr.
func SetConnectAddr(resAddr resolver.Address, addr string) resolver.Address {
resAddr.Attributes = resAddr.Attributes.WithValue(proxyConnectAddrKey, addr)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return resAddr
}

// GetConnectAddr returns the proxy connect address in resolver.Address, or nil
// if not present. The returned data should not be mutated.
func GetConnectAddr(addr resolver.Address) string {
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
s, _ := addr.Attributes.Value(proxyConnectAddrKey).(string)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return s
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this blank line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// SetUser returns a copy of the provided address with attributes containing
// user info. It's data should not be mutated after calling SetUser.
func SetUser(addr resolver.Address, user *url.Userinfo) resolver.Address {
addr.Attributes = addr.Attributes.WithValue(userKey, user)
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return addr
}

// GetUser returns the user info in the resolver.Address, or nil if not present.
// The returned data should not be mutated.
func GetUser(addr resolver.Address) *url.Userinfo {
user, _ := addr.Attributes.Value(userKey).(*url.Userinfo)
return user
}

func (r *delegatingResolver) updateState(state resolver.State) resolver.State {
var addresses []resolver.Address
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
for _, proxyAddr := range r.proxyAddrs {
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
for _, targetAddr := range r.targetAddrs {
newAddr := resolver.Address{
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
Addr: proxyAddr.Addr,
}
newAddr = SetConnectAddr(newAddr, targetAddr.Addr)
if r.proxyURL.User != nil {
newAddr = SetUser(newAddr, r.proxyURL.User)
}
addresses = append(addresses, newAddr)
}
}
// Set the addresses in the current state.
state.Addresses = addresses
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
return state
}

type wrappingClientConn struct {
easwars marked this conversation as resolved.
Show resolved Hide resolved
parent *delegatingResolver
resolverType string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's figure out if we can get rid of this. Definitely a string should not be used. An enum (enum-lite, since this is Go) would be better. Can we maybe store a pointer to the appropriate one of parent.targetAddrs or parent.proxyAddrs instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed it to use the enum but not the pointer to the address as we will need to know which resolver is the update from to pass the service config of the target resolver to the LB policy.

}

// UpdateState intercepts state updates from the target and proxy resolvers.
func (wcc *wrappingClientConn) UpdateState(state resolver.State) error {
wcc.parent.mu.Lock()
defer wcc.parent.mu.Unlock()
easwars marked this conversation as resolved.
Show resolved Hide resolved
arjan-bal marked this conversation as resolved.
Show resolved Hide resolved
var curState resolver.State
if wcc.resolverType == "target" {
wcc.parent.targetAddrs = state.Addresses
curState = state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a bug to not do this unconditionally?

Copy link
Member Author

@eshitachandwani eshitachandwani Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not done it unconditionally because I do that so that the service config and the other state information can be passed and becomes the part of curState.
I am not sure as to why will it be a bug because this UpdateState is only called in case of proxy and not return state unless both resolvers send their update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment to explain why curState is updated only for updates from the target resolver would help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
if wcc.resolverType == "proxy" {
wcc.parent.proxyAddrs = state.Addresses
}

if len(wcc.parent.targetAddrs) == 0 || len(wcc.parent.proxyAddrs) == 0 {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot completely eat UpdateState calls. If a channel has zero addresses being reported by a resolver, that is an error, and the resolver needs to receive an error in response.

Copy link
Member Author

@eshitachandwani eshitachandwani Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see an error being returned by cc.UpdateState currently for zero addresses. As of today, the balancer returns an error for resolver produced zero addresses which should work for the delegating resolver as well.

I used this condition to check if both the proxy and the target resolver have sent the update. I missed the case that the resolvers could send 0 addresses as well and it might not work correctly.

I am changing the condition to check the updates from both resolvers and letting the balancer handle the zero address error, because if either of the resolvers send zero address, the balancer will get zero address and act accordingyly.

curState = wcc.parent.updateState(curState)
return wcc.parent.cc.UpdateState(curState)
}

// ReportError intercepts errors from the child resolvers and pass to ClientConn.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix grammar in this comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

func (wcc *wrappingClientConn) ReportError(err error) {
wcc.parent.cc.ReportError(err)
easwars marked this conversation as resolved.
Show resolved Hide resolved
}

// NewAddress intercepts the new resolved address from the chid resolvers and
purnesh42H marked this conversation as resolved.
Show resolved Hide resolved
// pass to CLientConn
func (wcc *wrappingClientConn) NewAddress(addrs []resolver.Address) {
wcc.UpdateState(resolver.State{Addresses: addrs})
}

// ParseServiceConfig parses the provided service config and returns an
// object that provides the parsed config.
func (wcc *wrappingClientConn) ParseServiceConfig(serviceConfigJSON string) *serviceconfig.ParseResult {
return wcc.parent.cc.ParseServiceConfig(serviceConfigJSON)
}
Loading