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

transport: add peer information to http2Server and http2Client context #5589

Merged
merged 10 commits into from Aug 24, 2022

Conversation

feihu-stripe
Copy link
Contributor

@feihu-stripe feihu-stripe commented Aug 16, 2022

Motivation: #5586

Added peer to the http2server context to allow application to access this information when a connection is just established. This frees application from tracking this information on each and every rpc request.

RELEASE NOTES:

  • stats: provide peer information in HandleConn context

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 16, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@feihu-stripe
Copy link
Contributor Author

Looks like the failed test case is a known flake test: Test/DetailedGoawayErrorOnGracefulClosePropagatesToRPCError

How can I rerun the build?

@feihu-stripe
Copy link
Contributor Author

@easwars

All tests have passed, but LABEL and MILESTONE I don't have permission to add. Thanks

@easwars easwars added this to the 1.50 Release milestone Aug 17, 2022
@easwars
Copy link
Contributor

easwars commented Aug 17, 2022

Thanks for the PR.

Did you intend to do this only for the server, and not for the client? If so, why?

And could you please add tests for this.

s.ctx = peer.NewContext(s.ctx, pr)

// Add peer information to the stream context.
s.ctx = peer.NewContext(s.ctx, t.getPeer())
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is derived from the transport's context (immediately above), we shouldn't need to re-attach the peer here.

@feihu-stripe
Copy link
Contributor Author

Thanks for the PR.

Did you intend to do this only for the server, and not for the client? If so, why?

And could you please add tests for this.

Will update PR accordingly

@feihu-stripe
Copy link
Contributor Author

Updated:

  1. Stopped attaching Peer to stream context. @dfawley
  2. Added a test to validate Peer is set in server context @easwars

Not feeling confident to make the same change for client due to my limited understand of client side code, also unlike server context which is initiated from scratch, an external context is passed into client constructor, so I am concerned about isolation of that chagne.

Comment on lines 1419 to 1426
pr := &peer.Peer{
Addr: t.remoteAddr,
}
// Attach Auth info if there is any.
if t.authInfo != nil {
pr.AuthInfo = t.authInfo
}
return pr
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be simplified as:

	return &peer.Peer{
		Addr: t.remoteAddr,
		AuthInfo: t.authInfo, // Can be nil.
	}

@@ -2450,3 +2452,20 @@ func TestConnectionError_Unwrap(t *testing.T) {
t.Error("ConnectionError does not unwrap")
}
}

// Verify Peer is set in server context.
func TestPeerSetInServerContext(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please specify our test struct as the receiver for this test. This will ensure that our leakcheck runs at the end of the test.

func (s) TestPeerSetInServerContext(t *testing.T) { ... }


// Verify Peer is set in server context.
func TestPeerSetInServerContext(t *testing.T) {
server := setUpServerOnly(t, 0, &ServerConfig{}, suspended)
Copy link
Contributor

Choose a reason for hiding this comment

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

setUpServerOnly does not setup any transports. So, server.conns in your for loop will be nil, and therefore this test will not be doing anything.

You either need to use setUpWithOptions or you need to explicitly do a net.Dial() to create a transport. Please see some other tests in the same file to get a better idea. Thanks.

for k := range server.conns {
sc, ok := k.(*http2Server)
if !ok {
t.Fatalf("Failed to convert %v to *http2Server", k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to include the actual type as well:

  t.Fatalf("ServerTransport is of type %T, want %T, k, &http2Server{})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome!

Comment on lines 2466 to 2467
_, ok = peer.FromContext(sc.ctx)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines can be combined into one.

@feihu-stripe
Copy link
Contributor Author

@easwars really appreciated for your review.

Will update and respond individually.

@dfawley
Copy link
Contributor

dfawley commented Aug 22, 2022

an external context is passed into client constructor, so I am concerned about isolation of that chagne.

The main difference is: on the server, the stream's context is a child of the transport's context, whereas in the client, the stream's context is what the user provides for that RPC instead. It should be fine to attach the peer information on the transport's context as well as the stream's context, but note that we would have to do it in both places on the client.

@feihu-stripe
Copy link
Contributor Author

  1. set Peer in http2client context
  2. in unit test, verify Peer is set in server, client and stream context.

@feihu-stripe feihu-stripe requested review from easwars and dfawley and removed request for easwars and dfawley August 23, 2022 21:33
@feihu-stripe feihu-stripe removed their assignment Aug 23, 2022
@feihu-stripe feihu-stripe requested review from dfawley and removed request for easwars August 23, 2022 21:34
@feihu-stripe
Copy link
Contributor Author

r? @easwars @dfawley

@easwars
Copy link
Contributor

easwars commented Aug 23, 2022

r? @easwars @dfawley

Could you please take care of the failing tests. Specifically, it is complaining about the following:

Error: internal/transport/transport_test.go:2488:4: should replace count += 1 with count++


// verify peer is set in server transport context.
count := 0
for {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the waitWhileTrue helper method to simplify the waiting logic here. See : https://github.com/grpc/grpc-go/blob/master/internal/transport/transport_test.go#L1616

@feihu-stripe
Copy link
Contributor Author

Contributor

apologize, missed that error.

@feihu-stripe feihu-stripe changed the title Add peer information to http2server context. Add peer information to http2server and http2Client context. Aug 24, 2022
@feihu-stripe
Copy link
Contributor Author

Is there a recommended way to rerun the CI build? I believe these are unrelated errors.

@dfawley
Copy link
Contributor

dfawley commented Aug 24, 2022

Thank you for the PR!

@dfawley dfawley changed the title Add peer information to http2server and http2Client context. transport: add peer information to http2Server and http2Client context Aug 24, 2022
@dfawley dfawley merged commit 641dc87 into grpc:master Aug 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants