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.

Sign up for GitHub

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

Jump to bottom

fix(tonic): Preserve HTTP method in interceptor #912

Merged
merged 4 commits into from Feb 21, 2022
Merged

fix(tonic): Preserve HTTP method in interceptor #912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d526e1d
tonic: preserve HTTP method in interceptor
jdahlq Feb 12, 2022
b971c54
tonic: fix fmt issue
jdahlq Feb 18, 2022
bd3cf45
Revert "tonic: fix fmt issue"
jdahlq Feb 19, 2022
4954763
add test annotation to actually run test
jdahlq Feb 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion tonic/src/client/grpc.rs
View file
Open in desktop
Expand Up @@ -248,7 +248,12 @@ impl<T> Grpc<T> {
})
.map(BoxBody::new);

let mut request = request.into_http(uri, SanitizeHeaders::Yes);
let mut request = request.into_http(
uri,
http::Method::POST,
http::Version::HTTP_2,
SanitizeHeaders::Yes,
);
Comment on lines +251 to +256
Copy link
Member

Choose a reason for hiding this comment

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

Im not following why these values are not just hard coded intointo_http since they should always be POST and HTTP2 and can be overridden outside (aka for grpc-web).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking the time to review! Firstly, see my response on issue #911 to make sure we are on the same page.

If Rust supported default parameters, then I would propose making POST and HTTP2 defaults for Request::into_http.

But one issue here is that Request::from_http will happily accept HTTP requests of any version and method, silently dropping that information and creating subtle bugs like this one. By adding method and version to Request::into_http, we are making it more explicit that method and version are not preserved.

In any case, Request::into_http only has two callers currently, and it is restricted to the crate, so the stakes aren't very high here. I could leave grpc.rs alone and instead write a new method Request::from_http_with_custom_method if you think that would be cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, thanks for writing that up it all makes sense!


// Add the gRPC related HTTP headers
request
Expand Down
13 changes: 10 additions & 3 deletions tonic/src/request.rs
View file
Open in desktop
Expand Up @@ -169,12 +169,14 @@ impl<T> Request<T> {
pub(crate) fn into_http(
self,
uri: http::Uri,
method: http::Method,
version: http::Version,
sanitize_headers: SanitizeHeaders,
) -> http::Request<T> {
let mut request = http::Request::new(self.message);

*request.version_mut() = http::Version::HTTP_2;
*request.method_mut() = http::Method::POST;
*request.version_mut() = version;
*request.method_mut() = method;
*request.uri_mut() = uri;
*request.headers_mut() = match sanitize_headers {
SanitizeHeaders::Yes => self.metadata.into_sanitized_headers(),
Expand Down Expand Up @@ -441,7 +443,12 @@ mod tests {
.insert(*header, MetadataValue::from_static("invalid"));
}

let http_request = r.into_http(Uri::default(), SanitizeHeaders::Yes);
let http_request = r.into_http(
Uri::default(),
http::Method::POST,
http::Version::HTTP_2,
SanitizeHeaders::Yes,
);
assert!(http_request.headers().is_empty());
}

Expand Down
27 changes: 26 additions & 1 deletion tonic/src/service/interceptor.rs
View file
Open in desktop
Expand Up @@ -157,7 +157,14 @@ where
}

fn call(&mut self, req: http::Request<ReqBody>) -> Self::Future {
// It is bad practice to modify the body (i.e. Message) of the request via an interceptor.
// To avoid exposing the body of the request to the interceptor function, we first remove it
// here, allow the interceptor to modify the metadata and extensions, and then recreate the
// HTTP request with the body. Tonic requests do not preserve the URI, HTTP version, and
// HTTP method of the HTTP request, so we extract them here and then add them back in below.
let uri = req.uri().clone();
let method = req.method().clone();
let version = req.version().clone();
let req = crate::Request::from_http(req);
let (metadata, extensions, msg) = req.into_parts();

Expand All @@ -168,7 +175,7 @@ where
Ok(req) => {
let (metadata, extensions, _) = req.into_parts();
let req = crate::Request::from_parts(metadata, extensions, msg);
let req = req.into_http(uri, SanitizeHeaders::No);
let req = req.into_http(uri, method, version, SanitizeHeaders::No);
ResponseFuture::future(self.inner.call(req))
}
Err(status) => ResponseFuture::error(status),
Expand Down Expand Up @@ -322,4 +329,22 @@ mod tests {
assert_eq!(expected.version(), response.version());
assert_eq!(expected.headers(), response.headers());
}

#[tokio::test]
async fn doesnt_change_http_method() {
let svc = tower::service_fn(|request: http::Request<hyper::Body>| async move {
assert_eq!(request.method(), http::Method::OPTIONS);

Ok::<_, hyper::Error>(hyper::Response::new(hyper::Body::empty()))
});

let svc = InterceptedService::new(svc, |request: crate::Request<()>| Ok(request));

let request = http::Request::builder()
.method(http::Method::OPTIONS)
.body(hyper::Body::empty())
.unwrap();

svc.oneshot(request).await.unwrap();
}
}