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

client: use "localhost:port" as authority if target is ":port" #4017

Merged
merged 8 commits into from Nov 12, 2020
Merged

client: use "localhost:port" as authority if target is ":port" #4017

merged 8 commits into from Nov 12, 2020

Conversation

@GarrettGutierrez1
Copy link
Contributor

@GarrettGutierrez1 GarrettGutierrez1 commented Nov 5, 2020

Fixes #3983 .

@GarrettGutierrez1 GarrettGutierrez1 added this to the 1.34 Release milestone Nov 5, 2020
@dfawley dfawley self-assigned this Nov 5, 2020
Copy link
Contributor

@dfawley dfawley left a comment

Looks good, but please add a test for this in test/ (verify the :authority header metadata in a simple server).

@dfawley dfawley assigned GarrettGutierrez1 and unassigned dfawley Nov 6, 2020
@GarrettGutierrez1
Copy link
Contributor Author

@GarrettGutierrez1 GarrettGutierrez1 commented Nov 11, 2020

@dfawley Added the test in authority_test.go.

test/authority_test.go Outdated Show resolved Hide resolved
test/authority_test.go Outdated Show resolved Hide resolved
test/authority_test.go Outdated Show resolved Hide resolved
@@ -33,44 +34,48 @@ import (
testpb "google.golang.org/grpc/test/grpc_testing"
)

func authorityChecker(expectedAuthority *string) func(context.Context, *testpb.Empty) (*testpb.Empty, error) {
Copy link
Contributor

@dfawley dfawley Nov 12, 2020

I'm not sure about passing a pointer here without synchronization. It may fail the race test.

I'd do this instead:

func authorityChecker(ctx context.Context, expectedAuthority string) (*testpb.Empty, error) {
  // Same body as closure
}

...

ss := &stubServer{
	emptyCall: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) {
		// Simple case:
		return authorityChecker(ctx, wantAuthority)

		// Complex case:
		wantAuthorityMu.Lock()
		defer wantAuthorityMu.Unlock()
		return authorityChecker(ctx, wantAuthority)
	}
}

Or use a channel to pass the expected authority to the handler, but that's probably more trouble than it's worth.

Copy link
Contributor Author

@GarrettGutierrez1 GarrettGutierrez1 Nov 12, 2020

Made this change.

}
if err := ss.Start(nil); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
return
Copy link
Contributor

@dfawley dfawley Nov 12, 2020

return-after-fatal is unncessary.

Copy link
Contributor Author

@GarrettGutierrez1 GarrettGutierrez1 Nov 12, 2020

Removed return

s := strings.Split(ss.address, ":")
if len(s) != 2 {
t.Fatalf("No port in address: %v", ss.address)
return
}
Copy link
Contributor

@dfawley dfawley Nov 12, 2020

Use net.ParseIP instead.

Copy link
Contributor Author

@GarrettGutierrez1 GarrettGutierrez1 Nov 12, 2020

Using net package for this now.

Copy link
Contributor

@dfawley dfawley left a comment

Looks good, just one minor nit.

expectedAuthority = "localhost:" + s[1]
authorityMu.Lock()
expectedAuthority = "localhost:" + port
authorityMu.Unlock()
// ss.Start dials, but not the ":[port]" target that is being tested here.
// Dial again, with ":[port]" as the target.
cc, err := grpc.Dial(":"+s[1], grpc.WithInsecure())
Copy link
Contributor

@dfawley dfawley Nov 12, 2020

s/s[1]/port/

@dfawley dfawley changed the title fix: invalid authority header when target set to ":[port]" client: use "localhost:port" as authority if target is ":port" Nov 12, 2020
@GarrettGutierrez1 GarrettGutierrez1 merged commit 90f1b3e into grpc:master Nov 12, 2020
11 checks passed
davidkhala added a commit to Hyperledger-TWGC/grpc that referenced this issue Dec 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants