Conversation
| // double check content length is < max request content length in case transfer encoding is used | ||
| if body_size > config.max_request_content_length { | ||
| return log_and_create_http_response( | ||
| &format!("Error processing traces: Payload too large"), | ||
| StatusCode::PAYLOAD_TOO_LARGE, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
What do you think about adding this check inside the verify_request_content_length method? It would be consistent with the rest of the return statements in that method giving an HttpResponse.
There was a problem hiding this comment.
I would prefer not to, as that would require me to update other functions like StatsProcessor::process_stats, which doesn't generate the request size outside of verify_request_content_length. I’d also need to update functions in several spots where including the request length might not be as important. Please feel free to correct me if I’m mistaken, but I thought that we were mainly concerned about the content length for submitting the traces.
There was a problem hiding this comment.
I see what you're saying. If we wanted to make this check inside verify_request_content_length we would need to change the signature of the method to accept body_size, which affects other calls to it (like from StatsProcessor:process_stats. I think it would be preferable to have all of the content length checks in the method for that (verify_request_content_length) but if we only want to make this check for traces then I think your current approach is the best option.
|
Also do you know why the requests are large enough that the |
This behavior is part of the Golang net/HTTP package specification and is triggered for efficiency and flexibility in streaming large or dynamically generated content. This happens when the total size of the request body is not known in advance, especially when streaming data |
duncanpharvey
left a comment
duncanpharvey
left a comment
There was a problem hiding this comment.
One minor change needed then this is good to go!
| // double check content length is < max request content length in case transfer encoding is used | ||
| if body_size > config.max_request_content_length { | ||
| return log_and_create_http_response( | ||
| &format!("Error processing traces: Payload too large"), | ||
| StatusCode::PAYLOAD_TOO_LARGE, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I see what you're saying. If we wanted to make this check inside verify_request_content_length we would need to change the signature of the method to accept body_size, which affects other calls to it (like from StatsProcessor:process_stats. I think it would be preferable to have all of the content length checks in the method for that (verify_request_content_length) but if we only want to make this check for traces then I think your current approach is the best option.
| Some(res) => res, | ||
| None => { | ||
| if let Some(transfer_encoding_header) = header_map.get(header::TRANSFER_ENCODING) { | ||
| println!( |
There was a problem hiding this comment.
Can this be a debug log?
There was a problem hiding this comment.
We can use the tracing library which already provides a debug log method
2 participants
What does this PR do?
Add support for Transfer-Encoding header for GCP Go Gen 1 Cloud Functions. In golang, if the content length of the request body is too large, the Content-Length is not set, Go’s net/http server package automatically adds the
Transfer-Encoding: chunkedheader. This indicates that the response body is sent in chunks without a documented content length.This PR will check to see if the transfer-encoding header is present, as well as double-check the content length to ensure it is still below the max
Motivation
Support Golang Cloud Functions Gen 1 transfer-encoding header
Additional Notes
more info on the difference can be found here and here
Describe how to test/QA your changes
Tested in this Go cloud function , traces can be found here