Re: [PATCH] BUG/MINOR: checks: Include UNIX server paths in arguments
Hi Willy, Do you think we should abstract the formatting of the address to a function and use the same format for the proxy address for external checks? Currently my changes just mimic the proxy address logic for server address, so if we want to add the ability to easily distinguish the socket types and support abstract sockets, I think both proxy address and server address parameters should be updated. - linuxdaemon
Re: [PATCH] BUG/MINOR: checks: Include UNIX server paths in arguments
Hi,
I think we could take this one, however I'm having some comments :
On Mon, Jun 10, 2019 at 04:13:19PM -0500, linuxdaemon wrote:
> --- a/src/checks.c
> +++ b/src/checks.c
> @@ -1891,8 +1891,23 @@ static int prepare_external_check(struct check *check)
> goto err;
> }
>
> - addr_to_str(>addr, buf, sizeof(buf));
> - check->argv[3] = strdup(buf);
> + // Make sure to send the unix socket address for the server to the
> external check command
> + if (s->addr.ss_family == AF_INET || s->addr.ss_family == AF_INET6) {
> + addr_to_str(>addr, buf, sizeof(buf));
> + check->argv[3] = strdup(buf);
> + }
> + else if (s->addr.ss_family == AF_UNIX) {
> + const struct sockaddr_un *un;
> +
> + un = (struct sockaddr_un *)>addr;
> + check->argv[3] = strdup(un->sun_path);
This will still not work for abstract namespace sockets, resulting in an
empty string. Maybe we should think about a way to encode such addresses
and document it so that external checks could use it.
Also, just thinking loud, what above differenciates the argument format
allowing an external check command to detect that this is a unix socket ?
I think we should at least use a prefix that's not allowed in AF_INET
nor AF_INET6 to make it unambigous. Otherwise if your UNIX socket's
address is the text representation of the equivalent IP listening
address (which is not uncommon) such as "192.168.1.35:8080", you will
create confusion for the external check process.
Maybe we should prefix unix sockets paths with "unix:" then we can
prefix abstract sockets with "abns:" ? Or maybe use "unix@" and "abns@"
just like in the haproxy config, that's less work to maintain homogenous
configurations and parsers outside.
Just my two cents,
Willy
[PATCH] BUG/MINOR: checks: Include UNIX server paths in arguments
The path of UNIX server connections should be sent as an argument to
the external-check command, to mimic how it behaves with IPv4 and v6
server connections, and UNIX proxy listeners. The port is set to an
empty string (existing behavior) instead of "NOT_USED" (to mimic
the proxy_port argument) to keep current functionality in that case.
Due to the minimal nature of this change, it is likely trivially
backported to stable versions which would allow users of the stable
versions to use external-check with UNIX socket server connections.
---
doc/configuration.txt | 5 +
src/checks.c | 19 +--
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 0da753a6..5da748b9 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -7466,6 +7466,11 @@ external-check command
possible to determine a listener, and both and
will have the string value "NOT_USED".
+ The and correspond to the configured server
+ address and port in the "backend" section. In the case of a UNIX socket
+ server connection, the will be the path of the socket and
+ the will be empty.
+
Some values are also provided through environment variables.
Environment variables :
diff --git a/src/checks.c b/src/checks.c
index 308b4b72..80266535 100644
--- a/src/checks.c
+++ b/src/checks.c
@@ -1891,8 +1891,23 @@ static int prepare_external_check(struct check *check)
goto err;
}
- addr_to_str(>addr, buf, sizeof(buf));
- check->argv[3] = strdup(buf);
+ // Make sure to send the unix socket address for the server to the
external check command
+ if (s->addr.ss_family == AF_INET || s->addr.ss_family == AF_INET6) {
+ addr_to_str(>addr, buf, sizeof(buf));
+ check->argv[3] = strdup(buf);
+ }
+ else if (s->addr.ss_family == AF_UNIX) {
+ const struct sockaddr_un *un;
+
+ un = (struct sockaddr_un *)>addr;
+ check->argv[3] = strdup(un->sun_path);
+ // Fall through to setting the port to "" to keep old
functionality
+ // instead of setting it to "NOT_USED" to mimic proxy_port.
+ }
+ else {
+ ha_alert("Starting [%s:%s] check: unsupported server address
family.\n", px->id, s->id);
+ goto err;
+ }
if (s->addr.ss_family == AF_INET || s->addr.ss_family == AF_INET6)
snprintf(buf, sizeof(buf), "%u", s->svc_port);
--
2.21.0
