Hi Willy, I've just finished the implementation based on the tip with the deferred log format parsing so the default-server should be also fully supported now. 😊 I guess using the finalize method of the parser is the canonic implementation of this.
I have a few remarks about the current state of the code: - srv_settings_cpy in server.c has no form of error handling available. This is potentially causes trouble due to lack of error handling: https://github.com/haproxy/haproxy/blob/47ed1181f29a56ccb04e5b80ef4f9414466ada7a/src/server.c#L2370 For now, I did not want to change the signature, but I think, error handling would be beneficial here. - In the new list_for_each in srv_settings_cpy I have to check for srv_tlv == NULL as for some reason, the list contains NULL entries. I don't see any mistakes with the list handling. Maybe it is just due to the structure of the server logic. - Please double check that my arguments for the parse_logformat_string function are correct. I omit log options for now and use the capabilities of the proxy. Seems like the best fit, but I could be wrong. - I noticed that there are also no checks for strdup in server.c, that might need to be addressed in the future. For the srv_settings_cpy I cannot give an error, but only try to avoid the memory leak. The successor of original patch for our replies is down below. I will send the other updated path (now [PATCH 1/2]) to the corresponding submitted mail. Again, please ignore [PATH 3/4] and [PATH 4/4] now. Thanks and best, Alexander --- From 0f8691072ef17ceac98c6fffb063fdacc016a99c Mon Sep 17 00:00:00 2001 From: Alexander Stephan <alexander.step...@sap.com> Date: Sat, 28 Oct 2023 20:57:07 +0200 Subject: [PATCH 2/2] MINOR: connection: Send out generic, user-defined server TLVs To follow-up the implementation of the new set-proxy-v2-tlv-fmt keyword in the server, the connection is updated to use the previously allocated TLVs. If no value was specified, we send out an empty TLV. As the feature is fully working with this commit, documentation and a test for the server and default-server are added as well. --- doc/configuration.txt | 16 ++++ .../proxy_protocol_send_generic.vtc | 74 +++++++++++++++++++ src/connection.c | 36 ++++++++- 3 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 reg-tests/connection/proxy_protocol_send_generic.vtc diff --git a/doc/configuration.txt b/doc/configuration.txt index 3dcdb3e2a..b54e9cd2a 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -16725,6 +16725,22 @@ send-proxy-v2 of this version of the protocol. See also the "no-send-proxy-v2" option of this section and send-proxy" option of the "bind" keyword. +set-proxy-v2-tlv-fmt(<id>) <fmt> + The "set-proxy-v2-tlv-fmt" parameter is used to send arbitrary PROXY protocol + version 2 TLVs. For the type (<id>) range of the defined TLV type please refer + to section 2.2.8. of the proxy protocol specification. However, the value can + be chosen freely as long as it does not exceed the maximum length of 65,535 + bytes. It can also be used for forwarding TLVs by using the fetch "fc_pp_tlv" + to retrieve a received TLV from the frontend. It may be used as a server or + a default-server option. It must be used in combination with send-proxy-v2 + such that PPv2 TLVs are actually sent out. + + Example: + server srv1 192.168.1.1:80 send-proxy-v2 set-proxy-v2-tlv-fmt(0x20) %[fc_pp_tlv(0x20)] + + In this case, we fetch the TLV with the type 0x20 as a string and set as the value + of a newly created TLV that also has the type 0x20. + proxy-v2-options <option>[,<option>]* The "proxy-v2-options" parameter add options to send in PROXY protocol version 2 when "send-proxy-v2" is used. Options available are: diff --git a/reg-tests/connection/proxy_protocol_send_generic.vtc b/reg-tests/connection/proxy_protocol_send_generic.vtc new file mode 100644 index 000000000..f61bcae73 --- /dev/null +++ b/reg-tests/connection/proxy_protocol_send_generic.vtc @@ -0,0 +1,74 @@ +varnishtest "Check that generic TLV IDs are sent properly" + +#REQUIRE_VERSION=2.2 + +feature ignore_unknown_macro + +haproxy h1 -conf { + defaults + mode http + log global + + timeout connect "${HAPROXY_TEST_TIMEOUT-5s}" + timeout client "${HAPROXY_TEST_TIMEOUT-5s}" + timeout server "${HAPROXY_TEST_TIMEOUT-5s}" + + listen sender + bind "fd@${feS}" + server example ${h1_feR_addr}:${h1_feR_port} send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[str("foo")] set-proxy-v2-tlv-fmt(0xE2) + + listen receiver + bind "fd@${feR}" accept-proxy + + # Check that the TLV value is set in the backend. + http-request set-var(txn.custom_tlv_a) fc_pp_tlv(0xE1) + http-after-response set-header proxy_custom_tlv_a %[var(txn.custom_tlv_a)] + + # Check that TLVs without an value are sent out. + http-request set-var(txn.custom_tlv_b) fc_pp_tlv(0xE2) + http-after-response set-header proxy_custom_tlv_b %[var(txn.custom_tlv_b)] + + # Note that we do not check for an invalid TLV ID as that would result in an + # parser error anway. + + http-request return status 200 +} -start + + +client c1 -connect ${h1_feS_sock} { + txreq -url "/" + rxresp + expect resp.http.proxy_custom_tlv_a == "foo" + expect resp.http.proxy_custom_tlv_b == "" +} -run + +# Ensure that we achieve the same via a default-server. +haproxy h2 -conf { + defaults + mode http + log global + + timeout connect "${HAPROXY_TEST_TIMEOUT-5s}" + timeout client "${HAPROXY_TEST_TIMEOUT-5s}" + timeout server "${HAPROXY_TEST_TIMEOUT-5s}" + + listen sender + bind "fd@${feS}" + default-server send-proxy-v2 set-proxy-v2-tlv-fmt(0xE1) %[str("bar")] + server example ${h1_feR_addr}:${h1_feR_port} + + listen receiver + bind "fd@${feR}" accept-proxy + + http-request set-var(txn.custom_tlv_a) fc_pp_tlv(0xE1) + http-after-response set-header proxy_custom_tlv_a %[var(txn.custom_tlv_a)] + + http-request return status 200 +} -start + + +client c2 -connect ${h2_feS_sock} { + txreq -url "/" + rxresp + expect resp.http.proxy_custom_tlv_a == "bar" +} -run diff --git a/src/connection.c b/src/connection.c index 59893fbb5..3a13c2542 100644 --- a/src/connection.c +++ b/src/connection.c @@ -1927,10 +1927,11 @@ static int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct int ret = 0; struct proxy_hdr_v2 *hdr = (struct proxy_hdr_v2 *)buf; struct sockaddr_storage null_addr = { .ss_family = 0 }; + struct srv_pp_tlv_list *srv_tlv = NULL; const struct sockaddr_storage *src = &null_addr; const struct sockaddr_storage *dst = &null_addr; - const char *value; - int value_len; + const char *value = ""; + int value_len = 0; if (buf_len < PP2_HEADER_LEN) return 0; @@ -2000,6 +2001,37 @@ static int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct } } + if (strm) { + struct buffer *replace = NULL; + + 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)) + return 0; + + replace->data = build_logline(strm, replace->area, replace->size, &srv_tlv->fmt); + + if (unlikely((buf_len - ret) < sizeof(struct tlv))) { + free_trash_chunk(replace); + return 0; + } + ret += make_tlv(&buf[ret], (buf_len - ret), srv_tlv->type, replace->data, replace->area); + free_trash_chunk(replace); + } + else { + /* Create empty TLV as no value was specified */ + ret += make_tlv(&buf[ret], (buf_len - ret), srv_tlv->type, 0, NULL); + } + } + } + + /* Handle predefined TLVs as usual */ if (srv->pp_opts & SRV_PP_V2_CRC32C) { uint32_t zero_crc32c = 0; -- 2.35.3 -----Original Message----- From: Willy Tarreau <w...@1wt.eu> Sent: Friday, October 27, 2023 4:22 PM 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, On Fri, Oct 27, 2023 at 02:12:10PM +0000, Stephan, Alexander wrote: > > BTW, please check if this works in default-server directives. > > struct srv_pp_tlv_list { > struct list list; > struct list fmt; > unsigned char type; > }; > > To allow for use with the default server, I adjusted srv_settings_cpy > accordingly such that the server TLV entries (see above) are deep copied. > It works, but there is an edge case with the struct logformat_node > that is contained in such a TLV struct. default-server directives Its > member expr has the type of a void pointer, therefore I cannot > directly copy it. Alternatively, if I would reuse the memory by simply > copying the pointer, a double free will likely happen in srv_drop (I > always free the TLV list in it, alongside the logformat node list). > Besides, the servers created from the default-backend should operate > independently, so this is not an option, I guess. Definitely. From what I remember about what we do for other such expressions that are inherited from defaults, we use a trick: we only save the expression as a string during parsing, that's the same that is kept on the default server. Thus the new server just does an strdup() of that log-format expression that's just a string. And only later we resolve it. That's not the best example but the first I just found, please have a look at uniqueid_format_string. It's handled exactly like this and resolved in check_config_validity(). > > You may want to have a look at how the "sni" keyword which also > > supports an expression is handled, as I don't recall the exact details. > > Maybe in your case we don't need the expression but the log-format > > instead and it's not an issue, I just don't remember the details > > without having a deeper look to be honest. > > Maybe let's just use a sample expression as its usage is more straight > forward, basically similar to the sni option? Or do you have any other > ideas on how to tackle the issue I described above? Parsing the log-format string late definitely is the best option, and not too cumbersome. You can even still report the line number etc from the "server" struct to indicate in the parsing error if any, since everything is on the same line, so there's really no problem with parsing late. > Disabling the default-server support could be another solution. I am > very interested in your opinion on this. I'd really avoid disabling default-server as much as possible, or it will start to be quite difficult to configure given that other related options are accepted there. I tend to consider that the effort needed by users to work around our limited designs often outweighs the efforts we should have involved to make it smoother for them, so until we're certain it's not possible it's worth trying ;-) Thanks! Willy
