-
Notifications
You must be signed in to change notification settings - Fork 86
fix(client): if tonic has tls feature #264
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
Conversation
|
|
||
| // Taken from https://github.com/hyperium/tonic/commit/b90c3408001f762a32409f7e2cf688ebae39d89e#diff-f27114adeedf7b42e8656c8a86205685a54bae7a7929b895ab62516bdf9ff252R15 | ||
| let channel = Endpoint::try_from("https://[::]") | ||
| let channel = Endpoint::try_from("http://[::]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there some scenarios where we would want https?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tonic::transport::Error(Transport, hyper::Error(Connect, HttpsUriWithoutTlsSupport(())))
Possibly we could enable a missing feature in Tonic instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The https://[::] is disabled here, you can any str replace https.(Except https and tls feature enable)
Connect will replace connect_with_connector function with xx.sock .
Style of writing because Tonic channel only build with Endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I see now, the comment right above links to a sample that states:
// We will ignore this uri because uds do not use it
// if your connector does use the uri it will be provided
// as the request to the `MakeConnection`.
Endpoint parses as URI but the domain socket isn't a uri so we move on and connect to the with our client. The uri that was initially passed is ignored in statement below tower::service_fn(move |_<this is the uri>| {
When the value is https and tls feature is enabled (as it is) it executes this code on that temp value: https://github.com/hyperium/tonic/blob/eeb3268f71ae5d1107c937392389db63d8f721fb/tonic/src/transport/service/connector.rs#L80-L91
We could essentially change this string to anything we want that parses as an "endpoint" (Uri) like unix://unused since it isn't used.
We could also be more explicit and append file::// to the path like:
let channel = Endpoint::try_from("file://var/run/containerd/containerd.sock")
.unwrap()
.connect_with_connector(tower::service_fn(move |uri: tonic::transport::Uri| {
let p = uri.path().to_owned();
UnixStream::connect(p)
}))
.await?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sorry, now I get it too :)
I think it deserves a comment.
|
LGTM, @fcfangcc could you add a comment explaining what is happening? |
|
@jsturtevant You mean to add a comment explaining in the source code? |
|
Yes, essentially describing a summary of #264 (comment). We had to dive in and dig around to figure out what was going on. It would be nice to leave a quick explanation for the next person to point out what seems like a strange set up (setting up with http/https but actually ignoring that value all together and using our own logic to connect). |
fixed #261
In tonic, if uri startwith "https", will use tls connect.
https://github.com/hyperium/tonic/blob/eeb3268f71ae5d1107c937392389db63d8f721fb/tonic/src/transport/service/connector.rs#L74-L90
Just change uri prefix "http" can skip this logic.Like:
https://github.com/hyperium/tonic/blob/eeb3268f71ae5d1107c937392389db63d8f721fb/examples/src/uds/client.rs#L19