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

feihu-stripe · 2022-08-16T23:44:57Z

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 · 2022-08-16T23:45:00Z

The committers listed above are authorized under a signed CLA.

✅ login: feihu-stripe (dde7635)

feihu-stripe · 2022-08-17T01:59:48Z

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

How can I rerun the build?

This reverts commit 4a55ac5.

feihu-stripe · 2022-08-17T16:39:32Z

@easwars

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

easwars · 2022-08-17T17:13:07Z

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.

dfawley · 2022-08-17T17:38:11Z

internal/transport/http2_server.go

-	s.ctx = peer.NewContext(s.ctx, pr)
+
+	// Add peer information to the stream context.
+	s.ctx = peer.NewContext(s.ctx, t.getPeer())


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

feihu-stripe · 2022-08-17T18:10:32Z

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 · 2022-08-22T05:14:06Z

Updated:

Stopped attaching Peer to stream context. @dfawley
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.

easwars · 2022-08-17T17:11:31Z

internal/transport/http2_server.go

+	pr := &peer.Peer{
+		Addr: t.remoteAddr,
+	}
+	// Attach Auth info if there is any.
+	if t.authInfo != nil {
+		pr.AuthInfo = t.authInfo
+	}
+	return pr


This can probably be simplified as:

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

easwars · 2022-08-22T22:29:43Z

internal/transport/transport_test.go

@@ -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) {


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) { ... }

easwars · 2022-08-22T22:32:25Z

internal/transport/transport_test.go

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


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.

easwars · 2022-08-22T22:33:59Z

internal/transport/transport_test.go

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


Would be nice to include the actual type as well:

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

easwars · 2022-08-22T22:34:31Z

internal/transport/transport_test.go

+		_, ok = peer.FromContext(sc.ctx)
+		if !ok {


These two lines can be combined into one.

feihu-stripe · 2022-08-22T23:06:20Z

@easwars really appreciated for your review.

Will update and respond individually.

dfawley · 2022-08-22T23:36:11Z

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 · 2022-08-23T21:21:47Z

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

feihu-stripe · 2022-08-23T21:35:20Z

r? @easwars @dfawley

easwars · 2022-08-23T21:56:01Z

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++

easwars · 2022-08-23T22:01:18Z

internal/transport/transport_test.go

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


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 · 2022-08-24T00:03:39Z

Contributor

apologize, missed that error.

feihu-stripe · 2022-08-24T00:52:59Z

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

dfawley · 2022-08-24T16:43:44Z

Thank you for the PR!

Add peer infomation to http2server context.

dde7635

feihu-stripe added 2 commits August 17, 2022 08:42

small update

4a55ac5

Revert "small update"

33e6de3

This reverts commit 4a55ac5.

easwars added the Type: Feature New features or improvements in behavior label Aug 17, 2022

easwars added this to the 1.50 Release milestone Aug 17, 2022

dfawley reviewed Aug 17, 2022

View reviewed changes

pr

a2e0810

easwars reviewed Aug 22, 2022

View reviewed changes

easwars assigned feihu-stripe Aug 22, 2022

zasweq added the Status: Requires Reporter Clarification label Aug 23, 2022

handle feedbacks

23e151d

feihu-stripe added 3 commits August 23, 2022 14:22

handle feedbacks

0949681

handle feedbacks

661205c

pr

1e0e6d0

feihu-stripe requested review from easwars and dfawley and removed request for easwars and dfawley August 23, 2022 21:33

feihu-stripe removed their assignment Aug 23, 2022

feihu-stripe requested review from dfawley and removed request for easwars August 23, 2022 21:34

easwars removed the Status: Requires Reporter Clarification label Aug 23, 2022

easwars reviewed Aug 23, 2022

View reviewed changes

pr

1bb5d72

feihu-stripe changed the title ~~Add peer information to http2server context.~~ Add peer information to http2server and http2Client context. Aug 24, 2022

[empty] rerun ci

ee6c28a

easwars approved these changes Aug 24, 2022

View reviewed changes

easwars assigned dfawley Aug 24, 2022

dfawley approved these changes Aug 24, 2022

View reviewed changes

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 merged commit 641dc87 into grpc:master Aug 24, 2022

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

moul-bot mentioned this pull request Oct 17, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.1 moul/depviz#586

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

feihu-stripe commented Aug 16, 2022 •

edited by dfawley

Loading

linux-foundation-easycla bot commented Aug 16, 2022 •

edited

Loading

feihu-stripe commented Aug 17, 2022

feihu-stripe commented Aug 17, 2022

easwars commented Aug 17, 2022

dfawley Aug 17, 2022

feihu-stripe commented Aug 17, 2022

feihu-stripe commented Aug 22, 2022

easwars Aug 17, 2022

easwars Aug 22, 2022

easwars Aug 22, 2022

easwars Aug 22, 2022

feihu-stripe Aug 23, 2022

easwars Aug 22, 2022

feihu-stripe commented Aug 22, 2022

dfawley commented Aug 22, 2022

feihu-stripe commented Aug 23, 2022

feihu-stripe commented Aug 23, 2022

easwars commented Aug 23, 2022

easwars Aug 23, 2022

feihu-stripe commented Aug 24, 2022

feihu-stripe commented Aug 24, 2022

dfawley commented Aug 24, 2022

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

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

Conversation

feihu-stripe commented Aug 16, 2022 • edited by dfawley Loading

linux-foundation-easycla bot commented Aug 16, 2022 • edited Loading

feihu-stripe commented Aug 17, 2022

feihu-stripe commented Aug 17, 2022

easwars commented Aug 17, 2022

Choose a reason for hiding this comment

feihu-stripe commented Aug 17, 2022

feihu-stripe commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feihu-stripe commented Aug 22, 2022

dfawley commented Aug 22, 2022

feihu-stripe commented Aug 23, 2022

feihu-stripe commented Aug 23, 2022

easwars commented Aug 23, 2022

Choose a reason for hiding this comment

feihu-stripe commented Aug 24, 2022

feihu-stripe commented Aug 24, 2022

dfawley commented Aug 24, 2022

feihu-stripe commented Aug 16, 2022 •

edited by dfawley

Loading

linux-foundation-easycla bot commented Aug 16, 2022 •

edited

Loading