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.

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

Support http over unix domain sockets #392

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Dec 29, 2024

Adds a --unix-socket option similar to cURL for binding to a Unix domain socket. When used, all requests, including redirects, will go through the specified Unix socket.

Resources used:

Resolves #230


pub fn execute(&self, request: Request) -> Result<Response> {
self.rt.block_on(async {
// TODO: Support named pipes by replacing tokio::net::UnixStream::connect(..) with:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Besides Docker for Window, I couldn't find any other examples of HTTP over named pipes, so I'm not sure if it's worth supporting at this time.

src/main.rs Outdated
Comment on lines 555 to 556
#[cfg(target_family = "unix")]
if let Some(unix_socket) = args.unix_socket {
Copy link
Owner Author

Choose a reason for hiding this comment

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

There is a fork of hyperlocal that adds support for windows but it might be better to wait until it is merged upstream

@ducaale ducaale marked this pull request as ready for review December 29, 2024 22:18
@ducaale ducaale marked this pull request as draft December 29, 2024 22:19
}
});

// TODO: don't ignore value from --timeout option
Copy link
Owner Author

Choose a reason for hiding this comment

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

Regardless of how this is handled, I need to ensure the correct exit code is used for timeout errors

Copy link
Owner Author

@ducaale ducaale Jan 7, 2025

Choose a reason for hiding this comment

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

Note to self: refer to impl<B> hyper::body::Body for TotalTimeoutBody<B> for how to implement timeout for body.

Also, we should consider moving to a combination of connect_timeout and read_timeout which is closer to how it is handled in python requests/HTTPie. See https://requests.readthedocs.io/en/stable/user/advanced/#timeouts

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm, it seems read_timeout is not yet implemented for reqwest::blocking::Client

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Nice! I haven't looked at everything yet

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/cli.rs Outdated
@@ -347,6 +347,12 @@ Example: --print=Hb"
#[clap(short = '6', long)]
pub ipv6: bool,

/// Connect using a Unix domain socket.
///
/// Example: --unix_socket=/var/run/temp.sock
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this API you still have to make up a hostname for the URL, like curl, and unlike got and the HTTPie plugin.

AFAICT the typical use cases are for local services and not for proxies, meaning that the hostname is superfluous. But it also doesn't look like we're going to get a URL-based convention for this any time soon. And splitting it off into an option prevents redirect attacks. (Not sure how it should interact with sessions though... we need to look carefully at other areas of interference.)

A full command with the : shorthand might help users, e.g.

xh :/index.html --unix-socket=/var/run/temp.sock

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is a good chance that unix domain sockets will be treated as a proxy in reqwest. This has the advantage that anything relying on hostname like cookies and Host header will keep working. Also, see denoland/deno#8821 (comment)

However, I still think we can support xh http://unix:/var/run/docker.sock:/containers/json by treating it as a syntactic sugar for xh :/containers/json --unix-socket=/var/run/docker.sock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can always add syntactic sugar later. Right now there doesn't seem to be any consensus about the URL format.

If it ever gets to the point where browsers start supporting UDS URLs (as a non-experimental feature) then we should follow suit.

Comment on lines 65 to 70
let starting_time = Instant::now();
let mut response = self.execute(request)?;
response.extensions_mut().insert(ResponseMeta {
request_duration: starting_time.elapsed(),
content_download_duration: None,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we skip any subsequent middleware this way? I don't understand this yet but it might be the wrong layer to do it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is the last layer, but returning early and short-circuiting middlewares in this way might be confusing, not to mention the duplicated logic. Let me see if I can switch between UnixSocket and reqwest client inside ClientWithMiddleware.

@ducaale ducaale marked this pull request as ready for review January 7, 2025 10:46
@ducaale ducaale requested a review from blyxxyz January 11, 2025 18:11
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good so far! (I haven't understood everything yet.)

Do you have tips for manually testing this?

#[clap(
long,
value_name = "FILE",
conflicts_with_all=["proxy", "verify", "cert", "cert_key", "ssl", "resolve", "interface", "ipv4", "ipv6", "https", "http_version"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of these might plausibly appear in an alias or config file, like --https, --ssl.

If someone passes --proxy they probably made a mistake but I think the list could be shorter. Maybe a guideline could be whether the option is compatible with a normal request to http://127.0.0.1?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Were you thinking of silently ignoring some of the flags or maybe printing a warning?

Copy link
Collaborator

Choose a reason for hiding this comment

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

--https for example doesn't force HTTPS, it just changes the default scheme of the URL. So maybe the user gets an error if they pass :/index.html because that gets converted to https://localhost/index.html and the socket doesn't support TLS, but if they use --https with an explicit http://localhost/index.html then that should be perfectly fine, just like in xh --https http://127.0.0.1 the --https flag is useless but harmless.

Comment on lines +41 to +45
tokio::task::spawn(async move {
if let Err(err) = conn.await {
log::error!("Connection failed: {:?}", err);
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it OK to merely log this? I assume it leads to a harder error somewhere down the line and it's just hard to get it out of the task?

Comment on lines +54 to +55
let mut sender = with_timeout(self.connect(), self.timeout).await??;
let response = with_timeout(sender.send_request(http_request), self.timeout).await??;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean we have a separate timeout for each stage?

Will there be a timeout for e.g. a large file upload?

Comment on lines +138 to +142
let file_name: String = rand::thread_rng()
.sample_iter(&rand::distributions::Alphanumeric)
.take(10)
.map(char::from)
.collect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like tempfile can be used for this: https://docs.rs/tempfile/latest/tempfile/struct.Builder.html#examples-11

That way we also get cleanup

@ducaale
Copy link
Owner Author

ducaale commented Jan 21, 2025

Do you have tips for manually testing this?

I have been using Docker API but you can also use Bun which natively supports unix domain sockets

// bun index.js

async function* hello(times) {
  for (let i = 0; i < times; i++) {
    yield `data: hello ${i}\n\n`;
    await Bun.sleep(1000);
  }
}

const server = Bun.serve({
  unix: "/tmp/my-socket.sock",
  fetch(req){
    return new Response(hello(10), {
      headers: {
        "Content-Type": "text/event-stream",
        Connection: "keep-alive",
      }
    });
  }
});

console.log(`Listening on unix:///tmp/my-socket.sock`);

I'm noticing some decoding errors so I will have to investigate that

$ cargo r -- -v : --unix-socket=/tmp/my-socket.sock

...lines omitted

data: hello 7

data: hello 8

xh: error: error decoding response body

Caused by:
    0: request or response body error
    1: error reading a body from connection
    2: unexpected EOF during chunk size line

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

Successfully merging this pull request may close these issues.

Support For Calling Unix Domain Sockets
2 participants