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/server: add :method POST to incoming metadata #4770

Merged
merged 4 commits into from Sep 15, 2021
Merged

transport/server: add :method POST to incoming metadata #4770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d200c66
Added :method POST to Server Transport
zasweq Sep 10, 2021
14be6c4
Responded to Menghan's comments
zasweq Sep 15, 2021
2c5b58a
Responded to Menghan's comment
zasweq Sep 15, 2021
4dede1b
Moved break statement
zasweq Sep 15, 2021
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
16 changes: 16 additions & 0 deletions binarylog/binarylog_end2end_test.go
View file
Open in desktop
Expand Up @@ -850,11 +850,27 @@ func equalLogEntry(entries ...*pb.GrpcLogEntry) (equal bool) {
tmp := append(h.Metadata.Entry[:0], h.Metadata.Entry...)
h.Metadata.Entry = tmp
sort.Slice(h.Metadata.Entry, func(i, j int) bool { return h.Metadata.Entry[i].Key < h.Metadata.Entry[j].Key })
// Delete headers that have POST values here since we cannot control
// this.
for i, entry := range h.Metadata.Entry {
if entry.Key == ":method" {
h.Metadata.Entry = append(h.Metadata.Entry[:i], h.Metadata.Entry[i+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong if there are more than one :method entry in the list. (It won't happen in practice though).

Either make it break after in the if.
Or i++ only the key doesn't match :method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm I see what you're saying, would delete from list as you're iterating? Broke after deleting :method.

break
}
}
}
if h := e.GetServerHeader(); h != nil {
tmp := append(h.Metadata.Entry[:0], h.Metadata.Entry...)
h.Metadata.Entry = tmp
sort.Slice(h.Metadata.Entry, func(i, j int) bool { return h.Metadata.Entry[i].Key < h.Metadata.Entry[j].Key })
// Delete headers that have POST values here since we cannot control
// this.
for i, entry := range h.Metadata.Entry {
if entry.Key == ":method" {
h.Metadata.Entry = append(h.Metadata.Entry[:i], h.Metadata.Entry[i+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

And break here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops sorry, done.

break
}
}
}
if h := e.GetTrailer(); h != nil {
sort.Slice(h.Metadata.Entry, func(i, j int) bool { return h.Metadata.Entry[i].Key < h.Metadata.Entry[j].Key })
Expand Down
1 change: 1 addition & 0 deletions internal/transport/http2_server.go
View file
Open in desktop
Expand Up @@ -380,6 +380,7 @@ func (t *http2Server) operateHeaders(frame *http2.MetaHeadersFrame, handle func(
s.recvCompress = hf.Value
case ":method":
httpMethod = hf.Value
mdata[":method"] = append(mdata[":method"], hf.Value)
Copy link
Member

Choose a reason for hiding this comment

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

If we are adding this, I believe we also need to add validation that it is POST.

I thought the plan was to synthesize this to be POST?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have a validation that it is POST. This was actually my first PR merged: #4241.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See RBAC Filter PR for me and Menghan's back and forth on this topic.

Copy link
Member

@dfawley dfawley Sep 15, 2021 •
edited

Choose a reason for hiding this comment

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

Whoops. I had forgotten that. So you might want to change this in the PR description, then, as it's misleading:

This PR adds a hardcoded :method POST to Server's metadata

See RBAC Filter PR for me and Menghan's back and forth on this topic.

tl;dr so I don't have to go hunting for the right thread? This seems pretty straightforward to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, Eric wants a :method POST in the metadata, because RBAC needs to know this for matching logic. However, the codebase had no previous append to the metadata adding :method POST. The only thing we had was it client side, and reading the value and making sure it was POST, but not in the metadata structure itself. Thus, I decided to put the appending of :method POST in the interceptor, but Menghan mentioned that was hacky and it would be better if I did it in transport, as that is more general solution. Since this broke some tests, I decided to do this in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. SGTM!

case ":path":
s.method = hf.Value
case "grpc-timeout":
Expand Down
37 changes: 37 additions & 0 deletions test/end2end_test.go
View file
Open in desktop
Expand Up @@ -7819,3 +7819,40 @@ func (s) TestStreamingServerInterceptorGetsConnection(t *testing.T) {
t.Fatalf("ss.Client.StreamingInputCall(_) = _, %v, want _, %v", err, io.EOF)
}
}

func unaryInterceptorVerifyPost(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
md, ok := metadata.FromIncomingContext(ctx)
if !ok {
return nil, status.Error(codes.NotFound, "metadata was not in context")
}
method := md.Get(":method")
if len(method) != 1 {
return nil, status.Error(codes.InvalidArgument, ":method value had more than one value")
}
if method[0] != "POST" {
return nil, status.Error(codes.InvalidArgument, ":method value was not post")
}
return handler(ctx, req)
}

// TestUnaryInterceptorGetsPost verifies that the server transport adds a
// :method POST header to metadata, and that that added Header is visibile at
// the grpc layer.
func (s) TestUnaryInterceptorGetsPost(t *testing.T) {
ss := &stubserver.StubServer{
EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) {
return &testpb.Empty{}, nil
},
}
if err := ss.Start([]grpc.ServerOption{grpc.UnaryInterceptor(unaryInterceptorVerifyPost)}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need an interceptor. Just read and check the metadata in StubServer.EmptyCall.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, actually, it is OK. The purpose is to test interceptors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because it's in the context of RBAC. Agree with you otherwise though.

t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()

if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); status.Code(err) != codes.OK {
t.Fatalf("ss.Client.EmptyCall(_, _) = _, %v, want _, error code %s", err, codes.OK)
}
}