git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] Add unix domain socket support to HTTP transport

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 23, 2024, 08:37 UTC
Message-ID
<xmqqzfvrzic9.fsf@gitster.g>
In-Reply-To
<pull.1681.v2.git.git.1708653536115.gitgitgadget@gmail.com>
"Leslie Cheng via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH v2] Add unix domain socket support to HTTP transport
Perhaps
	Subject: [PATCH] http: enable proxying via unix-domain socket
to follow the usual "<area>: <description>" format?
> From: Leslie Cheng <leslie.cheng5@gmail.com>
>
> This changeset introduces an `http.unixSocket` option so that users can

"This changeset introduces" -> "Introduce". There may be other gotchas that might use help from Documentation/SubmittingPatches, but I didn't read too carefully.

Besides, it is a single patch, not a set of changes ;-).

`http.unixSocket` is a configuration variable. It may be confusing to use the word "option". Speaking of options, shouldn't there be a command line option that overrides the configured value?

We should honor the usual http.<url>.VARIABLE convention where http.<url>.VARIABLE that is destination-specific overrides a more generic http.VARIABLE configuration variable.

> proxy their git over HTTP remotes to a unix domain socket. In terms of
> why, since UDS are local and git already has a local protocol: some
> corporate environments use a UDS to proxy requests to internal resources
> (ie. source control), so this change would support those use-cases. This
"ie." -> "i.e.,"?
Show 5 quoted lines
> proxy can occasionally be necessary to attach MFA tokens or client
> certificates for CLI tools.
>
> The implementation leverages `--unix-socket` option [0] via the
> `CURLOPT_UNIX_SOCKET_PATH` flag available with libcurl [1].

There is a feature in libcURL library, that is enabled by setting the CURLOPT_UNIX_SOCKET_PATH option via the curl_easy_setopt() call, and their command line utility. You do the same to implement this feature. But when you are not adding "--unix-socket" option to any of our commands, mention of that option name makes it more confusing than necessary.

The usual way to compose a log message of this project is to
 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.
 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).
 - Give commands to the codebase to "become like so".
in this order.
How about following that convention, perhaps like:
    In some corporate environments, the proxy server listens to a
    local unix domain socket for requests, instead of listening to a
    network port.  Even though we have http.proxy (and more
    destination specific http.<url>.proxy) configuration variables
    to specify the network address/port of a proxy, that would not
    help if your proxy does not listen to the network.
    Introduce an `http.unixSocket` (and `http.<url>.unixSocket`)
    configuration variables that specify the path to a unix domain
    socket for such a proxy.  Recent versions of libcURL library
    added CURLOPT_UNIX_SOCKET_PATH to support "curl --unix-socket
    <path>"---use the same mechanism to implement it.
> `GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH` and `NO_UNIX_SOCKETS` were kept
> separate so that we can spit out better error messages for users if git
> was compiled with `NO_UNIX_SOCKETS`.

Unlike NO_UNIX_SOCKETS, GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is entirely internal to your implementation and not surfaced to neither the end-users or the binary packagers. Because of that, I suspect that any description that has to use that name probably falls on the other side of "too much implementation details" to be useful to help future developers..

Besides, I suspect that GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH might not be the optimum approach. See below.

Show 12 quoted lines
> diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
> index 2d4e0c9b869..bf48cbd599a 100644
> --- a/Documentation/config/http.txt
> +++ b/Documentation/config/http.txt
> @@ -277,6 +277,11 @@ http.followRedirects::
>  	the base for the follow-up requests, this is generally
>  	sufficient. The default is `initial`.
>  
> +http.unixSocket::
> +	Connect through this Unix domain socket via HTTP, instead of using the
> +	network. If set, this config takes precendence over `http.proxy` and
> +	is incompatible with the proxy options (see `curl(1)`).

Talking about precedence between this and http.proxy is good thing, but one very important piece of information is missing. What value does it take?

	The absolute path of a unix-domain socket to pass the HTTP
	traffic over, instead of using the network.
