Skip to content

feat: volo grpc https rpc ep #553

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ii64
Copy link
Member

@ii64 ii64 commented Mar 9, 2025

Motivation

Wrapper for core volo-grpc transport to use original endpoint directly, HTTPS ALPN rustls, example

Solution

@ii64 ii64 changed the title Feat/volo grpc rpc ep feat: volo grpc rpc ep Mar 9, 2025
@ii64 ii64 changed the title feat: volo grpc rpc ep feat: volo grpc https rpc ep Mar 9, 2025
@ii64
Copy link
Member Author

ii64 commented Mar 9, 2025

Flocky volo-http test result fail due to HTTPBIN responded with HTML

@ii64 ii64 marked this pull request as ready for review March 9, 2025 05:01
@ii64 ii64 requested review from a team as code owners March 9, 2025 05:01
});

let resp = client.list_app_gateways(req).await;
println!("resp = {:#?}", resp);
Copy link
Member

Choose a reason for hiding this comment

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

What output can we expect to verify?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one expect a gRPC status application level error, unauthorized - but that passes the gRPC deserialization

Comment on lines +167 to +168
req.headers_mut()
.insert(ACCEPT, HeaderValue::from_static("application/grpc"));
Copy link
Member

Choose a reason for hiding this comment

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

Should we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation specific, I think it can be optional

Comment on lines -89 to -118
let authority = uri.authority().expect("authority required").as_str();
let target: Address = match uri.scheme_str() {
Some("http") => Address::Ip(authority.parse::<SocketAddr>().map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"authority must be valid SocketAddr",
)
})?),
#[cfg(target_family = "unix")]
Some("http+unix") => {
use hex::FromHex;

let bytes = Vec::from_hex(authority).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"authority must be hex-encoded path",
)
})?;
Address::Unix(UnixSocketAddr::from_pathname(
String::from_utf8(bytes).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"authority must be valid UTF-8",
)
})?,
)?)
}
_ => unimplemented!(),
};

Copy link
Member

Choose a reason for hiding this comment

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

Why are these all removed? I think it will break the previous normal usage.

Copy link
Member Author

@ii64 ii64 Mar 16, 2025

Choose a reason for hiding this comment

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

Given that the connect transport is not exported, I'd say that nobody uses it besides volo-grpc client transport https://github.com/ii64/volo/blob/e4730223f692fdcd8a7b9c487fa9739378c125c3/volo-grpc/src/transport/mod.rs#L7

I understand that we need to encode the Address as hyper compatible connector only support http::Uri param, but since this connector never gets exported and that directly used by the volo-grpc client transport I think it is safe to assume that passing the Address via Future task local would work, that also eliminate the need to reencode Address on transport inter-call (Address -> http::Uri -> Address)

Copy link
Member

Choose a reason for hiding this comment

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

Get it, but we can still keep it as fallback if we don't get it from ADDRESS_HINT? we still have the Address -> http::Uri cost right now?

Copy link
Member

Choose a reason for hiding this comment

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

I think maybe we can add some comments here to explain the reason, or we may not remember the reason in the future.

let scheme = if self.tls { "https" } else { "http" };
let authority = match (scheme, self.port) {
("https", 443) | ("http", 80) => self.host.to_string(),
_ => format!("{}:{}", self.host.to_string(), self.port),
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary to_string()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants