[TEX-537] Optional support for unframing grpc-web+json requests.
#501
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timdawborn
force-pushed
the
TEX-537-optional-rewriting-of-grpc-web-json-requests-to-unframed-json
branch
from
efa431c
to
6faf81e
Compare
timdawborn
requested review from
a team,
flavray and
njday
and removed request for
a team
timdawborn
commented
Oct 4, 2023
Comment on lines
+44
to
+65
| test("response: simple text", async () => { | ||
| const response = await axios.get(`${PROXAY_HOST}${SIMPLE_TEXT_PATH}`); | ||
| expect(response.data).toBe(SIMPLE_TEXT_RESPONSE); | ||
| }); | ||
|
|
||
| test("response: utf-8", async () => { | ||
| const response = await axios.get(`${PROXAY_HOST}${UTF8_PATH}`); | ||
| expect(response.data).toBe(UTF8_RESPONSE); | ||
| }); | ||
|
|
||
| test("response: binary", async () => { | ||
| const response = await axios.get(`${PROXAY_HOST}${BINARY_PATH}`, { | ||
| responseType: "arraybuffer", | ||
| }); | ||
| expect(response.data).toEqual(BINARY_RESPONSE); | ||
| }); | ||
|
|
||
| test("can pick any tape", async () => { | ||
| await axios.post(`${PROXAY_HOST}/__proxay/tape`, { | ||
| tape: "new-tape", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These are all copies of the above tests, but with the proxay server running with unframing enabled. I duplicated these to ensure they weren't affected by running with unframing enabled.
timdawborn
commented
Oct 4, 2023
Comment on lines
+67
to
+120
| test("unframes a grpc-web+json request", async () => { | ||
| const requestBody = Buffer.from([ | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 31, | ||
| 123, | ||
| 34, | ||
| 101, | ||
| 109, | ||
| 97, | ||
| 105, | ||
| 108, | ||
| 34, | ||
| 58, | ||
| 34, | ||
| 102, | ||
| 111, | ||
| 111, | ||
| 46, | ||
| 98, | ||
| 97, | ||
| 114, | ||
| 64, | ||
| 101, | ||
| 120, | ||
| 97, | ||
| 109, | ||
| 112, | ||
| 108, | ||
| 101, | ||
| 46, | ||
| 99, | ||
| 111, | ||
| 109, | ||
| 34, | ||
| 125, | ||
| ]); | ||
| const response = await axios.post( | ||
| `${PROXAY_HOST}${GRPC_WEB_JSON_PATH}`, | ||
| requestBody, | ||
| { | ||
| headers: { | ||
| "content-type": "application/grpc-web+json", | ||
| host: `localhost:${TEST_SERVER_PORT}`, | ||
| }, | ||
| } | ||
| ); | ||
| expect(response.headers["content-type"]).toBe( | ||
| "application/json; charset=utf-8" | ||
| ); | ||
| expect(response.data).toEqual({ email: "foo.bar@example.com" }); | ||
| }); |
There was a problem hiding this comment.
This is a new test for unframing.
timdawborn
commented
Oct 4, 2023
Comment on lines
-77
to
-84
| if ( | ||
| this.preventConditionalRequests && | ||
| (req.method === "GET" || req.method === "HEAD") | ||
| ) { | ||
| // Headers are always coming in as lowercase. | ||
| delete req.headers["if-modified-since"]; | ||
| delete req.headers["if-none-match"]; | ||
| } |
There was a problem hiding this comment.
Move this logic into the new rewriteRequest helper method that includes other potential request modifications.
timdawborn
commented
Oct 4, 2023
Comment on lines
+252
to
+260
| // Potentially prevent 304 responses from being able to be generated. | ||
| if ( | ||
| this.preventConditionalRequests && | ||
| (request.method === "GET" || request.method === "HEAD") | ||
| ) { | ||
| // Headers are always coming in as lowercase. | ||
| delete request.headers["if-modified-since"]; | ||
| delete request.headers["if-none-match"]; | ||
| } |
There was a problem hiding this comment.
This is moved logic.
timdawborn
commented
Oct 4, 2023
Comment on lines
+262
to
+270
| // Potentially unframe a grpc-web+json request. | ||
| if ( | ||
| request.method === "POST" && | ||
| request.headers["content-type"] === "application/grpc-web+json" && | ||
| hostname != null && | ||
| this.unframeGrpcWebJsonRequestsHostnames.includes(hostname) | ||
| ) { | ||
| this.rewriteGrpcWebJsonRequest(request); | ||
| } |
There was a problem hiding this comment.
This is new logic.
njday
reviewed
Oct 4, 2023
| @@ -78,6 +78,10 @@ async function main(argv: string[]) { | |||
| "--https-ca <filename.pem>", | |||
| "Enable HTTPS server where the certificate was generated by this CA. Useful if you are using a self-signed certificate. Also requires --https-key and --https-cert." | |||
| ) | |||
| .option( | |||
| "--unframe-grpc-web-json-requests-hostname [hostname...]", | |||
| "Rewrite received requests whose content-type is application/grpc-web+json to be application/json, mutating the body of the request accordingly. This is useful if you want plain text tape records rather than binary data. The gRPC server needs to support receiving unframed requests for this option to be useful." | |||
There was a problem hiding this comment.
Nit: We could also include the details about hostname matching in the description here
timdawborn
deleted the
TEX-537-optional-rewriting-of-grpc-web-json-requests-to-unframed-json
branch
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new option for unframing
application/grpc-web+jsonrequests to be justapplication/jsonrequests if the request is to a whitelisted hostname. This unframing is performed by mutating the received request before doing anything else with it, including passing it upstream or searching for existing tapes. This has the effect that, for all intents and purposes, the request is anapplication/jsonrequest for the purposes of persistence, similarity scoring, and proxying.