"Leslie Cheng via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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
}
