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.

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

[TEX-537] Add support for gRPC-web request body inspection #548

Merged
merged 36 commits into from
Jan 4, 2024

Conversation

timdawborn
Copy link
Contributor

@timdawborn timdawborn commented Nov 24, 2023

Adds support to proxay to perform gRPC-web request body inspection when computing request similarity. This is done by inspecting the content type of the request. If it's application/grpc-web or application/grpc-web-text, we invoke new body-to-object decoding logic, like we do for application/json requests. If this body-to-object decoding logic returns an object, the normal object similarity scoring happens, which is what we're after all along.

@timdawborn timdawborn force-pushed the TEX-537-grpc-web-request-body-inspection branch from 4018c8b to 22d1717 Compare November 24, 2023 03:55
@timdawborn timdawborn changed the base branch from master to TEX-537-refactor-compression December 8, 2023 05:40
Base automatically changed from TEX-537-refactor-compression to master December 10, 2023 22:01
@@ -57,7 +57,8 @@
"deep-diff": "^1.0.2",
"fs-extra": "^11.2.0",
"js-yaml": "^4.1.0",
"string-similarity": "^4.0.4"
"string-similarity": "^4.0.4",
"unicode-properties": "^1.4.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds a new library to get basic unicode properties for Unicode code points. As far as I could tell, NodeJS does not have anything like this builtin (equivalent of the Python unicodedata package).

Comment on lines +57 to +60
case "json":
return convertJsonMessageToObject(message);
case "proto":
return convertProtoMessageToObject(message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gRPC-Web payloads can have various sub-content types. We handle json and proto here. All others are unhandled, and will result in the default binary comparison logic being used for the request payloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes in this file are best viewed with the diff ignoring whitespace. A bunch of indentation changes makes the diff with whitespace very hard to read compared to what the changes actually are.

@timdawborn timdawborn marked this pull request as ready for review January 4, 2024 05:35
@timdawborn timdawborn requested a review from a team as a code owner January 4, 2024 05:35
Comment on lines +101 to +116
} else if (contentType.startsWith("application/grpc-web-text")) {
return countBodyDifferencesGrpcWebText(
request1,
contentType1,
request2,
contentType2,
rewriteBeforeDiffRules,
);
} else if (contentType.startsWith("application/grpc-web")) {
return countBodyDifferencesGrpcWeb(
request1,
contentType1,
request2,
contentType2,
rewriteBeforeDiffRules,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds special similarity scoring logic for gRPC-Web request types, like we do for other content types like JSON.

Copy link

@flavray flavray left a comment

Choose a reason for hiding this comment

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

Really neat. Code for the protobuf spec LGTM (caveat: I didn't read the official spec), tests LGTM, and similarity logic LGTM. Will be great to see it in action. 🙂

The protobuf decoding part might even be worth being its own library some day 😄 It looks more powerful than https://github.com/konsumer/rawproto which seems to be the only alternative for now.

Copy link

@njday njday left a comment

Choose a reason for hiding this comment

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

Nice one 👍 LGTM

Really interesting to read this - iv looked over the specs before but its cool to understand it at a lower level.

@timdawborn timdawborn merged commit 1522f47 into master Jan 4, 2024
6 checks passed
@timdawborn timdawborn deleted the TEX-537-grpc-web-request-body-inspection branch January 4, 2024 22:51
timdawborn added a commit that referenced this pull request Jan 5, 2024
Removes the functionality to unframe gRPC-web+json requests (added in
#501 and #503) in favour of the proper solution to handle grpc-web
requests in full (added in #548).

This functionality didn't work quite as expected anyway, so it's good to
clean it up in favour of the the full, proper working solution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants