-
Notifications
You must be signed in to change notification settings - Fork 111
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
Add support for dynamic gRPC tables #1530
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1530 +/- ##
==========================================
- Coverage 72.19% 64.39% -7.80%
==========================================
Files 194 194
Lines 19239 19412 +173
==========================================
- Hits 13889 12501 -1388
- Misses 4679 6116 +1437
- Partials 671 795 +124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pkg/internal/ebpf/bhpack/hpack.go
Outdated
if !ok { | ||
// If we've failed once to find an index, don't allow us to find | ||
// a value for index that's greater than the last successful one | ||
if !ok || (d.failedToIndex && idx > d.lastGoodIndex) { |
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.
did you mean !d.failedToIndex
here?
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.
it should be d.failedToIndex, but I caught a bug with that. I have a new version which changes how this is done now. All I block is adding new entries to the dynamic table. I'll push an update soon once I have integration tests
Our current kprobes gRPC support lacked hpack dynamic tables support. I had postponed the implementation of this, but it essentially prevents us from finding the path of the request if a connection is reused.
Essentially, the gRPC implementation will internally use one hpack Decoder/Encoder instance per connection, which is what we do in http2grpc_transform.go. Once a key value pair of the gRPC headers is sent, the client will create an index internally to represent the key value pair in the future, and it will only send the index if the key value pair is repeated. The server will also create an index on the decoder side, the first time a key value pair is received. Since the communication is one at a time, even though there are multiple streams, the two tables will always be in sync.
This poses a problem for our decoding logic for the
:path
field. Namely the:path
will be paired with the RPC function to be called, e.g.[:path]:[/someGRPCMethod]
. Once this pair is sent, the combination will be encoded in a dynamic table, with an index, for example 234. Next time around a request to /someGRPCMethod is made, the client will send 234 and the server will look up its own index 234 to decipher that this means[:path]:[/someGRPCMethod]
.Until now we simply resolved to
*
(asterisk) when we couldn't find the index.With this change we start tracking the dynamic table indices. We already used per connection decoders, so it was a matter of detecting when we can start tracking the indices and to store them in the LRU table in our userspace code.
Essentially the logic boils down to this:
PRI * HTTP/2.0...
we know that this is a new connection established. Therefore we can start tracking the indices in the same way the client and the server do. We only need one hpack per connection, since both the client and the server store the same information.TODO: