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

dialing against registered schemes is not case-insensitive #6011

Closed
iamricard opened this issue Feb 8, 2023 · 2 comments · Fixed by #6014
Closed

dialing against registered schemes is not case-insensitive #6011

iamricard opened this issue Feb 8, 2023 · 2 comments · Fixed by #6014
Assignees

Comments

@iamricard
Copy link

What version of gRPC are you using?

1.45.0

What version of Go are you using (go version)?

go version go1.19.3 darwin/arm64

What operating system (Linux, Windows, …) and version?

Darwin

What did you do?

  • Registered custom resolver, with a scheme containing uppercase letters
  • Dialed a target with the same scheme
  • Registered customer resolver, all lowercase letters
  • Dialed a target with the mixed casing scheme
package main

import (
	"fmt"

	"google.golang.org/grpc"
	"google.golang.org/grpc/credentials/insecure"
	"google.golang.org/grpc/resolver"
)

var (
	_ resolver.Builder  = &resolverBuilder{}
	_ resolver.Resolver = &testResolver{}
)

type resolverBuilder struct {
	scheme string
}

type testResolver struct{}

func (r *resolverBuilder) Scheme() string {
	return r.scheme
}

func (r *resolverBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
	panic("built resolver")
}

func (r *testResolver) ResolveNow(opts resolver.ResolveNowOptions) {
}
func (r *testResolver) Close() {
}

func main() {
	resolver.Register(&resolverBuilder{scheme: "CasedScheme"})
	grpc.Dial("CasedScheme:///foo:6400", grpc.WithTransportCredentials(insecure.NewCredentials()))
	fmt.Println("did not panic")

	// will panic
	resolver.Register(&resolverBuilder{scheme: "casedscheme"})
	grpc.Dial("CasedScheme:///foo:6400", grpc.WithTransportCredentials(insecure.NewCredentials()))
}

What did you expect to see?

  • Dial should have picked up the custom resolver in both occasions, but didn't when the registered resolver had a scheme with mixed casing
  • Register should have overwritten/error'd when registering two resolvers with the same scheme, instead it registered both

It looks like this change was introduced in 1.42, when #4817 landed. Which made Dial more correct by using net/url's parse URL. Before that change was introduced register would preserve the casing so the first example worked, but the second one didn't.

@dfawley
Copy link
Member

dfawley commented Feb 10, 2023

Thanks for the bug. I'll send a fix in a moment.

dfawley added a commit to dfawley/grpc-go that referenced this issue Feb 10, 2023
Also recommend Scheme() to return no uppercase letters, as RFC3986 requires
parsing to convert the scheme to lowercase

Fixes grpc#6011.
@dfawley
Copy link
Member

dfawley commented Feb 14, 2023

Actually, since the behavior has been this way for over a year, and since Java and C++ both behave this way, I'm just going to simply update the documentation to mention that the case is collapsed to lower-case and all resolver Scheme()s should return only lowercase characters instead.

@dfawley dfawley self-assigned this Feb 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants