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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from Aug 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dde7635
Add peer infomation to http2server context.
feihu-stripe Aug 16, 2022
4a55ac5
small update
feihu-stripe Aug 17, 2022
33e6de3
Revert "small update"
feihu-stripe Aug 17, 2022
a2e0810
pr
feihu-stripe Aug 22, 2022
23e151d
handle feedbacks
feihu-stripe Aug 23, 2022
0949681
handle feedbacks
feihu-stripe Aug 23, 2022
661205c
handle feedbacks
feihu-stripe Aug 23, 2022
1e0e6d0
pr
feihu-stripe Aug 23, 2022
1bb5d72
pr
feihu-stripe Aug 24, 2022
ee6c28a
[empty] rerun ci
feihu-stripe Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 15 additions & 8 deletions internal/transport/http2_server.go
View file
Open in desktop
Expand Up @@ -265,6 +265,9 @@ func NewServerTransport(conn net.Conn, config *ServerConfig) (_ ServerTransport,
czData: new(channelzData),
bufferPool: newBufferPool(),
}
// Add peer information to the http2server context.
t.ctx = peer.NewContext(t.ctx, t.getPeer())

t.controlBuf = newControlBuffer(t.done)
if dynamicWindow {
t.bdpEst = &bdpEstimator{
Expand Down Expand Up @@ -485,14 +488,7 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func(
} else {
s.ctx, s.cancel = context.WithCancel(t.ctx)
}
pr := &peer.Peer{
Addr: t.remoteAddr,
}
// Attach Auth info if there is any.
if t.authInfo != nil {
pr.AuthInfo = t.authInfo
}
s.ctx = peer.NewContext(s.ctx, pr)

// Attach the received metadata to the context.
if len(mdata) > 0 {
s.ctx = metadata.NewIncomingContext(s.ctx, mdata)
Expand Down Expand Up @@ -1416,6 +1412,17 @@ func (t *http2Server) getOutFlowWindow() int64 {
}
}

func (t *http2Server) getPeer() *peer.Peer {
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.
	}

}

func getJitter(v time.Duration) time.Duration {
if v == infinity {
return 0
Expand Down
19 changes: 19 additions & 0 deletions internal/transport/transport_test.go
View file
Open in desktop
Expand Up @@ -35,6 +35,8 @@ import (
"testing"
"time"

"google.golang.org/grpc/peer"

"github.com/google/go-cmp/cmp"
"golang.org/x/net/http2"
"golang.org/x/net/http2/hpack"
Expand Down Expand Up @@ -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) { ... }

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.

defer server.stop()

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!

}
_, 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.

t.Fatalf("peer expected in server context, but actually not found.")
}
}
}