grpc: Add a pointer of server to ctx passed into stats handler #6625
Conversation
zasweq
added
Type: Feature
| @@ -1920,6 +1960,24 @@ func (s *Server) getCodec(contentSubtype string) baseCodec { | |||
| return codec | |||
| } | |||
|
|
|||
| // isRegisteredMethod returns whether the passed in method is registered as a | |||
| // method on the server. | |||
| func (s *Server) isRegisteredMethod(method string) bool { | |||
There was a problem hiding this comment.
As discussed offline this needs to be a "full method name", parsed accordingly, with both service & method included in the check.
| // getServer gets the Server from the context. | ||
| func getServer(ctx context.Context) *Server { |
| } | ||
|
|
||
| // setServer sets the Server in the context. | ||
| func setServer(ctx context.Context, server *Server) context.Context { |
| if registeredMethod := internal.IsRegisteredMethod.(func(*grpc.Server, string) bool)(server, "UnaryCall"); !registeredMethod { | ||
| shsa.errorCh.Send(errors.New("UnaryCall should be a registered method according to server")) | ||
| return ctx | ||
| } |
There was a problem hiding this comment.
A little more readable:
isRegistered := internal.IsRegisteredMethod.(func..etc..)
if !isRegistered(server, "UnaryCall) {
// fail
}
// etcThere was a problem hiding this comment.
Ah, great point. Switched.
| @@ -6539,3 +6539,89 @@ func (s) TestRPCBlockingOnPickerStatsCall(t *testing.T) { | |||
| t.Fatalf("sh.pickerUpdated count: %v, want: %v", pickerUpdatedCount, 2) | |||
| } | |||
| } | |||
|
|
|||
| type statsHandlerServerAssert struct { | |||
There was a problem hiding this comment.
Should we make a stub stats handler that works like our server, so the implementation relevant to the test can live inside the test itself?
There was a problem hiding this comment.
Sure. I'm gonna switch this test to use this new util. However, I won't switch all the tests that I wrote that use stats handler components to use this, but if you want me to I can do that (probably as another PR). Any future tests I will write I'll use this stub type.
| // Within the stats handler, asking the server whether unary or duplex method | ||
| // names are registered should return true, and any other query should return | ||
| // false. | ||
| func (s) TestStatsHandlerCallsServerIsRegisteredMethod(t *testing.T) { |
There was a problem hiding this comment.
Can you add this to a stats_test.go, or put it into the stats directory, or even in the root directory since it's actually testing grpc.Server functionality? This file is approaching 7k lines which is crazy.
There was a problem hiding this comment.
Chose to put it in stats directory, since that is where we want this helper to be called.
|
Got to all your comments and made a new pr containing all your comments: #6724. Thanks for the pass. |
This PR adds a pointer to the grpc.Server into the context passed into the stats handler. It also adds an internal only helper on the server that returns whether a given method is registered or not. This will be used in OpenTelemetry instrumentation for security purposes with respect to the cardinality of the method attribute. Contains and is based on (and technically requires) #6624. The next step after these two PRs is to move the rest of the stats handler callouts to the gRPC layer, get registered methods plumbed client side, canoncalize Target() without breaking it and then the OpenTelemetry instrumentation itself :).
RELEASE NOTES: N/A