or something, perhaps?
Show 14 quoted lines
> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index fd96b3cdffd..f0f3bec0e17 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -74,6 +74,13 @@
>  #define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
>  #endif
>  
> +/**
> + * CURLOPT_UNIX_SOCKET_PATH was added in 7.40.0, released in January 2015.
> + */
> +#if LIBCURL_VERSION_NUM >= 0x074000
> +#define GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH 1
> +#endif

The "HAVE" part in GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is a statement of a fact. If the version of cURL library we have is certain value, we have it. OK.

Show 7 quoted lines
> diff --git a/http.c b/http.c
> index e73b136e589..8cfdcaeac82 100644
> --- a/http.c
> +++ b/http.c
> @@ -79,6 +79,9 @@ static const char *http_proxy_ssl_ca_info;
>  static struct credential proxy_cert_auth = CREDENTIAL_INIT;
>  static int proxy_ssl_cert_password_required;
It might make the code easier to follow if you did:
	#if !defined(NO_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
	#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH)
        #define USE_CURLOPT_UNIX_SOCKET_PATH
	#endif
	#endif
        
The points are
 (1) the users can decline to use CURLOPT_UNIX_SOCKET_PATH while
     still using unix domain socket for other purposes, and
 (2) you do not have to care if you HAVE it or not most of time;
     what matters more often is if the user told you to USE it.
Hmm?
> +#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
> +static const char *curl_unix_socket_path;
> +#endif

The guard here would become "#ifdef USE_CURLOPT_UNIX_SOCKET_PATH" if we wanted this to be conditional, but I think it is easier to make the variable unconditionally available; see below.

Show 17 quoted lines
> @@ -455,6 +458,20 @@ static int http_options(const char *var, const char *value,
>  		return 0;
>  	}
>  
> +	if (!strcmp("http.unixsocket", var)) {
> +#ifdef GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH
> +#ifndef NO_UNIX_SOCKETS
> +		return git_config_string(&curl_unix_socket_path, var, value);
> +#else
> +		warning(_("Unix socket support unavailable in this build of Git"));
> +		return 0;
> +#endif
> +#else
> +		warning(_("Unix socket support is not supported with cURL < 7.40.0"));
> +		return 0;
> +#endif
> +	}

In general, it is inadvisable to issue a warning in the codepath that parses configuration variables, as the values we read may not be necessarily used. We could instead accept the given path into a variable unconditionally, and complain only before it gets used, near the call to curl_easy_setopt().

Show 12 quoted lines
>  	if (!strcmp("http.cookiefile", var))
>  		return git_config_pathname(&curl_cookie_file, var, value);
>  	if (!strcmp("http.savecookies", var)) {
> @@ -1203,6 +1220,12 @@ static CURL *get_curl_handle(void)
>  	}
>  	init_curl_proxy_auth(result);
>  
> +#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
> +	if (curl_unix_socket_path) {
> +		curl_easy_setopt(result, CURLOPT_UNIX_SOCKET_PATH, curl_unix_socket_path);
> +	}
> +#endif
Here, the guard may become more like
		if (curl_unix_socket_path) {
	#ifdef USE_CURLOPT_UNIX_SOCKET_PATH
			curl_easy_setopt(...);
	#elif defined(NO_CURLOPT_UNIX_SOCKET_PATH) || defined(NO_UNIX_SOCKETS)
			warning(_("this build disables the unix-domain-socket feature"));
	#elif
			warning(_("your cURL library is too old"));
	#endif
		}
Previous: Leslie Cheng via GitGitGadgetNext: Junio C Hamano
Message 5 of 7 in “Add unix domain socket support to HTTP transport.”
  1. Add unix domain socket support to HTTP transport.Leslie Cheng via GitGitGadget, Feb 21, 2024
  2. Eric WongFeb 21, 2024
  3. Leslie ChengFeb 22, 2024
  4. Add unix domain socket support to HTTP transportLeslie Cheng via GitGitGadget, Feb 23, 2024
  5. Junio C HamanoFeb 23, 2024
  6. Junio C HamanoFeb 23, 2024
  7. Leslie ChengFeb 23, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.