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

[Junio C Hamano <gitster@pobox.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.,"?

> 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.

> 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?

> 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.

> 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.

> @@ -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().

>  	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
		}