-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Do not require authority for UNIX domain sockets #487
base: master
Are you sure you want to change the base?
Conversation
c4b2913
to
86979b3
Compare
95a0d59
to
69444f5
Compare
Test are failing now for sure. In case of an unix socket, we would only get a path and nothing else. Do you see any feasible way to implement this in h2? |
The description mentions not requiring an authority, but it looks like this changes it to stop checking if the provided authority is valid. So, in the Unix socket case, does it send an |
Unfortunately not, the authority would be the complete path, like |
Would it be okay to early exit the validation path if the parsed URI is a path? Not sure how to classify a “path” though… |
0447e60
to
7482820
Compare
@seanmonstar I checked for a path. If this exists locally, then we assume that the connection through the UDS should work. |
7482820
to
1fc278f
Compare
Is there anyone over here to give this a review? 😇 |
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.
As I mentioned in grpc/grpc-go#3854 (comment), I think it's fine to ignore the :authority
if it's a blank value. I would expect a client to not send an invalid authority.
src/server.rs
Outdated
// When connecting to a UNIX Domain Socket (UDS), then we might get a path for the | ||
// authority field. If it's a local path and exists, then we do not error in that case | ||
// and assume an UDS. | ||
if !Path::new(authority.as_str()).exists() { |
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.
This will do a blocking IO call, which we wouldn't want to do in an async environment.
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.
Removed in favor of a check for a .sock
suffix.
c993b8c
to
1fc278f
Compare
Yes, but in case of a unix domain socket the authority is not blank. It's the socket path. |
1fc278f
to
3a7d79b
Compare
812e70d
to
3354233
Compare
This fixes connections where a local UNIX domain socket path is provided, where the authority contains the full path to the *.sock file or is empty. Signed-off-by: Sascha Grunert <sgrunert@suse.com>
3354233
to
1f99ad3
Compare
PTAL @seanmonstar I changed the check to either exclude |
Has 0.4.0 solved the problem? |
Just want to add that I am facing a similar issue using h2 to process gRPC requests using tonic. I am using h2 v0.3.1. I have fixed the problem by just sending a warning when Maybe we should find another more elegant solution than just ignoring |
Didn't grpc-go change to not emit those malformed authority headers? |
Any idea when this could be merged? Calling gRPC methods from Go clients or grpcurl on unix domain sockets still does not work due to this issue. |
@seanmonstar PTAL |
For anyone trying to get that working, the workaround is to simply send a dummy authority field from the client side:
|
Changing this on the client side is currently not an option as I can't patch every kubelet. Currently this issue is preventig me from properly implementing a Kubernetes Device plugin in Rust using Tonic. |
I am exactly in the same situation: kubelet is the client and patch it is not an option for me either. This is why few month ago I have fork this repository to disable panic if authority field is Moreover, from k8s point of view, authority field is not invalid. It represent namespace from where the request came from. This is not just a valid http authority but it still contains meaningful network information. In my case, forking unlock me and I have been able to make k8s extension works correctly. But from my point of view this is too bad request fail because an optional field cannot be parsed. |
@saschagrunert I think only checking for @seanmonstar What would be the harm if we just ignore an invalid authority instead of failing with an error? So the change would essentially become bachp@0a3f73b |
Totally agree on the fact there is nothing that enforce a socket file ending with A better check for that would be to check file descriptor exists and check it is a valid socket file (but it will still not works for unnamed unix socket). The patch you have made is exactly the same I have suggest around 1 year ago 😉 2c26eda . If any maintainers want I would be happy to make a PR for this or you could take the solution of @bachp without the log. |
@arthurlm I think logging it makes a lot of sense. |
Before creating any PR, I have done more investigation to understand clearly what is happening. In my extension service k8s case the Looking at RFCs:
So, from my understanding:
NOTE: I have not read the whole RFCs but just read in details the mentioned sections. If anyone have better understanding of this RFCs, please feel free to comment 😉 ! Looking at current code:
I now think we are here in a corner case.
@seanmonstar / hyperium maintainers: what is your point on view on this ? Should we consider Should we fail request if Any other ideas or comments ? |
@arthurlm I like your proposal to only ignore the malformed |
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.
I encountered the same problem, this solution can be applied to the scenario of UDS address.
Copy from hyperium#487 Thanks for the help from Sascha Grunert.
This fixes connections where paths to UNIX domain sockets (
*.sock
) are provided or the authority is empty.Refers to hyperium/tonic#243, containers/containrs#34