Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
Willy, Ilya, Am 13.03.20 um 12:06 schrieb Willy Tarreau: >> I just realized that BUG_ON is not active by default (should we add >> DEBUG_STRICT=1 to CI?). The check in there does not even compile (facepalm). > > Yes, definitely a good idea. I've filed an issue for the CI expert Ilya to look into that: https://github.com/haproxy/haproxy/issues/546 Best regards Tim Düsterhus
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
On Fri, Mar 06, 2020 at 01:15:40PM +0100, Tim Duesterhus wrote:
> @@ -1466,6 +1466,20 @@ int make_proxy_line_v2(char *buf, int buf_len, struct
> server *srv, struct connec
> ret += make_tlv([ret], (buf_len - ret),
> PP2_TYPE_AUTHORITY, value_len, value);
> }
> }
> +
> + if (srv->pp_opts & SRV_PP_V2_UNIQUE_ID) {
> + struct session* sess = strm_sess(strm);
> + BUG_ON(sess != conn->owner); /* XXX is this correct? */
I'm pretty sure this one will break from time to time, maybe during
retries or when reusing an idle connection or something. Better drop
it. You don't really care who's the owner of the connection you're
using anyway. In case of multiplexing it could be anyone because
the connection will plugged onto by several streams before it's
finally connected and once it's OK and the sending code starts, you
have no way to tell whether the selected stream is the one that asked
for the connection first.
> + struct ist unique_id = stream_generate_unique_id(strm,
> >fe->format_unique_id);
> + value = unique_id.ptr;
> + value_len = unique_id.len;
Also above, please do two things:
- always leave a blank line between declarations and statements, that
allow to quickly spot where variables are declared
- and as such please don't place BUG_ON() in the declarations since
it's code :-)
> + if (value_len >= 0) {
> + if ((buf_len - ret) < sizeof(struct tlv))
> + return 0;
> + ret += make_tlv([ret], (buf_len - ret),
> PP2_TYPE_UNIQUE_ID, value_len, value);
> + }
> + }
>
> #ifdef USE_OPENSSL
> if (srv->pp_opts & SRV_PP_V2_SSL) {
> diff --git a/src/log.c b/src/log.c
> index 8f502ac7e..2cac07486 100644
> --- a/src/log.c
> +++ b/src/log.c
> @@ -137,7 +137,7 @@ static const struct logformat_type logformat_keywords[] =
> {
> { "CC", LOG_FMT_CCLIENT, PR_MODE_HTTP, LW_REQHDR, NULL }, /* client
> cookie */
> { "CS", LOG_FMT_CSERVER, PR_MODE_HTTP, LW_RSPHDR, NULL }, /* server
> cookie */
> { "H", LOG_FMT_HOSTNAME, PR_MODE_TCP, LW_INIT, NULL }, /* Hostname */
> - { "ID", LOG_FMT_UNIQUEID, PR_MODE_HTTP, LW_BYTES, NULL }, /* Unique ID
> */
> + { "ID", LOG_FMT_UNIQUEID, PR_MODE_TCP, LW_BYTES, NULL }, /* Unique ID */
> { "ST", LOG_FMT_STATUS, PR_MODE_TCP, LW_RESP, NULL }, /* status code
> */
> { "T", LOG_FMT_DATEGMT, PR_MODE_TCP, LW_INIT, NULL }, /* date GMT */
> { "Ta", LOG_FMT_Ta, PR_MODE_HTTP, LW_BYTES, NULL }, /* Time active
> (tr to end) */
> diff --git a/src/server.c b/src/server.c
> index 965570222..908439d00 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -656,6 +656,8 @@ static int srv_parse_proxy_v2_options(char **args, int
> *cur_arg,
> newsrv->pp_opts |= SRV_PP_V2_AUTHORITY;
> } else if (!strcmp(p, "crc32c")) {
> newsrv->pp_opts |= SRV_PP_V2_CRC32C;
> + } else if (!strcmp(p, "unique-id")) {
> + newsrv->pp_opts |= SRV_PP_V2_UNIQUE_ID;
> } else
> goto fail;
> }
> diff --git a/src/stream_interface.c b/src/stream_interface.c
> index 47cbd9693..d4acea8bc 100644
> --- a/src/stream_interface.c
> +++ b/src/stream_interface.c
> @@ -354,9 +354,11 @@ int conn_si_send_proxy(struct connection *conn, unsigned
> int flag)
> if (cs && cs->data_cb == _conn_cb) {
> struct stream_interface *si = cs->data;
> struct conn_stream *remote_cs =
> objt_cs(si_opposite(si)->end);
> + struct stream *strm = si_strm(si);
Same here, a blank line please, even though I know you were not the first
one to miss it.
> ret = make_proxy_line(trash.area, trash.size,
> objt_server(conn->target),
> - remote_cs ? remote_cs->conn :
> NULL);
> + remote_cs ? remote_cs->conn :
> NULL,
> + strm);
> /* We may not have a conn_stream yet, if we don't
>* know which mux to use, because it will be decided
>* during the SSL handshake. In this case, there should
(...)
So modulo the very few minor stuff above I'm overall OK without your
patch, just let me know if you have any question.
Thanks!
Willy
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
On Mon, Mar 09, 2020 at 05:07:38PM +0100, Tim Düsterhus wrote: > Willy, > > Am 06.03.20 um 13:15 schrieb Tim Duesterhus: > > is acceptable. There's also a `BUG_ON` left in the patch, because I wasn't > > sure if I should grab the session from the stream or from the connection. > > I just realized that BUG_ON is not active by default (should we add > DEBUG_STRICT=1 to CI?). The check in there does not even compile (facepalm). Yes, definitely a good idea. Willy
Re: [PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
Willy, Am 06.03.20 um 13:15 schrieb Tim Duesterhus: > is acceptable. There's also a `BUG_ON` left in the patch, because I wasn't > sure if I should grab the session from the stream or from the connection. I just realized that BUG_ON is not active by default (should we add DEBUG_STRICT=1 to CI?). The check in there does not even compile (facepalm). In any case: I just fixed that build failure locally and can confirm that the BUG_ON does not trigger. Consider that line a "Please carefully check my assumptions, Willy". Best regards Tim Düsterhus
[PATCH 3/3] MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
Willy, this patch adds the sending of generated unique IDs using PROXYv2. I'm not sure whether the way I made the stream available in the proxy line sending is acceptable. There's also a `BUG_ON` left in the patch, because I wasn't sure if I should grab the session from the stream or from the connection. I added a reg-test that verifies the current behavior. It's based on HTTP mode, even if that is not the primary purpose of the unique ID feature. The documentation update acknowledges that sending unique IDs in PROXYv2 might lead to unexpected results, but it generally works and does not crash anything. Best regards Tim Düsterhus Apply with `git am --scissors` to automatically cut the commit message. -- >8 -- This patch adds the `unique-id` option to `proxy-v2-options`. If this option is set a unique ID will be generated based on the `unique-id-format` while sending the proxy protocol v2 header and stored as the unique id for the first stream of the connection. This feature is meant to be used in `tcp` mode. It works on HTTP mode, but might result in inconsistent unique IDs for the first request on a keep-alive connection, because the unique ID for the first stream is generated earlier than the others. Now that we can send unique IDs in `tcp` mode the `%ID` log variable is made available in TCP mode. --- doc/configuration.txt | 24 +++ include/proto/connection.h| 4 +- include/types/server.h| 1 + .../proxy_protocol_send_unique_id.vtc | 42 +++ src/connection.c | 20 +++-- src/log.c | 2 +- src/server.c | 2 + src/stream_interface.c| 10 +++-- 8 files changed, 89 insertions(+), 16 deletions(-) create mode 100644 reg-tests/connection/proxy_protocol_send_unique_id.vtc diff --git a/doc/configuration.txt b/doc/configuration.txt index a078942bb..d781aba40 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -12651,13 +12651,23 @@ send-proxy-v2 this section and send-proxy" option of the "bind" keyword. proxy-v2-options [,]* - The "proxy-v2-options" parameter add option to send in PROXY protocol version - 2 when "send-proxy-v2" is used. Options available are "ssl" (see also - send-proxy-v2-ssl), "cert-cn" (see also "send-proxy-v2-ssl-cn"), "ssl-cipher": - name of the used cipher, "cert-sig": signature algorithm of the used - certificate, "cert-key": key algorithm of the used certificate), "authority": - host name value passed by the client (only sni from a tls connection is - supported), "crc32c": checksum of the proxy protocol v2 header. + The "proxy-v2-options" parameter add options to send in PROXY protocol + version 2 when "send-proxy-v2" is used. Options available are: + + - ssl : See also "send-proxy-v2-ssl". + - cert-cn : See also "send-proxy-v2-ssl-cn". + - ssl-cipher: Name of the used cipher. + - cert-sig : Signature algorithm of the used certificate. + - cert-key : Key algorithm of the used certificate + - authority : Host name value passed by the client (only SNI from a TLS +connection is supported). + - crc32c: Checksum of the PROXYv2 header. + - unique-id : Send a unique ID generated using the frontend's +"unique-id-format" within the PROXYv2 header. +This unique-id is primarily meant for "mode tcp". It can +lead to unexpected results in "mode http", because the +generated unique ID is also used for the first HTTP request +within a Keep-Alive connection. send-proxy-v2-ssl The "send-proxy-v2-ssl" parameter enforces use of the PROXY protocol version diff --git a/include/proto/connection.h b/include/proto/connection.h index 9b8eb8ad3..ecc03de8a 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -47,9 +47,9 @@ int conn_fd_check(struct connection *conn); /* receive a PROXY protocol header over a connection */ int conn_recv_proxy(struct connection *conn, int flag); -int make_proxy_line(char *buf, int buf_len, struct server *srv, struct connection *remote); +int make_proxy_line(char *buf, int buf_len, struct server *srv, struct connection *remote, struct stream *strm); int make_proxy_line_v1(char *buf, int buf_len, struct sockaddr_storage *src, struct sockaddr_storage *dst); -int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connection *remote); +int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct connection *remote, struct stream *strm); int conn_subscribe(struct connection *conn, void *xprt_ctx, int event_type, struct wait_event *es); int conn_unsubscribe(struct connection *conn, void *xprt_ctx, int event_type, struct wait_event *es); diff --git a/include/types/server.h b/include/types/server.h index 598dfe6d8..0f3052ee5 100644 ---
