Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
751dd5e
no proxy if not tcp
eshitachandwani Apr 2, 2025
4476246
endpoint check
eshitachandwani Apr 2, 2025
39d3ded
passing tests
eshitachandwani Apr 2, 2025
fcbc507
test
eshitachandwani Apr 2, 2025
618877a
correct test
eshitachandwani Apr 2, 2025
d150d8e
Merge branch 'master' into d_error
eshitachandwani Apr 2, 2025
325e8cb
merge conflicts
eshitachandwani Apr 2, 2025
f2a6133
check
eshitachandwani Apr 7, 2025
8742ea7
var change
eshitachandwani Apr 7, 2025
607ec91
var change
eshitachandwani Apr 7, 2025
d254b04
refactor
eshitachandwani Apr 11, 2025
cfe7e4f
comments
eshitachandwani Apr 14, 2025
e8e8438
comments
eshitachandwani Apr 14, 2025
b941b53
proxy resolver
eshitachandwani Apr 21, 2025
fc393ba
remove bool
eshitachandwani Apr 21, 2025
00c62de
make channel buffered
eshitachandwani Apr 21, 2025
a98a62f
make channel buffered
eshitachandwani Apr 21, 2025
2d449cb
fix non tcp after tcp
eshitachandwani Apr 23, 2025
0f831a1
rough
eshitachandwani Apr 23, 2025
aa60db2
correct test
eshitachandwani Apr 24, 2025
854daca
correct test
eshitachandwani Apr 24, 2025
d4942ba
correct test
eshitachandwani Apr 24, 2025
821475e
correct test
eshitachandwani Apr 24, 2025
a3fa521
refactor
eshitachandwani Apr 24, 2025
9b384af
refactor
eshitachandwani Apr 24, 2025
8f157d8
blank line
eshitachandwani Apr 24, 2025
94c1f47
blank line
eshitachandwani Apr 24, 2025
7c5ad78
test
eshitachandwani Apr 25, 2025
1aa9f1c
test
eshitachandwani Apr 28, 2025
0049fa9
minor changes
eshitachandwani Apr 28, 2025
bf57c2c
assignment
eshitachandwani Apr 29, 2025
6e77ff3
minors
eshitachandwani Apr 30, 2025
fa78c83
minor
eshitachandwani May 2, 2025
1679375
goroutine order comment
eshitachandwani May 6, 2025
0a50ca5
comment
eshitachandwani May 6, 2025
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
106 changes: 81 additions & 25 deletions internal/resolver/delegatingresolver/delegatingresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (

"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal/proxyattributes"
"google.golang.org/grpc/internal/transport"
"google.golang.org/grpc/internal/transport/networktype"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/serviceconfig"
)
Expand All @@ -44,9 +46,10 @@ var (
//
// It implements the [resolver.Resolver] interface.
type delegatingResolver struct {
target resolver.Target // parsed target URI to be resolved
cc resolver.ClientConn // gRPC ClientConn
proxyURL *url.URL // proxy URL, derived from proxy environment and target
target resolver.Target // parsed target URI to be resolved
cc resolver.ClientConn // gRPC ClientConn
proxyURL *url.URL // proxy URL, derived from proxy environment and target
proxyResolverCh chan struct{} // closed once the proxy resolver has been created

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need a channel here, instead we can have a bool named something like proxyResolverInitialized and have it protected by childMu. We're acquiring childMu to access proxyResolver anyways, we can rely on the mutex for synchronization.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we do , the most prominent use case being when ResolveNow is called before UpdateState. In that case , we want that proxy resolver is built first ( called in targetResolver's UpdateState ) before resolveNow for proxy resolver is called. And in that case we need an order, but using mutex, we cannot define the order in which the actions might execute , we also want to block until proxyResolver is built. So I think using channel is more appropriate. Is there a preference using channels or variables with mutexes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If ResolveNow is called by the channel before the first UpdateState call comes from the resolver, then I think we can safely ignore it. Or maybe I'm not understanding the situation you're describing. It would be helpful to list clear operations and their orders for things like this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right! that was the question because the recent test introduced was calling ResolveNow just after delegating resolver is built and we have now changed the policy for creating proxy resolver only on target resolver update


mu sync.Mutex // protects all the fields below
targetResolverState *resolver.State // state of the target resolver
Expand Down Expand Up @@ -95,8 +98,9 @@ func proxyURLForTarget(address string) (*url.URL, error) {
// resolution is enabled using the dial option.
func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions, targetResolverBuilder resolver.Builder, targetResolutionEnabled bool) (resolver.Resolver, error) {
Comment thread
easwars marked this conversation as resolved.
r := &delegatingResolver{
target: target,
cc: cc,
target: target,
cc: cc,
proxyResolverCh: make(chan struct{}),
}

var err error
Expand Down Expand Up @@ -128,6 +132,7 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti
Endpoints: []resolver.Endpoint{{Addresses: []resolver.Address{{Addr: target.Endpoint()}}}},
}
r.targetResolverState = &state
Comment thread
arjan-bal marked this conversation as resolved.
Outdated
r.updateTargetResolverState(*r.targetResolverState)
Comment thread
easwars marked this conversation as resolved.
} else {
wcc := &wrappingClientConn{
stateListener: r.updateTargetResolverState,
Expand All @@ -138,10 +143,6 @@ func New(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOpti
}
}

if r.proxyResolver, err = r.proxyURIResolver(opts); err != nil {
return nil, fmt.Errorf("delegating_resolver: failed to build resolver for proxy URL %q: %v", r.proxyURL, err)
}

if r.targetResolver == nil {
r.targetResolver = nopResolver{}
}
Expand Down Expand Up @@ -174,27 +175,55 @@ func (r *delegatingResolver) proxyURIResolver(opts resolver.BuildOptions) (resol

func (r *delegatingResolver) ResolveNow(o resolver.ResolveNowOptions) {
r.childMu.Lock()
defer r.childMu.Unlock()
r.targetResolver.ResolveNow(o)
r.proxyResolver.ResolveNow(o)
r.childMu.Unlock()
if _, ok := <-r.proxyResolverCh; !ok {
r.childMu.Lock()
r.proxyResolver.ResolveNow(o)
r.childMu.Unlock()
}

Comment thread
eshitachandwani marked this conversation as resolved.
Outdated
}

func (r *delegatingResolver) Close() {
r.childMu.Lock()
defer r.childMu.Unlock()
r.targetResolver.Close()
r.targetResolver = nil

r.proxyResolver.Close()
r.proxyResolver = nil
}

// updateClientConnStateLocked creates a list of combined addresses by
// pairing each proxy address with every target address. For each pair, it
// generates a new [resolver.Address] using the proxy address, and adding the
// target address as the attribute along with user info. It returns nil if
// either resolver has not sent update even once and returns the error from
// ClientConn update once both resolvers have sent update atleast once.
func networkTypeFromAddr(addr resolver.Address) (string, resolver.Address) {
Comment thread
arjan-bal marked this conversation as resolved.
Outdated
networkType, ok := networktype.Get(addr)
if !ok {
networkType, addr.Addr = transport.ParseDialTarget(addr.Addr)
}
return networkType, addr
}

func tcpAddressPresent(state *resolver.State) bool {
Comment thread
arjan-bal marked this conversation as resolved.
Outdated
for _, addr := range state.Addresses {
if networkType, _ := networkTypeFromAddr(addr); networkType == "tcp" {
return true
}
}
for _, endpoint := range state.Endpoints {
for _, addr := range endpoint.Addresses {
if networktype, _ := networkTypeFromAddr(addr); networktype == "tcp" {
return true
}
}
}
return false
}

// updateClientConnStateLocked creates a list of combined addresses by pairing
// each proxy address with every target address. For each pair, it generates a
// new [resolver.Address] using the proxy address, and adding the target address
// as the attribute along with user info. It returns nil if either resolver has
// not sent update even once and returns the error from ClientConn update once
// both resolvers have sent update atleast once.
func (r *delegatingResolver) updateClientConnStateLocked() error {
if r.targetResolverState == nil || r.proxyAddrs == nil {
return nil
Expand All @@ -217,7 +246,12 @@ func (r *delegatingResolver) updateClientConnStateLocked() error {
proxyAddr = resolver.Address{Addr: r.proxyURL.Host}
}
var addresses []resolver.Address
for _, targetAddr := range (*r.targetResolverState).Addresses {
for _, targetAddr := range curState.Addresses {
Comment thread
arjan-bal marked this conversation as resolved.
Outdated
// Avoid proxy when network is not tcp.
if networkType, targetAddr := networkTypeFromAddr(targetAddr); networkType != "tcp" {
addresses = append(addresses, targetAddr)
continue
}
addresses = append(addresses, proxyattributes.Set(proxyAddr, proxyattributes.Options{
User: r.proxyURL.User,
ConnectAddr: targetAddr.Addr,
Expand All @@ -232,10 +266,15 @@ func (r *delegatingResolver) updateClientConnStateLocked() error {
// address.The resulting list of addresses is then grouped into endpoints,
// covering all combinations of proxy and target endpoints.
var endpoints []resolver.Endpoint
for _, endpt := range (*r.targetResolverState).Endpoints {
for _, endpt := range curState.Endpoints {
var addrs []resolver.Address
for _, proxyAddr := range r.proxyAddrs {
for _, targetAddr := range endpt.Addresses {
for _, targetAddr := range endpt.Addresses {
// Avoid proxy when network is not tcp.
if networkType, targetAddr := networkTypeFromAddr(targetAddr); networkType != "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,
Expand Down Expand Up @@ -297,8 +336,8 @@ func (r *delegatingResolver) updateProxyResolverState(state resolver.State) erro
// updateTargetResolverState updates the target resolver state by storing target
// addresses, endpoints, and service config, marking the resolver as ready, and
// 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.
// returns a non-nil error, it calls `ResolveNow()` on the proxy resolver. It is
// a StateListener function of wrappingClientConn passed to the target resolver.
func (r *delegatingResolver) updateTargetResolverState(state resolver.State) error {
r.mu.Lock()
defer r.mu.Unlock()
Expand All @@ -307,6 +346,22 @@ func (r *delegatingResolver) updateTargetResolverState(state resolver.State) err
logger.Infof("Addresses received from target resolver: %v", state.Addresses)
}
r.targetResolverState = &state
// If no addresses returned by resolver have network type as tcp , do not
// wait for proxy update.
if !tcpAddressPresent(r.targetResolverState) {
return r.cc.UpdateState(*r.targetResolverState)
}

go func() {
Comment thread
arjan-bal marked this conversation as resolved.
Outdated
r.childMu.Lock()
defer r.childMu.Unlock()
_, ok := r.proxyResolver.(nopResolver)
if r.proxyResolver == nil || ok {
r.proxyResolver, _ = r.proxyURIResolver(resolver.BuildOptions{})
close(r.proxyResolverCh)
}
}()

err := r.updateClientConnStateLocked()
if err != nil {
go func() {
Expand Down Expand Up @@ -335,7 +390,8 @@ func (wcc *wrappingClientConn) UpdateState(state resolver.State) error {
return wcc.stateListener(state)
}

// ReportError intercepts errors from the child resolvers and passes them to ClientConn.
// ReportError intercepts errors from the child resolvers and passes them to
// ClientConn.
func (wcc *wrappingClientConn) ReportError(err error) {
wcc.parent.cc.ReportError(err)
}
Expand Down
Loading