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/server: add :method POST to incoming metadata #4770
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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:]...) | ||
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:]...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And break here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
tl;dr so I don't have to go hunting for the right thread? This seems pretty straightforward to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the title. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. SGTM! |
||
case ":path": | ||
s.method = hf.Value | ||
case "grpc-timeout": | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, actually, it is OK. The purpose is to test interceptors. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.