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: add optional interface to return channel authority #5360

Closed
2 of 7 tasks
easwars opened this issue May 16, 2022 · 7 comments · Fixed by #6752
Closed
2 of 7 tasks

grpc: add optional interface to return channel authority #5360

easwars opened this issue May 16, 2022 · 7 comments · Fixed by #6752
Assignees
Labels
Hacktoberfest P2 Type: Feature New features or improvements in behavior

Comments

@easwars
Copy link
Contributor

easwars commented May 16, 2022

This is one of the items mentioned in #4717 which did not get done. The below description has been copied over verbatim from #4717. Java and C already support this.

  • Determining :authority from the target URI must be the responsibility of the name resolver. See: HTTP2 authority header is not set to resolver.Address.ServerName (?) #4516
    • Add an optional interface, to be implemented by name resolvers or their builders, that contains a single method to return the channel’s authority.
      • This method must not perform any I/O, be non-blocking and the return value must not change over time.
      • This value will be used to populate the :authority pseudo-header, and serverName for TLS handshake by default.
      • gRPC will provide an implementation which conforms to RFC 3986 and returns the path portion of the target, after stripping query parameters. This will be used if the scheme’s name resolver does not provide a custom one.
      • This parsed target will be passed to load balancers in their BuildOptions, with the correct authority set.
@Aditya-Sood
Copy link
Contributor

hi @dfawley @ginayeh, can I pick this up?

@dfawley
Copy link
Member

dfawley commented Oct 17, 2023

Sure, thanks! Let's name the interface:

package resolver

// 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 other blocking operaitons.
	OverrideAuthority(target Target) string
}

And I think the only real motivation for this feature is to move this special case into the function to be implemented by the unix resolver's builder:

grpc-go/clientconn.go

Lines 1990 to 1993 in e14d583

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"

Around here:

func (b *builder) Scheme() string {

Which would also be a pretty good test for this functionality in practice.

One thing we need to decide is: do we do the escapeAuthority on the result of this function, to ensure it complies with RFC 3986? I'm of the opinion we should, but I'm not sure what Java and C++ do. They probably just assume it's the responsibility of the name resolver.

@github-actions
Copy link

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Oct 23, 2023
@Aditya-Sood
Copy link
Contributor

WIP, adding comment to prevent issue being closed

Copy link

github-actions bot commented Nov 1, 2023

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@easwars
Copy link
Contributor Author

easwars commented Nov 27, 2023

One thing we need to decide is: do we do the escapeAuthority on the result of this function, to ensure it complies with RFC 3986?

Did we make a decision on this one? Thanks.
@dfawley

@Aditya-Sood
Copy link
Contributor

hi @easwars, the PR doesn't include percent-encoding and expects it from the name resolver currently
(point 3 in #6752 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest P2 Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants