Hi Willy, Thanks for your feedback once again.
TLDR: I agree with your proposed changes. So, here is my corresponding proposal: We can ignore the last two commits for now (LOW: connection: Add TLV update function and MEDIUM: tcp-act: Add new set-tlv TCP action for PPv2 TLVs). Based on the first two commits, I created a diff that would implement something like you requested (new parser and no use of remote). It should be fully working, so you could even try it out locally if you want. If you think it looks promising, I will adjust the tests and the docs, and update the series. Now to address your feedback in more detail: > I'm really having an issue with using "the remote frontend connection" > here. We've done this mistake in the past with the transparent mode that > connects to the original destination address, that resulted in "set-dst" > having to be implemented, then being broken by multiplexed streams (e.g. > h2), then having to be internally emulated at various layers (connection, > session, stream, transaction etc). Same for src. The solution is not durable > at all, and as you noticed, you already had to implement the ability to > modify these fields before passing them, hence reintroducing that class of > trouble on a new area. I choose to use the remote connection since it is already done that way for other TLV types like SRV_PP_V2_AUTHORITY (https://github.com/haproxy/haproxy/commit/8a4ffa0aabf8509098f95ac78fa48b18f415c42c). It simply forwards what is found the remote connection, if there is something set. Similarly, PP2_TYPE_NETNS and SRV_PP_V2_SSL depend on remote. First, I did not like the method with an extra fetch since it creates potential overhead. However, I am likely a bit excessive here. I am not opposed to not using remote at all. I simply was not aware of the intricacies. > Also what will be left in the logs after you modify the contents ? etc. You mean that the log does not match the content of the actual TLV? It could happen, I suppose. But why doesn't this problem with multiplexed connections apply to all the TCP actions? As far as I know they all alter fields of the connection directly. Doesn't really matter though, I don't plan to use it anymore. > I'm seeing another issue with passing type=fmt like this. The option is > currently defined as cutting around commas, and that's what it does. This > means that the format will not be parsable (no sample fetch functions with > more than one argument, no converters). Example: > > proxy-v2-options 0xEE=%[var(txn.clientname,_unknown_)] > ^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^ > First option Second option I see, that is a problem. Of course, we could track whether we are currently in a log format expression. But I think I have come up with a better implementation, see below. > Why not something like "set-proxy-v2-tlv"? Maybe we then could also leave out > the v2. I would propose to go with the set-proxy-v2-tlv-fmt as you suggested later. I would leave in the v2 as other server options that concern v2 also leave it in. I don't mind being more verbose here. Realistically, there are only 2-3 of these set-proxy-v2-tlv-fmt anyway. Expression could make sense at some later point in time. > That's also why I'm extremely careful about the config shortcomings that can > come with it, because I suspect it could become popular and we really want to > avoid new fall traps. Sure, that is completely understandable. > I've been wondering if we want to use a format string or raw binary data > here, because some options are binary and nothing indicates that this will > not continue in the future. However since fc_pp_tlv() returns a string, I > guess most of them will continue this way. I also think type string is fine. > Also once this is merged, a new problem will emerge. There's still no > registry for the PPv2 TLV types. I guess this should be okay for most cases where it is just used internally. But yeah, most people will just come up with TLVs off the top their head. I will definitely add a note about the ranges. > Given that you had previously contributed the ability to fetch a TLV field > from the front connection's proxy protocol using fc_pp_tlv(), I'd rather make > use of these ones explicitly. If you need to change them, then you just > assign them to a variable and edit that variable, then send the variable. Or > you can change them in-flight using a converter. Of course it would make > configs slightly more verbose, but they would always do the right thing and > we would not have to continue to special-case them over time because they > break expectations like set-src/set-dst did. Generally, I wouldn't mind a more verbose config. As usually there shouldn't be too many TLV types that need to be considered. I have no problem leaving out the setter. It was just merely an (premature) optimization approach. > I think that most of your patch will remain as-is, it's only the parsing part > which needs to change a little bit to take such points into account. Ok, great! Here is how I adjusted the first and the second commit (besides Doc and Tests). It works quite nicely with the keyword. An example server config works like this now: server dummy_server 127.0.0.1:2319 send-proxy-v2 set-proxy-v2-tlv-fmt(0x10) %[str("hi")] set-proxy-v2-tlv-fmt(0x11) %[fc_pp_tlv(0x11)] I could perfectly replicate the forwarding behavior with the fc_pp_tlv. I hope you are fine with the changes. If so, I would send the updated series. Then, we could hopefully get into the final, detailed part of review. BTW, I've read that there is a feature freeze for 2.9, so I guess this change comes too late now? Best, Alexander ------------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/connection.c b/src/connection.c index 07da8a5e2..6446a8fed 100644 --- a/src/connection.c +++ b/src/connection.c @@ -1927,7 +1927,6 @@ static int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct const struct sockaddr_storage *dst = &null_addr; const char *value = ""; int value_len = 0; - int found = 0; if (buf_len < PP2_HEADER_LEN) return 0; @@ -2003,6 +2002,9 @@ static int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct list_for_each_entry(srv_tlv, &srv->pp_tlvs, list) { replace = NULL; + /* Users will always need to provide a value, in case of forwarding, they should use fc_pp_tlv. + * for generic types. Otherwise, we will send an empty TLV. + */ if (!LIST_ISEMPTY(&srv_tlv->fmt)) { replace = alloc_trash_chunk(); if (unlikely(!replace)) @@ -2017,23 +2019,8 @@ static int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct free_trash_chunk(replace); } else { - found = 0; - /* Fall back to use TLV value from remote connection, if available */ - if (remote) { - struct conn_tlv_list *remote_tlv = NULL; - - /* Search in remote connection TLV list */ - list_for_each_entry(remote_tlv, &remote->tlv_list, list) { - if (remote_tlv->type != srv_tlv->type) - continue; - ret += make_tlv(&buf[ret], (buf_len - ret), srv_tlv->type, remote_tlv->len, remote_tlv->value); - found = 1; - break; - } - } - if (!found) - /* Create empty TLV as no value was specified */ - ret += make_tlv(&buf[ret], (buf_len - ret), srv_tlv->type, 0, NULL); + /* Create empty TLV as no value was specified */ + ret += make_tlv(&buf[ret], (buf_len - ret), srv_tlv->type, 0, NULL); } } } diff --git a/src/server.c b/src/server.c index 08f86d030..02084314a 100644 --- a/src/server.c +++ b/src/server.c @@ -1036,13 +1036,7 @@ static int srv_parse_proto(char **args, int *cur_arg, static int srv_parse_proxy_v2_options(char **args, int *cur_arg, struct proxy *px, struct server *newsrv, char **err) { - char *p = NULL, *n = NULL, *m = NULL; - char *key = NULL, *val = NULL; - char *endp = NULL; - unsigned char idx = 0; - size_t key_length = 0, val_length = 0; - struct srv_pp_tlv_list *srv_tlv = NULL; - + char *p, *n; for (p = args[*cur_arg+1]; p; p = n) { n = strchr(p, ','); if (n) @@ -1067,61 +1061,75 @@ static int srv_parse_proxy_v2_options(char **args, int *cur_arg, newsrv->pp_opts |= SRV_PP_V2_CRC32C; } else if (strcmp(p, "unique-id") == 0) { newsrv->pp_opts |= SRV_PP_V2_UNIQUE_ID; - } else { - /* reset val in case it was set before */ - val = NULL; - - /* try to split by '=' into generic pair key value pair (<id>=<expr>) */ - m = strchr(p, '='); - if (m) { - key_length = m - p; - key = (char *) malloc(key_length + 1); - if (unlikely(!key)) - goto fail; - snprintf(key, key_length + 1, "%s", p); - - val_length = strlen(p) - key_length; - val = (char *) malloc(val_length + 1); - if (unlikely(!val)) - goto fail; - snprintf(val, val_length + 1, "%s", m + 1); - } - else { - key = (char *) malloc(strlen(p) + 1); - if (unlikely(!key)) - goto fail; - snprintf(key, strlen(p) + 1, "%s", p); - } + } else + goto fail; + } + return 0; + fail: + if (err) + memprintf(err, "'%s' : proxy v2 option not implemented", p); + return ERR_ALERT | ERR_FATAL; +} - errno = 0; - idx = strtoul(key, &endp, 0); - if (unlikely((endp && *endp) != '\0' || (key == endp) || (errno != 0))) - goto fail; +/* Parse the "set-proxy-v2-tlv-fmt" server keyword */ +static int srv_parse_set_proxy_v2_tlv_fmt(char **args, int *cur_arg, + struct proxy *px, struct server *newsrv, char **err) +{ + char *error = NULL, *cmd_name = NULL, *tlv_fmt = NULL; + unsigned int tlv_type = 0; + struct srv_pp_tlv_list *srv_tlv = NULL; - /* processing is done in connection.c */ - srv_tlv = malloc(sizeof(struct srv_pp_tlv_list)); - if (unlikely(!srv_tlv)) - goto fail; + cmd_name = args[*cur_arg]; + if (!*cmd_name) { + goto fail; + } - srv_tlv->type = idx; - LIST_INIT(&srv_tlv->fmt); - if (val && unlikely(!parse_logformat_string(val, px, &srv_tlv->fmt, LOG_OPT_HTTP, PR_CAP_LISTEN, err))) - goto fail; + cmd_name += strlen("set-proxy-v2-tlv-fmt"); - LIST_APPEND(&newsrv->pp_tlvs, &srv_tlv->list); - free(key); - free(val); + if (*cmd_name == '(') { + cmd_name++; /* skip the '(' */ + errno = 0; + tlv_type = strtoul(cmd_name, &error, 0); /* convert TLV ID */ + if (unlikely((cmd_name == error) || (errno != 0))) { + memprintf(err, "'%s' : could not convert TLV ID", args[*cur_arg]); + goto fail; + } + if (errno == EINVAL) { + memprintf(err, "'%s' : could not find a valid number for the TLV ID", args[*cur_arg]); + goto fail; + } + if (*error != ')') { + memprintf(err, "'%s' : expects set-proxy-v2-tlv(<TLV ID>)", args[*cur_arg]); + goto fail; } + if (tlv_type > 0xFF) { + memprintf(err, "'%s' : the maximum allowed TLV ID is %d", args[*cur_arg], 0xFF); + goto fail; + } + } + + srv_tlv = malloc(sizeof(struct srv_pp_tlv_list)); + if (unlikely(!srv_tlv)) { + memprintf(err, "'%s' : failed to parse allocate TLV entry", args[*cur_arg]); + goto fail; } + srv_tlv->type = tlv_type; + + tlv_fmt = args[*cur_arg + 1]; + LIST_INIT(&srv_tlv->fmt); + if (tlv_fmt && unlikely(!parse_logformat_string(tlv_fmt, px, &srv_tlv->fmt, LOG_OPT_HTTP, PR_CAP_LISTEN, err))) { + memprintf(err, "'%s' : failed to parse format string", args[*cur_arg]); + goto fail; + } + LIST_APPEND(&newsrv->pp_tlvs, &srv_tlv->list); + + (*cur_arg)++; + return 0; -fail: - free(key); - free(val); + + fail: free(srv_tlv); errno = 0; - - if (err) - memprintf(err, "'%s' : failed to set proxy v2 option", p); return ERR_ALERT | ERR_FATAL; } @@ -1933,50 +1941,51 @@ void srv_compute_all_admin_states(struct proxy *px) * Note: -1 as ->skip value means that the number of arguments are variable. */ static struct srv_kw_list srv_kws = { "ALL", { }, { - { "backup", srv_parse_backup, 0, 1, 1 }, /* Flag as backup server */ - { "cookie", srv_parse_cookie, 1, 1, 0 }, /* Assign a cookie to the server */ - { "disabled", srv_parse_disabled, 0, 1, 1 }, /* Start the server in 'disabled' state */ - { "enabled", srv_parse_enabled, 0, 1, 1 }, /* Start the server in 'enabled' state */ - { "error-limit", srv_parse_error_limit, 1, 1, 1 }, /* Configure the consecutive count of check failures to consider a server on error */ - { "ws", srv_parse_ws, 1, 1, 1 }, /* websocket protocol */ - { "id", srv_parse_id, 1, 0, 1 }, /* set id# of server */ - { "init-addr", srv_parse_init_addr, 1, 1, 0 }, /* */ - { "log-proto", srv_parse_log_proto, 1, 1, 0 }, /* Set the protocol for event messages, only relevant in a ring section */ - { "maxconn", srv_parse_maxconn, 1, 1, 1 }, /* Set the max number of concurrent connection */ - { "maxqueue", srv_parse_maxqueue, 1, 1, 1 }, /* Set the max number of connection to put in queue */ - { "max-reuse", srv_parse_max_reuse, 1, 1, 0 }, /* Set the max number of requests on a connection, -1 means unlimited */ - { "minconn", srv_parse_minconn, 1, 1, 1 }, /* Enable a dynamic maxconn limit */ - { "namespace", srv_parse_namespace, 1, 1, 0 }, /* Namespace the server socket belongs to (if supported) */ - { "no-backup", srv_parse_no_backup, 0, 1, 1 }, /* Flag as non-backup server */ - { "no-send-proxy", srv_parse_no_send_proxy, 0, 1, 1 }, /* Disable use of PROXY V1 protocol */ - { "no-send-proxy-v2", srv_parse_no_send_proxy_v2, 0, 1, 1 }, /* Disable use of PROXY V2 protocol */ - { "no-tfo", srv_parse_no_tfo, 0, 1, 1 }, /* Disable use of TCP Fast Open */ - { "non-stick", srv_parse_non_stick, 0, 1, 0 }, /* Disable stick-table persistence */ - { "observe", srv_parse_observe, 1, 1, 1 }, /* Enables health adjusting based on observing communication with the server */ - { "on-error", srv_parse_on_error, 1, 1, 1 }, /* Configure the action on check failure */ - { "on-marked-down", srv_parse_on_marked_down, 1, 1, 1 }, /* Configure the action when a server is marked down */ - { "on-marked-up", srv_parse_on_marked_up, 1, 1, 1 }, /* Configure the action when a server is marked up */ - { "pool-low-conn", srv_parse_pool_low_conn, 1, 1, 1 }, /* Set the min number of orphan idle connecbefore being allowed to pick from other threads */ - { "pool-max-conn", srv_parse_pool_max_conn, 1, 1, 1 }, /* Set the max number of orphan idle connections, -1 means unlimited */ - { "pool-purge-delay", srv_parse_pool_purge_delay, 1, 1, 1 }, /* Set the time before we destroy orphan idle connections, defaults to 1s */ - { "proto", srv_parse_proto, 1, 1, 1 }, /* Set the proto to use for all outgoing connections */ - { "proxy-v2-options", srv_parse_proxy_v2_options, 1, 1, 1 }, /* options for send-proxy-v2 */ - { "redir", srv_parse_redir, 1, 1, 0 }, /* Enable redirection mode */ - { "resolve-net", srv_parse_resolve_net, 1, 1, 0 }, /* Set the preferred network range for name resolution */ - { "resolve-opts", srv_parse_resolve_opts, 1, 1, 0 }, /* Set options for name resolution */ - { "resolve-prefer", srv_parse_resolve_prefer, 1, 1, 0 }, /* Set the preferred family for name resolution */ - { "resolvers", srv_parse_resolvers, 1, 1, 0 }, /* Configure the resolver to use for name resolution */ - { "send-proxy", srv_parse_send_proxy, 0, 1, 1 }, /* Enforce use of PROXY V1 protocol */ - { "send-proxy-v2", srv_parse_send_proxy_v2, 0, 1, 1 }, /* Enforce use of PROXY V2 protocol */ - { "shard", srv_parse_shard, 1, 1, 1 }, /* Server shard (only in peers protocol context) */ - { "slowstart", srv_parse_slowstart, 1, 1, 1 }, /* Set the warm-up timer for a previously failed server */ - { "source", srv_parse_source, -1, 1, 1 }, /* Set the source address to be used to connect to the server */ - { "stick", srv_parse_stick, 0, 1, 0 }, /* Enable stick-table persistence */ - { "tfo", srv_parse_tfo, 0, 1, 1 }, /* enable TCP Fast Open of server */ - { "track", srv_parse_track, 1, 1, 1 }, /* Set the current state of the server, tracking another one */ - { "socks4", srv_parse_socks4, 1, 1, 0 }, /* Set the socks4 proxy of the server*/ - { "usesrc", srv_parse_usesrc, 0, 1, 1 }, /* safe-guard against usesrc without preceding <source> keyword */ - { "weight", srv_parse_weight, 1, 1, 1 }, /* Set the load-balancing weight */ + { "backup", srv_parse_backup, 0, 1, 1 }, /* Flag as backup server */ + { "cookie", srv_parse_cookie, 1, 1, 0 }, /* Assign a cookie to the server */ + { "disabled", srv_parse_disabled, 0, 1, 1 }, /* Start the server in 'disabled' state */ + { "enabled", srv_parse_enabled, 0, 1, 1 }, /* Start the server in 'enabled' state */ + { "error-limit", srv_parse_error_limit, 1, 1, 1 }, /* Configure the consecutive count of check failures to consider a server on error */ + { "ws", srv_parse_ws, 1, 1, 1 }, /* websocket protocol */ + { "id", srv_parse_id, 1, 0, 1 }, /* set id# of server */ + { "init-addr", srv_parse_init_addr, 1, 1, 0 }, /* */ + { "log-proto", srv_parse_log_proto, 1, 1, 0 }, /* Set the protocol for event messages, only relevant in a ring section */ + { "maxconn", srv_parse_maxconn, 1, 1, 1 }, /* Set the max number of concurrent connection */ + { "maxqueue", srv_parse_maxqueue, 1, 1, 1 }, /* Set the max number of connection to put in queue */ + { "max-reuse", srv_parse_max_reuse, 1, 1, 0 }, /* Set the max number of requests on a connection, -1 means unlimited */ + { "minconn", srv_parse_minconn, 1, 1, 1 }, /* Enable a dynamic maxconn limit */ + { "namespace", srv_parse_namespace, 1, 1, 0 }, /* Namespace the server socket belongs to (if supported) */ + { "no-backup", srv_parse_no_backup, 0, 1, 1 }, /* Flag as non-backup server */ + { "no-send-proxy", srv_parse_no_send_proxy, 0, 1, 1 }, /* Disable use of PROXY V1 protocol */ + { "no-send-proxy-v2", srv_parse_no_send_proxy_v2, 0, 1, 1 }, /* Disable use of PROXY V2 protocol */ + { "no-tfo", srv_parse_no_tfo, 0, 1, 1 }, /* Disable use of TCP Fast Open */ + { "non-stick", srv_parse_non_stick, 0, 1, 0 }, /* Disable stick-table persistence */ + { "observe", srv_parse_observe, 1, 1, 1 }, /* Enables health adjusting based on observing communication with the server */ + { "on-error", srv_parse_on_error, 1, 1, 1 }, /* Configure the action on check failure */ + { "on-marked-down", srv_parse_on_marked_down, 1, 1, 1 }, /* Configure the action when a server is marked down */ + { "on-marked-up", srv_parse_on_marked_up, 1, 1, 1 }, /* Configure the action when a server is marked up */ + { "pool-low-conn", srv_parse_pool_low_conn, 1, 1, 1 }, /* Set the min number of orphan idle connecbefore being allowed to pick from other threads */ + { "pool-max-conn", srv_parse_pool_max_conn, 1, 1, 1 }, /* Set the max number of orphan idle connections, -1 means unlimited */ + { "pool-purge-delay", srv_parse_pool_purge_delay, 1, 1, 1 }, /* Set the time before we destroy orphan idle connections, defaults to 1s */ + { "proto", srv_parse_proto, 1, 1, 1 }, /* Set the proto to use for all outgoing connections */ + { "proxy-v2-options", srv_parse_proxy_v2_options, 1, 1, 1 }, /* options for send-proxy-v2 */ + { "redir", srv_parse_redir, 1, 1, 0 }, /* Enable redirection mode */ + { "resolve-net", srv_parse_resolve_net, 1, 1, 0 }, /* Set the preferred network range for name resolution */ + { "resolve-opts", srv_parse_resolve_opts, 1, 1, 0 }, /* Set options for name resolution */ + { "resolve-prefer", srv_parse_resolve_prefer, 1, 1, 0 }, /* Set the preferred family for name resolution */ + { "resolvers", srv_parse_resolvers, 1, 1, 0 }, /* Configure the resolver to use for name resolution */ + { "send-proxy", srv_parse_send_proxy, 0, 1, 1 }, /* Enforce use of PROXY V1 protocol */ + { "send-proxy-v2", srv_parse_send_proxy_v2, 0, 1, 1 }, /* Enforce use of PROXY V2 protocol */ + { "set-proxy-v2-tlv-fmt", srv_parse_set_proxy_v2_tlv_fmt, 0, 1, 1 }, /* Set TLV of PROXY V2 protocol */ + { "shard", srv_parse_shard, 1, 1, 1 }, /* Server shard (only in peers protocol context) */ + { "slowstart", srv_parse_slowstart, 1, 1, 1 }, /* Set the warm-up timer for a previously failed server */ + { "source", srv_parse_source, -1, 1, 1 }, /* Set the source address to be used to connect to the server */ + { "stick", srv_parse_stick, 0, 1, 0 }, /* Enable stick-table persistence */ + { "tfo", srv_parse_tfo, 0, 1, 1 }, /* enable TCP Fast Open of server */ + { "track", srv_parse_track, 1, 1, 1 }, /* Set the current state of the server, tracking another one */ + { "socks4", srv_parse_socks4, 1, 1, 0 }, /* Set the socks4 proxy of the server*/ + { "usesrc", srv_parse_usesrc, 0, 1, 1 }, /* safe-guard against usesrc without preceding <source> keyword */ + { "weight", srv_parse_weight, 1, 1, 1 }, /* Set the load-balancing weight */ { NULL, NULL, 0 }, }}; -----Original Message----- From: Willy Tarreau <w...@1wt.eu> Sent: Wednesday, October 18, 2023 9:33 AM To: Stephan, Alexander <alexander.step...@sap.com> Cc: haproxy@formilux.org Subject: Re: [PATCH 2/4] MEDIUM: connection: Send out generically allocated proxy-v2-options Hi Alexander, I'm starting from the doc as it eases the discussion. On Thu, Oct 05, 2023 at 11:05:50AM +0000, Stephan, Alexander wrote: > --- a/doc/configuration.txt > +++ b/doc/configuration.txt > @@ -16671,6 +16671,26 @@ proxy-v2-options <option>[,<option>]* > generated unique ID is also used for the first HTTP request > within a Keep-Alive connection. > > + Besides, you can specify the type of TLV numerically instead. > + > + Example 1: > + server example 127.0.0.1:2319 send-proxy-v2 proxy-v2-options > + crc32c,0xEE > + > + The following logic applies: By default, the remote frontend > + connection is searched for the value 0xEE. If found, it is simply > + forwarded. Otherwise, a TLV with the ID "0xEE" and empty payload is > + sent out. In addition, crc32c is also set here. > + > + You can also specify a key-value pair <id>=<fmt> syntax. <id> > + represents the > + 8 bit TLV type field in the range of 0 to 255. It can be expressed > + in decimal or hexadecimal format (prefixed by "0x"). > + > + Example 2: > + server example_server 127.0.0.1:2319 send-proxy-v2 proxy-v2-options > + 0xEE=%[str("foo")] > + > + This will always send out the value "foo". Another common use case > + would be to reference a variable. I'm really having an issue with using "the remote frontend connection" here. We've done this mistake in the past with the transparent mode that connects to the original destination address, that resulted in "set-dst" having to be implemented, then being broken by multiplexed streams (e.g. h2), then having to be internally emulated at various layers (connection, session, stream, transaction etc). Same for src. The solution is not durable at all, and as you noticed, you already had to implement the ability to modify these fields before passing them, hence reintroducing that class of trouble on a new area. Also what will be left in the logs after you modify the contents ? etc. Given that you had previously contributed the ability to fetch a TLV field from the front connection's proxy protocol using fc_pp_tlv(), I'd rather make use of these ones explicitly. If you need to change them, then you just assign them to a variable and edit that variable, then send the variable. Or you can change them in-flight using a converter. Of course it would make configs slightly more verbose, but they would always do the right thing and we would not have to continue to special-case them over time because they break expectations like set-src/set-dst did. I'm seeing another issue with passing type=fmt like this. The option is currently defined as cutting around commas, and that's what it does. This means that the format will not be parsable (no sample fetch functions with more than one argument, no converters). Example: proxy-v2-options 0xEE=%[var(txn.clientname,_unknown_)] ^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^ First option Second option Also the syntax in the format something=value is not that great for config parsers and generators. However it looks like our server keywords support parenthesis, so I think that instead you could do the following: proxy-v2-options(0xEE) %[var(txn.clientname,_unknown_)] In this case the proxy-v2-options keyword would only add one option and take a log-format, and you'd add as many such keywords for each option. Note that from a configuration perspective this is cleaner as it's easier to maintain value pairs like this for config management. But I don't think the option name is that great especially to send a single one. So maybe this one instead should be "proxy-v2-option(type) <fmt>" though it may maintain some confusion (option vs options). The other solution might be to have a shorter one, which is also nicer when using many of these, e.g. "proxy-v2-opt()". Such an approach would basically split the proxy-v2 usage in two groups: - options that are either there or not there (proxy-v2-options) - options that need to take a value (proxy-v2-opt()). I've been wondering if we want to use a format string or raw binary data here, because some options are binary and nothing indicates that this will not continue in the future. However since fc_pp_tlv() returns a string, I guess most of them will continue this way. For set-var() we're using a raw typed expression, but then we created set-var-fmt() to have one that takes a log format. Just thinking loud, but then what about "proxy-v2-optfmt()" or even "proxy-v2-tlvfmt()" or "pp-tlv-fmt()" (after all when speaking about tlv we imply v2, right?). That would allow us later to implement "pp-tlv()" taking an expression if really needed, e.g. to pass a DER certificate, a SHA256, a sub-tlv or any such a thing. Generally speaking I'm fine with the approach of making the output PP TLV configurable, I think it does bring some value and flexibility in general. That's also why I'm extremely careful about the config shortcomings that can come with it, because I suspect it could become popular and we really want to avoid new fall traps. I think that most of your patch will remain as-is, it's only the parsing part which needs to change a little bit to take such points into account. Also once this is merged, a new problem will emerge. There's still no registry for the PPv2 TLV types. Years ago I've been suggested to write an RFC about it but I just can't take two more hours of my daily sleep time to work on that. I'm seeing that the proxy-protocol doc already reserves a two ranges (one for locally-administered values, one for experiment). I think that the doc for the keyword should put a big caution about this, suggesting to have a look at that section and not to pick random values out of nowhere. Willy
