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

grpc: optional interface to provide channel authority #6752

Merged
merged 6 commits into from Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions clientconn.go
Expand Up @@ -1981,16 +1981,14 @@ func (cc *ClientConn) determineAuthority() error {
}

endpoint := cc.parsedTarget.Endpoint()
target := cc.target
auth, isAuthOverrider := any(cc.resolverBuilder).(resolver.AuthorityOverrider)
Copy link
Member

Choose a reason for hiding this comment

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

Why cast it to any first? That shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, cc.resolverBuilder is already an interface type
thanks

switch {
Copy link
Member

Choose a reason for hiding this comment

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

Optional: maybe just convert this into a few ifs and do the auth overrider as a compound if.

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:
Copy link
Member

Choose a reason for hiding this comment

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

Please delete the TODO below.

Expand Down
4 changes: 4 additions & 0 deletions internal/resolver/unix/unix.go
Expand Up @@ -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 {
}

Expand Down
9 changes: 9 additions & 0 deletions resolver/resolver.go
Expand Up @@ -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()
easwars marked this conversation as resolved.
Show resolved Hide resolved
type AuthorityOverrider interface {
// GetServiceAuthority returns the authority to use for a ClientConn with the
// given target. It must not perform I/O or any other blocking operations.
easwars marked this conversation as resolved.
Show resolved Hide resolved
GetServiceAuthority(t Target) string
Copy link
Member

Choose a reason for hiding this comment

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

Go style generally doesn't like GetXYZ. OverrideAuthority makes more sense given the name of the interface; otherwise AuthorityForTarget or something like that could be okay.

Copy link
Member

Choose a reason for hiding this comment

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

Please name the parameter target to be explicit, or don't name it at all, as t doesn't really add documentation value here, and it's not necessary to name.

}