From eab9b33bd8819c756bc9876af4289323d7a0443f Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Fri, 27 Oct 2023 21:29:18 +0530 Subject: [PATCH 1/6] Increment: Adds AuthorityOverrider interface Also implements the same for unix resolver's builder --- clientconn.go | 8 +++----- internal/resolver/unix/unix.go | 4 ++++ resolver/resolver.go | 9 +++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/clientconn.go b/clientconn.go index c7bf6849f07e..db28a3c27843 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1981,16 +1981,14 @@ func (cc *ClientConn) determineAuthority() error { } endpoint := cc.parsedTarget.Endpoint() - target := cc.target + auth, isAuthOverrider := any(cc.resolverBuilder).(resolver.AuthorityOverrider) switch { case authorityFromDialOption != "": cc.authority = authorityFromDialOption case authorityFromCreds != "": cc.authority = authorityFromCreds - case strings.HasPrefix(target, "unix:") || strings.HasPrefix(target, "unix-abstract:"): - // TODO: remove when the unix resolver implements optional interface to - // return channel authority. - cc.authority = "localhost" + case isAuthOverrider: + cc.authority = auth.GetServiceAuthority(cc.parsedTarget) case strings.HasPrefix(endpoint, ":"): cc.authority = "localhost" + endpoint default: diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 160911687738..9b63ac4df2e0 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -61,6 +61,10 @@ func (b *builder) Scheme() string { return b.scheme } +func (b *builder) GetServiceAuthority(t resolver.Target) string { + return "localhost" +} + type nopResolver struct { } diff --git a/resolver/resolver.go b/resolver/resolver.go index c9411907d3e5..2f49c5fa2619 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -319,3 +319,12 @@ type Resolver interface { // Close closes the resolver. Close() } + +// AuthorityOverrider is implemented by Builders that wish to override the +// default authority for the ClientConn. +// By default, the authority used is target.Endpoint() +type AuthorityOverrider interface { + // OverrideAuthority returns the authority to use for a ClientConn with the + // given target. It must not perform I/O or any other blocking operations. + GetServiceAuthority(t Target) string +} From 5cc6bab20031721427f96471f6dadbcf9c6589bf Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Fri, 27 Oct 2023 22:06:37 +0530 Subject: [PATCH 2/6] Fix typo in comments --- resolver/resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resolver/resolver.go b/resolver/resolver.go index 2f49c5fa2619..3f87da3730af 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -324,7 +324,7 @@ type Resolver interface { // default authority for the ClientConn. // By default, the authority used is target.Endpoint() type AuthorityOverrider interface { - // OverrideAuthority returns the authority to use for a ClientConn with the + // GetServiceAuthority returns the authority to use for a ClientConn with the // given target. It must not perform I/O or any other blocking operations. GetServiceAuthority(t Target) string } From fa153011fcf04c08f9b82591e61e596ad9197abc Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Wed, 8 Nov 2023 11:26:56 +0530 Subject: [PATCH 3/6] Increment: Code review changes --- clientconn.go | 23 ++++++----------------- internal/resolver/unix/unix.go | 2 +- resolver/resolver.go | 4 ++-- 3 files changed, 9 insertions(+), 20 deletions(-) diff --git a/clientconn.go b/clientconn.go index db28a3c27843..d66a33cb1dc4 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1980,26 +1980,15 @@ func (cc *ClientConn) determineAuthority() error { return fmt.Errorf("ClientConn's authority from transport creds %q and dial option %q don't match", authorityFromCreds, authorityFromDialOption) } - endpoint := cc.parsedTarget.Endpoint() - auth, isAuthOverrider := any(cc.resolverBuilder).(resolver.AuthorityOverrider) - switch { - case authorityFromDialOption != "": + if authorityFromDialOption != "" { cc.authority = authorityFromDialOption - case authorityFromCreds != "": + } else if authorityFromCreds != "" { cc.authority = authorityFromCreds - case isAuthOverrider: - cc.authority = auth.GetServiceAuthority(cc.parsedTarget) - case strings.HasPrefix(endpoint, ":"): + } else if auth, isAuthOverrider := cc.resolverBuilder.(resolver.AuthorityOverrider); isAuthOverrider { + cc.authority = auth.OverrideAuthority(cc.parsedTarget) + } else if endpoint := cc.parsedTarget.Endpoint(); strings.HasPrefix(endpoint, ":") { cc.authority = "localhost" + endpoint - default: - // TODO: Define an optional interface on the resolver builder to return - // the channel authority given the user's dial target. For resolvers - // which don't implement this interface, we will use the endpoint from - // "scheme://authority/endpoint" as the default authority. - // Escape the endpoint to handle use cases where the endpoint - // might not be a valid authority by default. - // For example an endpoint which has multiple paths like - // 'a/b/c', which is not a valid authority by default. + } else { cc.authority = encodeAuthority(endpoint) } channelz.Infof(logger, cc.channelzID, "Channel authority set to %q", cc.authority) diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 9b63ac4df2e0..92af36054087 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -61,7 +61,7 @@ func (b *builder) Scheme() string { return b.scheme } -func (b *builder) GetServiceAuthority(t resolver.Target) string { +func (b *builder) OverrideAuthority(t resolver.Target) string { return "localhost" } diff --git a/resolver/resolver.go b/resolver/resolver.go index 3f87da3730af..bbaa6c9ea7da 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -324,7 +324,7 @@ type Resolver interface { // default authority for the ClientConn. // By default, the authority used is target.Endpoint() type AuthorityOverrider interface { - // GetServiceAuthority returns the authority to use for a ClientConn with the + // OverrideAuthority returns the authority to use for a ClientConn with the // given target. It must not perform I/O or any other blocking operations. - GetServiceAuthority(t Target) string + OverrideAuthority(Target) string } From b6617ece8506746485658346935d43a2eb159cd0 Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Thu, 16 Nov 2023 08:43:39 +0530 Subject: [PATCH 4/6] Code review refactors --- clientconn.go | 2 +- internal/resolver/unix/unix.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clientconn.go b/clientconn.go index d66a33cb1dc4..9c1b4cdfdfb6 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1984,7 +1984,7 @@ func (cc *ClientConn) determineAuthority() error { cc.authority = authorityFromDialOption } else if authorityFromCreds != "" { cc.authority = authorityFromCreds - } else if auth, isAuthOverrider := cc.resolverBuilder.(resolver.AuthorityOverrider); isAuthOverrider { + } else if auth, ok := cc.resolverBuilder.(resolver.AuthorityOverrider); ok { cc.authority = auth.OverrideAuthority(cc.parsedTarget) } else if endpoint := cc.parsedTarget.Endpoint(); strings.HasPrefix(endpoint, ":") { cc.authority = "localhost" + endpoint diff --git a/internal/resolver/unix/unix.go b/internal/resolver/unix/unix.go index 92af36054087..27cd81af9e5f 100644 --- a/internal/resolver/unix/unix.go +++ b/internal/resolver/unix/unix.go @@ -61,7 +61,7 @@ func (b *builder) Scheme() string { return b.scheme } -func (b *builder) OverrideAuthority(t resolver.Target) string { +func (b *builder) OverrideAuthority(resolver.Target) string { return "localhost" } From 1dd2231f5094fc14b3c1986d727603244575d844 Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Fri, 1 Dec 2023 09:35:37 +0530 Subject: [PATCH 5/6] Code review changes --- clientconn.go | 3 ++- resolver/resolver.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clientconn.go b/clientconn.go index 9c1b4cdfdfb6..3ec676c60b8e 100644 --- a/clientconn.go +++ b/clientconn.go @@ -1980,13 +1980,14 @@ func (cc *ClientConn) determineAuthority() error { return fmt.Errorf("ClientConn's authority from transport creds %q and dial option %q don't match", authorityFromCreds, authorityFromDialOption) } + endpoint := cc.parsedTarget.Endpoint() if authorityFromDialOption != "" { cc.authority = authorityFromDialOption } else if authorityFromCreds != "" { cc.authority = authorityFromCreds } else if auth, ok := cc.resolverBuilder.(resolver.AuthorityOverrider); ok { cc.authority = auth.OverrideAuthority(cc.parsedTarget) - } else if endpoint := cc.parsedTarget.Endpoint(); strings.HasPrefix(endpoint, ":") { + } else if strings.HasPrefix(endpoint, ":") { cc.authority = "localhost" + endpoint } else { cc.authority = encodeAuthority(endpoint) diff --git a/resolver/resolver.go b/resolver/resolver.go index bbaa6c9ea7da..4083e5ae2e1d 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -322,7 +322,7 @@ type Resolver interface { // AuthorityOverrider is implemented by Builders that wish to override the // default authority for the ClientConn. -// By default, the authority used is target.Endpoint() +// By default, the authority used is target.Endpoint(). type AuthorityOverrider interface { // OverrideAuthority returns the authority to use for a ClientConn with the // given target. It must not perform I/O or any other blocking operations. From fb654581a777cadb9e2cd4bee7cdaa2e55f1094b Mon Sep 17 00:00:00 2001 From: Aditya-Sood Date: Tue, 5 Dec 2023 14:37:12 +0530 Subject: [PATCH 6/6] Increment: Modifies description of OverrideAuthority() --- resolver/resolver.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/resolver/resolver.go b/resolver/resolver.go index 4083e5ae2e1d..24b32bd981a2 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -325,6 +325,7 @@ type Resolver interface { // By default, the authority used is target.Endpoint(). type AuthorityOverrider interface { // OverrideAuthority returns the authority to use for a ClientConn with the - // given target. It must not perform I/O or any other blocking operations. + // given target. The implementation must generate it without blocking, + // typically in line, and must keep it unchanged. OverrideAuthority(Target) string }